Feature/mentioned in commit or upstream commits strategy #12
Conversation
…ras mentioned anywhere in the current commit or any upstream commits.
dalvizu
left a comment
dalvizu
left a comment
There was a problem hiding this comment.
Thanks for the awesome submission @milowg! Thanks for looking at a problem and sharing your solution with the community in a well thought-out pull request! I've left some comments requesting changes before I can merge:
- Some minor style issues
- Remove the getBuildChangeSetEntries() method -- guessing stuff was refactored out at one point?
- additional unit tests on your new functionality
| <configuration> | ||
| <source>1.7</source> | ||
| <target>1.7</target> | ||
| <source>1.8</source> |
| import org.jenkinsci.plugins.jiraext.view.IssueStrategyExtension; | ||
| import org.jenkinsci.plugins.jiraext.view.MentionedInCommitStrategy; | ||
| import org.jenkinsci.plugins.jiraext.view.SingleTicketStrategy; | ||
| import org.jenkinsci.plugins.jiraext.view.*; |
There was a problem hiding this comment.
IDE might have gobbled this up? We don't have a style guide but try to be consistent - please revert.
| return result; | ||
| } | ||
|
|
||
| private void getBuildChangeSetEntries(BuildListener listener, AbstractBuild build, List<Object> changeSetEntries) { |
There was a problem hiding this comment.
Have changeSetEntries be the return object here - there is no value in passing as mutable parameter. A return object is a self-documenting post condition.
There was a problem hiding this comment.
Again no style guide but try to be consistent w/ existing style -- curly brace on newline
| String projectName = build.getProject() == null ? "" : build.getProject().getName(); | ||
| Integer buildNumber = build.getNumber(); | ||
| listener.getLogger().println(String.format("ChangeLogSet from %s build %d, class: %s", projectName, buildNumber, changeSets.getClass())); | ||
| changeSetEntries.addAll(Lists.newArrayList(changeSets.iterator())); |
There was a problem hiding this comment.
Why are you adding all of the elements of an iterator to a list simply to use that list to only iterate over them? The only thing that has been added here is the additional logging describing build and class. Revert this new function and simply add your additional logging.
|
|
||
| /** | ||
| * Find JiraCommits by looking for word in the build's changelog. Issues must match in the pattern defined in | ||
| * global config. Also looks in Jira comments for all upstream commits |
There was a problem hiding this comment.
this looks in git commits, not jira comments
| * @author milowg | ||
| */ | ||
| public class MentionedInCommitOrUpstreamCommitsStrategy | ||
| extends MentionedInCommitStrategy { |
There was a problem hiding this comment.
There is no style guide but try to be consistent w/ existing style -- curly brace on newline
| } | ||
|
|
||
| @Override | ||
| public List<JiraCommit> getJiraCommits(AbstractBuild build, BuildListener buildListener) { |
There was a problem hiding this comment.
There is no style guide but try to be consistent w/ existing style -- curly brace on newline
| public List<JiraCommit> getJiraCommits(AbstractBuild build, BuildListener buildListener) { | ||
| List<JiraCommit> jiraCommits = super.getJiraCommits(build, buildListener); | ||
|
|
||
| for (AbstractBuild upstreamBuild : UpstreamBuildUtil.getUpstreamBuilds(build)) { |
There was a problem hiding this comment.
There is no style guide but try to be consistent w/ existing style -- curly brace on newline
| if (jiraCommits.addAll( | ||
| super.getJiraCommits(upstreamBuild, buildListener) | ||
| .stream() | ||
| .filter(jc -> !jiraCommits.contains(jc)) |
There was a problem hiding this comment.
This is to remove duplicate entries? List is not the data structure you want -- you want to use Set during collection -- that should allow you to avoid looping and just stream the whole thing.
| /** | ||
| * @author dalvizu | ||
| */ | ||
| public class MentionedInCommitOrUpstreamCommitStrategyTest |
There was a problem hiding this comment.
Tests on Discovery behavior are required for submission - see MentionedInCommitStrategyTest.java for inspiration
|
@dalvizu you've made a lot of good suggestions, I will implement the changes. I'm unsure how to proceed with a unit test, however. In order to mock the getJiraCommits method, there are a lot of other things to mock, some of which involve calling static methods or base class methods. Do you have any suggestions on how I can mock it up or make changes to the code to make it more easily unit testable? |
2 participants
Added strategy to use upstream commits in the determination of Jiras