From 5b6669a88ab327a0a599a061693e9f4a988ffe5d Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sun, 5 Jan 2025 00:24:26 +0530 Subject: [PATCH 1/8] Add test for MultiBucketStrategy class --- .../strategy/MultiBucketStrategyTest.java | 118 ++++++++++++++++++ 1 file changed, 118 insertions(+) create mode 100644 src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java diff --git a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java new file mode 100644 index 00000000..a4e1a4f2 --- /dev/null +++ b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java @@ -0,0 +1,118 @@ +package jenkins.advancedqueue.sorter.strategy; + +import static org.junit.Assert.assertEquals; + +import hudson.model.Queue; +import hudson.util.ListBoxModel; +import jenkins.advancedqueue.sorter.SorterStrategyCallback; +import org.jetbrains.annotations.NotNull; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +public class MultiBucketStrategyTest extends MultiBucketStrategy { + + /** + * @param item the {@link hudson.model.Queue.WaitingItem} or {@link hudson.model.BuildableItem} that + * enters the queue + * @param weightCallback the callback holds the priority to use anded the called method must set + * the weight before returning + * @return + */ + @Override + public SorterStrategyCallback onNewItem(@NotNull Queue.Item item, SorterStrategyCallback weightCallback) { + return weightCallback; + } + + @ClassRule + public static JenkinsRule j = new JenkinsRule(); + + private static MultiBucketStrategy strategy; + private static MultiBucketStrategy.MultiBucketStrategyDescriptor descriptor; + private static final int DEFAULT_PRIORITIES_NUMBER = 10; + private static final int DEFAULT_PRIORITY = 5; + + @Before + public void setUp() { + strategy = new MultiBucketStrategy(DEFAULT_PRIORITIES_NUMBER, DEFAULT_PRIORITY) { + @Override + public SorterStrategyCallback onNewItem(@NotNull Queue.Item item, SorterStrategyCallback weightCallback) { + return weightCallback; + } + }; + + descriptor = new MultiBucketStrategy.MultiBucketStrategyDescriptor() { + @Override + public String getShortName() { + return ""; + } + + @Override + protected MultiBucketStrategy getStrategy() { + return strategy; + } + }; + } + + @Test + public void getNumberOfPrioritiesReturnsCorrectValue() { + assertEquals(10, strategy.getNumberOfPriorities()); + } + + @Test + public void getDefaultPriorityReturnsCorrectValue() { + assertEquals(5, strategy.getDefaultPriority()); + } + + @Test + public void doFillDefaultPriorityItemsReturnsCorrectItems() { + ListBoxModel items = descriptor.doFillDefaultPriorityItems(); + assertEquals(10, items.size()); + for (int i = 0; i < 10; i++) { + assertEquals(String.valueOf(i + 1), items.get(i).value); + } + } + + @Test + public void doUpdateDefaultPriorityItemsHandlesInvalidInput() { + ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("invalid"); + assertEquals(3, items.size()); + for (int i = 0; i < 3; i++) { + assertEquals(String.valueOf(i + 1), items.get(i).value); + } + } + + @Test + public void doUpdateDefaultPriorityItemsHandlesValidInput() { + ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("3"); + assertEquals(3, items.size()); + for (int i = 0; i < 3; i++) { + assertEquals(String.valueOf(i + 1), items.get(i).value); + } + } + + @Test + public void doUpdateDefaultPriorityItemsHandlesNegativeInput() { + ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("-1"); + assertEquals(3, items.size()); + for (int i = 0; i < 3; i++) { + assertEquals(String.valueOf(i + 1), items.get(i).value); + } + } + + @Test + public void doUpdateDefaultPriorityItemsHandlesZeroInput() { + ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("0"); + assertEquals(0, items.size()); + } + + @Test + public void doUpdateDefaultPriorityItemsHandlesLargeInput() { + ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("100"); + assertEquals(100, items.size()); + for (int i = 0; i < 100; i++) { + assertEquals(String.valueOf(i + 1), items.get(i).value); + } + } +} From 62a1e7b7cccba75e5d8882d5a54e4e91ac3b2243 Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sun, 5 Jan 2025 00:26:24 +0530 Subject: [PATCH 2/8] Made getStrategy method package-protected and condition for non-integer input. --- .../sorter/strategy/MultiBucketStrategy.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java index b9a34e62..467ea63c 100644 --- a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java +++ b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java @@ -64,7 +64,7 @@ public final int getDefaultPriority() { return defaultPriority; } - public ListBoxModel doFillDefaultPriorityItems() { + public ListBoxModel doFillDefaultPriorityItemsDynamicRetrieval() { // TODO: replace by dynamic retrieval throw new RuntimeException(); } @@ -75,6 +75,9 @@ public ListBoxModel doUpdateDefaultPriorityItems(@QueryParameter("value") String int value = DEFAULT_PRIORITY; try { value = Integer.parseInt(strValue); + if (value < 0) { + value = DEFAULT_PRIORITY; + } } catch (NumberFormatException e) { // Use default value } @@ -95,8 +98,9 @@ private ListBoxModel internalFillDefaultPriorityItems(int value) { return items; } + /* package-protected */ @CheckForNull - private MultiBucketStrategy getStrategy() { + MultiBucketStrategy getStrategy() { SorterStrategy strategy = PrioritySorterConfiguration.get().getStrategy(); if (strategy == null || !(strategy instanceof MultiBucketStrategy)) { return null; From 68a093875230587dd0437e9165253ec31026dc23 Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sun, 5 Jan 2025 00:29:22 +0530 Subject: [PATCH 3/8] Revert the name back to original --- .../advancedqueue/sorter/strategy/MultiBucketStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java index 467ea63c..32330c0e 100644 --- a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java +++ b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java @@ -64,7 +64,7 @@ public final int getDefaultPriority() { return defaultPriority; } - public ListBoxModel doFillDefaultPriorityItemsDynamicRetrieval() { + public ListBoxModel doFillDefaultPriorityItems() { // TODO: replace by dynamic retrieval throw new RuntimeException(); } From 04735b6a86e972505a542e3574bc632b962963af Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 6 Jan 2025 17:46:45 -0700 Subject: [PATCH 4/8] Remove behavior change from production object Value is set to 0 if a negative value is passed. --- .../advancedqueue/sorter/strategy/MultiBucketStrategy.java | 3 --- .../sorter/strategy/MultiBucketStrategyTest.java | 5 +---- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java index 32330c0e..29eaed19 100644 --- a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java +++ b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java @@ -75,9 +75,6 @@ public ListBoxModel doUpdateDefaultPriorityItems(@QueryParameter("value") String int value = DEFAULT_PRIORITY; try { value = Integer.parseInt(strValue); - if (value < 0) { - value = DEFAULT_PRIORITY; - } } catch (NumberFormatException e) { // Use default value } diff --git a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java index a4e1a4f2..5a7f91b8 100644 --- a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java @@ -95,10 +95,7 @@ public void doUpdateDefaultPriorityItemsHandlesValidInput() { @Test public void doUpdateDefaultPriorityItemsHandlesNegativeInput() { ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("-1"); - assertEquals(3, items.size()); - for (int i = 0; i < 3; i++) { - assertEquals(String.valueOf(i + 1), items.get(i).value); - } + assertEquals(0, items.size()); } @Test From 5d9a4ee4f1e507fe002c3269d88e43d302d4a4ac Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 6 Jan 2025 17:47:50 -0700 Subject: [PATCH 5/8] Adjust comment for package protected method --- .../advancedqueue/sorter/strategy/MultiBucketStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java index 29eaed19..40c4aa6a 100644 --- a/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java +++ b/src/main/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategy.java @@ -95,7 +95,7 @@ private ListBoxModel internalFillDefaultPriorityItems(int value) { return items; } - /* package-protected */ + /* package-protected for testing */ @CheckForNull MultiBucketStrategy getStrategy() { SorterStrategy strategy = PrioritySorterConfiguration.get().getStrategy(); From 438d441f59322f2f1b3632e01f2f53eaf83e1f2f Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 6 Jan 2025 17:48:17 -0700 Subject: [PATCH 6/8] Use spotbugs NonNull instead of Atlassian NotNull --- .../sorter/strategy/MultiBucketStrategyTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java index 5a7f91b8..4e1cfbdc 100644 --- a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java @@ -2,10 +2,10 @@ import static org.junit.Assert.assertEquals; +import edu.umd.cs.findbugs.annotations.NonNull; import hudson.model.Queue; import hudson.util.ListBoxModel; import jenkins.advancedqueue.sorter.SorterStrategyCallback; -import org.jetbrains.annotations.NotNull; import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; @@ -21,7 +21,7 @@ public class MultiBucketStrategyTest extends MultiBucketStrategy { * @return */ @Override - public SorterStrategyCallback onNewItem(@NotNull Queue.Item item, SorterStrategyCallback weightCallback) { + public SorterStrategyCallback onNewItem(@NonNull Queue.Item item, SorterStrategyCallback weightCallback) { return weightCallback; } @@ -37,7 +37,7 @@ public SorterStrategyCallback onNewItem(@NotNull Queue.Item item, SorterStrategy public void setUp() { strategy = new MultiBucketStrategy(DEFAULT_PRIORITIES_NUMBER, DEFAULT_PRIORITY) { @Override - public SorterStrategyCallback onNewItem(@NotNull Queue.Item item, SorterStrategyCallback weightCallback) { + public SorterStrategyCallback onNewItem(@NonNull Queue.Item item, SorterStrategyCallback weightCallback) { return weightCallback; } }; From 8d73868f1b2c5ecc51102037458e6eb670f40cd7 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 6 Jan 2025 17:49:17 -0700 Subject: [PATCH 7/8] Inherit javadoc from the parent object --- .../sorter/strategy/MultiBucketStrategyTest.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java index 4e1cfbdc..0275d8a3 100644 --- a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java @@ -13,13 +13,7 @@ public class MultiBucketStrategyTest extends MultiBucketStrategy { - /** - * @param item the {@link hudson.model.Queue.WaitingItem} or {@link hudson.model.BuildableItem} that - * enters the queue - * @param weightCallback the callback holds the priority to use anded the called method must set - * the weight before returning - * @return - */ + /** @{inheritDoc} */ @Override public SorterStrategyCallback onNewItem(@NonNull Queue.Item item, SorterStrategyCallback weightCallback) { return weightCallback; From c0b5af5ebda2e79fce7e4094f403af470549492a Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 6 Jan 2025 18:32:17 -0700 Subject: [PATCH 8/8] Clarify source of test values Show when default is from test vs. parent object --- .../strategy/MultiBucketStrategyTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java index 0275d8a3..cc7aede2 100644 --- a/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/sorter/strategy/MultiBucketStrategyTest.java @@ -24,12 +24,12 @@ public SorterStrategyCallback onNewItem(@NonNull Queue.Item item, SorterStrategy private static MultiBucketStrategy strategy; private static MultiBucketStrategy.MultiBucketStrategyDescriptor descriptor; - private static final int DEFAULT_PRIORITIES_NUMBER = 10; - private static final int DEFAULT_PRIORITY = 5; + private static final int TEST_PRIORITY_COUNT = 10; + private static final int TEST_PRIORITY_DEFAULT = 5; @Before public void setUp() { - strategy = new MultiBucketStrategy(DEFAULT_PRIORITIES_NUMBER, DEFAULT_PRIORITY) { + strategy = new MultiBucketStrategy(TEST_PRIORITY_COUNT, TEST_PRIORITY_DEFAULT) { @Override public SorterStrategyCallback onNewItem(@NonNull Queue.Item item, SorterStrategyCallback weightCallback) { return weightCallback; @@ -51,19 +51,19 @@ protected MultiBucketStrategy getStrategy() { @Test public void getNumberOfPrioritiesReturnsCorrectValue() { - assertEquals(10, strategy.getNumberOfPriorities()); + assertEquals(TEST_PRIORITY_COUNT, strategy.getNumberOfPriorities()); } @Test public void getDefaultPriorityReturnsCorrectValue() { - assertEquals(5, strategy.getDefaultPriority()); + assertEquals(TEST_PRIORITY_DEFAULT, strategy.getDefaultPriority()); } @Test public void doFillDefaultPriorityItemsReturnsCorrectItems() { ListBoxModel items = descriptor.doFillDefaultPriorityItems(); - assertEquals(10, items.size()); - for (int i = 0; i < 10; i++) { + assertEquals(TEST_PRIORITY_COUNT, items.size()); + for (int i = 0; i < TEST_PRIORITY_COUNT; i++) { assertEquals(String.valueOf(i + 1), items.get(i).value); } } @@ -71,8 +71,8 @@ public void doFillDefaultPriorityItemsReturnsCorrectItems() { @Test public void doUpdateDefaultPriorityItemsHandlesInvalidInput() { ListBoxModel items = descriptor.doUpdateDefaultPriorityItems("invalid"); - assertEquals(3, items.size()); - for (int i = 0; i < 3; i++) { + assertEquals(MultiBucketStrategy.DEFAULT_PRIORITY, items.size()); + for (int i = 0; i < MultiBucketStrategy.DEFAULT_PRIORITY; i++) { assertEquals(String.valueOf(i + 1), items.get(i).value); } }