Add test for Sorter strategy #439
Conversation
Refactored the onCanceledItem method to invoke item.isCancelled for test
yashpal2104
force-pushed
the
sorterStrategyTest
branch
from
67e4fd9 to
54ca69c
Compare
|
@MarkEWaite do let me know about the changes I made. Are they viable or not |
MarkEWaite
changed the title
|
As a general pattern, most Jenkins plugin maintainers don't review draft pull requests. A pull request is usually marked as draft by the submitter because they are not ready for review. Since you asked for my feedback, I checked the CI job. There is a spotbugs warning. You need to fix the spotbus warning before I'll spend time reviewing the pull request in any other way. |
yashpal2104
force-pushed
the
sorterStrategyTest
branch
from
059229f to
49af843
Compare
| @@ -23,7 +23,10 @@ | |||
| */ | |||
| package jenkins.advancedqueue.sorter; | |||
|
|
|||
| import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.LOGGER; | |||
There was a problem hiding this comment.
Using the logger from a class that is outside this plugin is not the way to add logging to a class. Refer to src/main/java/jenkins/advancedqueue/PriorityConfiguration.java for the technique that is used to add a logger to a class.
| import static org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.LOGGER; |
| @@ -66,7 +69,12 @@ public void onStartedItem(@NonNull LeftItem item, float weight) {} | |||
| /** | |||
| * Called when a {@link hudson.model.Item} leaves the queue and it is canceled. | |||
| */ | |||
| public void onCanceledItem(@NonNull LeftItem item) {} | |||
| @CheckReturnValue | |||
There was a problem hiding this comment.
The CheckReturnValue javadoc says:
This annotation is used to denote a method whose return value should always be checked when invoking the method.
Since the method is void, it has no return value. That seems like the wrong annotation for the method
-Corrected the annotation method for void method and deleted @checkfornull -Imported Logger from the same plugin
Testing done
Submitter checklist