From c8a0827148595e1855ebd91926403438546b5cc0 Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sat, 28 Dec 2024 15:20:42 +0530 Subject: [PATCH 1/9] Add test for PropertyBasedJobInclusionStrategy class --- ...PropertyBasedJobInclusionStrategyTest.java | 196 ++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java diff --git a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java new file mode 100644 index 00000000..912d8b63 --- /dev/null +++ b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java @@ -0,0 +1,196 @@ +package jenkins.advancedqueue.jobinclusion.strategy; + +import static org.junit.Assert.*; + +import hudson.model.FreeStyleProject; +import hudson.util.ListBoxModel; +import java.util.List; +import jenkins.advancedqueue.DecisionLogger; +import jenkins.advancedqueue.JobGroup; +import jenkins.advancedqueue.PriorityConfiguration; +import jenkins.advancedqueue.jobinclusion.strategy.PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + +public class PropertyBasedJobInclusionStrategyTest { + + @Rule + public JenkinsRule jenkinsRule = new JenkinsRule(); + + private FreeStyleProject project; + private PropertyBasedJobInclusionStrategy strategy; + private static DecisionLogger decisionLogger; + + @Before + public void setUp() throws Exception { + project = jenkinsRule.createFreeStyleProject(); + strategy = new PropertyBasedJobInclusionStrategy("testGroup"); + decisionLogger = new DecisionLogger() { + /** + * @param indent + * @param log + * @return + */ + @Override + public DecisionLogger addDecisionLog(int indent, String log) { + return null; + } + }; + } + + @After + public void tearDown() throws Exception { + project.delete(); + } + + @Test + public void getDescriptor() { + PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor descriptor = + strategy.getDescriptor(); + assertNotNull(descriptor); + assertTrue(descriptor.getDisplayName().contains("Jobs")); + } + + @Test + public void all() { + List jobGroups = PriorityConfiguration.get().getJobGroups(); + assertNotNull(jobGroups); + } + + @Test + public void getName() { + assertEquals("testGroup", strategy.getName()); + } + + @Test + public void contains() { + + boolean result = strategy.contains(decisionLogger, project); + assertFalse(result); // Assuming the project does not have the required property + } + + @Test + public void getPropertyBasesJobGroups() { + ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); + assertNotNull(jobGroups); + } + + @Test + public void getDescriptorReturnsNonNullDescriptor() { + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + assertNotNull(descriptor); + } + + @Test + public void getDescriptorDisplayNameContainsJobs() { + PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor descriptor = + strategy.getDescriptor(); + assertTrue(descriptor.getDisplayName().contains("Jobs")); + } + + @Test + public void allReturnsNonNullJobGroups() { + List jobGroups = PriorityConfiguration.get().getJobGroups(); + assertNotNull(jobGroups); + } + + @Test + public void getNameReturnsCorrectName() { + assertEquals("testGroup", strategy.getName()); + } + + @Test + public void containsReturnsFalseForProjectWithoutProperty() { + boolean result = strategy.contains(decisionLogger, project); + assertFalse(result); + } + + @Test + public void getPropertyBasesJobGroupsReturnsNonNullListBoxModel() { + ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); + assertNotNull(jobGroups); + } + + @Test + public void containsReturnsTrueForProjectWithMatchingJobGroup() throws Exception { + project.addProperty(new JobInclusionJobProperty(true, "testGroup")); + + boolean result = strategy.contains(decisionLogger, project); + assertTrue(result); + } + + @Test + public void containsReturnsFalseForProjectWithNonMatchingJobGroup() throws Exception { + project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); + + boolean result = strategy.contains(decisionLogger, project); + assertFalse(result); + } + + @Test + public void containsReturnsFalseWhenCloudBeesFoldersPluginNotEnabled() throws Exception { + // Simulate CloudBees Folders plugin not enabled + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + descriptor.cloudbeesFolders = false; + + boolean result = strategy.contains(decisionLogger, project); + assertFalse(result); + } + + @Test + public void getPropertyBasesJobGroupsReturnsEmptyListBoxModelWhenNoJobGroups() { + // Simulate no job groups + PriorityConfiguration.get().getJobGroups().clear(); + + ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); + assertNotNull(jobGroups); + assertTrue(jobGroups.isEmpty()); + } + + @Test + public void containsReturnsTrueForProjectWithMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { + project.addProperty(new JobInclusionJobProperty(true, "testGroup")); + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + descriptor.cloudbeesFolders = true; + + boolean result = strategy.contains(decisionLogger, project); + assertTrue(result); + } + + @Test + public void containsReturnsFalseForProjectWithNonMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { + project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + descriptor.cloudbeesFolders = true; + + boolean result = strategy.contains(decisionLogger, project); + assertFalse(result); + } + + @Test + public void containsReturnsFalseForProjectWithoutPropertyAndCloudBeesFoldersEnabled() throws Exception { + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + descriptor.cloudbeesFolders = true; + + boolean result = strategy.contains(decisionLogger, project); + assertFalse(result); + } + + @Test + public void getPropertyBasesJobGroupsReturnsNonNullListBoxModelWhenMultipleJobGroups() { + // Simulate multiple job groups + PriorityConfiguration.get() + .getJobGroups() + .add(new JobGroup("group1", 1, new PropertyBasedJobInclusionStrategy("group1"))); + PriorityConfiguration.get() + .getJobGroups() + .add(new JobGroup("group2", 2, new PropertyBasedJobInclusionStrategy("group2"))); + + ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); + assertNotNull(jobGroups); + assertEquals(2, jobGroups.size()); + } +} From 432b04b46cc465df94252d7febf8d25fb2a7559f Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sat, 28 Dec 2024 15:24:03 +0530 Subject: [PATCH 2/9] Made cloudbeesFolder package-private --- .../strategy/PropertyBasedJobInclusionStrategy.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java index 80465fa3..804f0cb4 100644 --- a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java +++ b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java @@ -45,7 +45,7 @@ public class PropertyBasedJobInclusionStrategy extends JobInclusionStrategy { @Extension public static class PropertyBasedJobInclusionStrategyDescriptor extends Descriptor { - private boolean cloudbeesFolders = true; + boolean cloudbeesFolders = true; @Override public String getDisplayName() { @@ -65,6 +65,11 @@ public PropertyBasedJobInclusionStrategyDescriptor() { } ; + @Override + public PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor getDescriptor() { + return (PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor) super.getDescriptor(); + } + private String name; @DataBoundConstructor From 6b7f3b16ce6e45b968b8a8243690e06207daee18 Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sat, 28 Dec 2024 15:26:55 +0530 Subject: [PATCH 3/9] Add constructor to JobGroup class Added a constructor to initialize description, priority, and jobGroupStrategy fields. This change allows creating JobGroup instances with specific values for these fields. --- src/main/java/jenkins/advancedqueue/JobGroup.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/main/java/jenkins/advancedqueue/JobGroup.java b/src/main/java/jenkins/advancedqueue/JobGroup.java index 15202053..64f594c6 100644 --- a/src/main/java/jenkins/advancedqueue/JobGroup.java +++ b/src/main/java/jenkins/advancedqueue/JobGroup.java @@ -31,6 +31,7 @@ import java.util.ArrayList; import java.util.List; import jenkins.advancedqueue.jobinclusion.JobInclusionStrategy; +import jenkins.advancedqueue.jobinclusion.strategy.PropertyBasedJobInclusionStrategy; import jenkins.advancedqueue.jobinclusion.strategy.ViewBasedJobInclusionStrategy; import jenkins.advancedqueue.priority.PriorityStrategy; import jenkins.model.Jenkins; @@ -47,6 +48,12 @@ */ public class JobGroup { + public JobGroup(String name, int priority, PropertyBasedJobInclusionStrategy jobGroupStrategy) { + this.description = name; + this.priority = priority; + this.jobGroupStrategy = jobGroupStrategy; + } + public static class PriorityStrategyHolder extends PriorityStrategy { private int id = 0; private PriorityStrategy priorityStrategy; From eb18e0096cebe7ab0389bf8fa3ea5b183c16b25e Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Sat, 28 Dec 2024 18:06:57 +0530 Subject: [PATCH 4/9] Changed FreestyleProject from project to j --- ...PropertyBasedJobInclusionStrategyTest.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java index 912d8b63..ba0bf458 100644 --- a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java @@ -20,13 +20,13 @@ public class PropertyBasedJobInclusionStrategyTest { @Rule public JenkinsRule jenkinsRule = new JenkinsRule(); - private FreeStyleProject project; + private FreeStyleProject j; private PropertyBasedJobInclusionStrategy strategy; private static DecisionLogger decisionLogger; @Before public void setUp() throws Exception { - project = jenkinsRule.createFreeStyleProject(); + j = jenkinsRule.createFreeStyleProject(); strategy = new PropertyBasedJobInclusionStrategy("testGroup"); decisionLogger = new DecisionLogger() { /** @@ -43,7 +43,7 @@ public DecisionLogger addDecisionLog(int indent, String log) { @After public void tearDown() throws Exception { - project.delete(); + j.delete(); } @Test @@ -68,7 +68,7 @@ public void getName() { @Test public void contains() { - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertFalse(result); // Assuming the project does not have the required property } @@ -104,7 +104,7 @@ public void getNameReturnsCorrectName() { @Test public void containsReturnsFalseForProjectWithoutProperty() { - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertFalse(result); } @@ -116,17 +116,17 @@ public void getPropertyBasesJobGroupsReturnsNonNullListBoxModel() { @Test public void containsReturnsTrueForProjectWithMatchingJobGroup() throws Exception { - project.addProperty(new JobInclusionJobProperty(true, "testGroup")); + j.addProperty(new JobInclusionJobProperty(true, "testGroup")); - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertTrue(result); } @Test public void containsReturnsFalseForProjectWithNonMatchingJobGroup() throws Exception { - project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); + j.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertFalse(result); } @@ -136,7 +136,7 @@ public void containsReturnsFalseWhenCloudBeesFoldersPluginNotEnabled() throws Ex PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = false; - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertFalse(result); } @@ -152,21 +152,21 @@ public void getPropertyBasesJobGroupsReturnsEmptyListBoxModelWhenNoJobGroups() { @Test public void containsReturnsTrueForProjectWithMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { - project.addProperty(new JobInclusionJobProperty(true, "testGroup")); + j.addProperty(new JobInclusionJobProperty(true, "testGroup")); PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = true; - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertTrue(result); } @Test public void containsReturnsFalseForProjectWithNonMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { - project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); + j.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = true; - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertFalse(result); } @@ -175,7 +175,7 @@ public void containsReturnsFalseForProjectWithoutPropertyAndCloudBeesFoldersEnab PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = true; - boolean result = strategy.contains(decisionLogger, project); + boolean result = strategy.contains(decisionLogger, j); assertFalse(result); } From 0a971b0560c1913b8c094efd210b5090fd226c15 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sat, 28 Dec 2024 09:42:07 -0700 Subject: [PATCH 5/9] Check results from decision logger Do not use wildcard import of org.junit.Assert because that includes the deprecated assertThat method. Use the assertThat method from hamcrest because it is not deprecated and works very well with hamcrest assertions. Name the strategy uniquely for each test by including the test name in the strategy name. The TestName Rule is a very handy way to assure that test data is unique for each test method without needing to worry about the uniqueness directly. Split the Before methods so that they can be named more specifically. --- ...PropertyBasedJobInclusionStrategyTest.java | 116 +++++++++++++----- 1 file changed, 86 insertions(+), 30 deletions(-) diff --git a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java index ba0bf458..6e539911 100644 --- a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java @@ -1,9 +1,17 @@ package jenkins.advancedqueue.jobinclusion.strategy; -import static org.junit.Assert.*; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import hudson.model.FreeStyleProject; import hudson.util.ListBoxModel; +import java.util.ArrayList; import java.util.List; import jenkins.advancedqueue.DecisionLogger; import jenkins.advancedqueue.JobGroup; @@ -11,39 +19,53 @@ import jenkins.advancedqueue.jobinclusion.strategy.PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor; import org.junit.After; import org.junit.Before; +import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TestName; import org.jvnet.hudson.test.JenkinsRule; public class PropertyBasedJobInclusionStrategyTest { + @ClassRule + public static JenkinsRule j = new JenkinsRule(); + @Rule - public JenkinsRule jenkinsRule = new JenkinsRule(); + public TestName testName = new TestName(); - private FreeStyleProject j; + private FreeStyleProject project; + private String strategyName; private PropertyBasedJobInclusionStrategy strategy; - private static DecisionLogger decisionLogger; + private DecisionLogger decisionLogger; + private List loggedMessages; + + @Before + public void createStrategy() throws Exception { + strategyName = "testGroup-" + testName.getMethodName(); + strategy = new PropertyBasedJobInclusionStrategy(strategyName); + } @Before - public void setUp() throws Exception { - j = jenkinsRule.createFreeStyleProject(); - strategy = new PropertyBasedJobInclusionStrategy("testGroup"); + public void createProject() throws Exception { + // Name each freestyle project with the name of the test creating it + project = j.createFreeStyleProject("freestyle-" + testName.getMethodName()); + } + + @Before + public void createDecisionLogger() throws Exception { + loggedMessages = new ArrayList<>(); decisionLogger = new DecisionLogger() { - /** - * @param indent - * @param log - * @return - */ @Override public DecisionLogger addDecisionLog(int indent, String log) { - return null; + loggedMessages.add(log); + return this; } }; } @After - public void tearDown() throws Exception { - j.delete(); + public void deleteProject() throws Exception { + project.delete(); } @Test @@ -52,36 +74,41 @@ public void getDescriptor() { strategy.getDescriptor(); assertNotNull(descriptor); assertTrue(descriptor.getDisplayName().contains("Jobs")); + assertThat(loggedMessages, is(empty())); } @Test public void all() { List jobGroups = PriorityConfiguration.get().getJobGroups(); assertNotNull(jobGroups); + assertThat(loggedMessages, is(empty())); } @Test public void getName() { - assertEquals("testGroup", strategy.getName()); + assertEquals(strategyName, strategy.getName()); + assertThat(loggedMessages, is(empty())); } @Test public void contains() { - - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertFalse(result); // Assuming the project does not have the required property + assertThat(loggedMessages, hasItems("No match ...")); } @Test public void getPropertyBasesJobGroups() { ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); assertNotNull(jobGroups); + assertThat(loggedMessages, is(empty())); } @Test public void getDescriptorReturnsNonNullDescriptor() { PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); assertNotNull(descriptor); + assertThat(loggedMessages, is(empty())); } @Test @@ -89,45 +116,60 @@ public void getDescriptorDisplayNameContainsJobs() { PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); assertTrue(descriptor.getDisplayName().contains("Jobs")); + assertThat(loggedMessages, is(empty())); } @Test public void allReturnsNonNullJobGroups() { List jobGroups = PriorityConfiguration.get().getJobGroups(); assertNotNull(jobGroups); + assertThat(loggedMessages, is(empty())); } @Test public void getNameReturnsCorrectName() { - assertEquals("testGroup", strategy.getName()); + assertEquals(strategyName, strategy.getName()); + assertThat(loggedMessages, is(empty())); } @Test public void containsReturnsFalseForProjectWithoutProperty() { - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertFalse(result); + assertThat(loggedMessages, hasItems("No match ...")); } @Test public void getPropertyBasesJobGroupsReturnsNonNullListBoxModel() { ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); assertNotNull(jobGroups); + assertThat(loggedMessages, is(empty())); } @Test public void containsReturnsTrueForProjectWithMatchingJobGroup() throws Exception { - j.addProperty(new JobInclusionJobProperty(true, "testGroup")); + project.addProperty(new JobInclusionJobProperty(true, strategyName)); - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertTrue(result); + assertThat( + loggedMessages, + hasItems( + "JobGroup is enabled on job, with JobGroup [" + strategyName + "] ...", + "Job is included in JobGroup ...")); } @Test public void containsReturnsFalseForProjectWithNonMatchingJobGroup() throws Exception { - j.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); + project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertFalse(result); + assertThat( + loggedMessages, + hasItems( + "JobGroup is enabled on job, with JobGroup [nonMatchingGroup] ...", + "Job is not included in JobGroup ...")); } @Test @@ -136,8 +178,9 @@ public void containsReturnsFalseWhenCloudBeesFoldersPluginNotEnabled() throws Ex PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = false; - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertFalse(result); + assertThat(loggedMessages, hasItems("Checking for Job Property inclusion for [" + strategyName + "]...")); } @Test @@ -148,26 +191,37 @@ public void getPropertyBasesJobGroupsReturnsEmptyListBoxModelWhenNoJobGroups() { ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); assertNotNull(jobGroups); assertTrue(jobGroups.isEmpty()); + assertThat(loggedMessages, is(empty())); } @Test public void containsReturnsTrueForProjectWithMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { - j.addProperty(new JobInclusionJobProperty(true, "testGroup")); + project.addProperty(new JobInclusionJobProperty(true, strategyName)); PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = true; - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertTrue(result); + assertThat( + loggedMessages, + hasItems( + "JobGroup is enabled on job, with JobGroup [" + strategyName + "] ...", + "Job is included in JobGroup ...")); } @Test public void containsReturnsFalseForProjectWithNonMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { - j.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); + project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = true; - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertFalse(result); + assertThat( + loggedMessages, + hasItems( + "JobGroup is enabled on job, with JobGroup [nonMatchingGroup] ...", + "Job is not included in JobGroup ...")); } @Test @@ -175,8 +229,9 @@ public void containsReturnsFalseForProjectWithoutPropertyAndCloudBeesFoldersEnab PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); descriptor.cloudbeesFolders = true; - boolean result = strategy.contains(decisionLogger, j); + boolean result = strategy.contains(decisionLogger, project); assertFalse(result); + assertThat(loggedMessages, hasItems("No match ...")); } @Test @@ -192,5 +247,6 @@ public void getPropertyBasesJobGroupsReturnsNonNullListBoxModelWhenMultipleJobGr ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); assertNotNull(jobGroups); assertEquals(2, jobGroups.size()); + assertThat(loggedMessages, is(empty())); } } From e8e919a0dad4511afbfb5110600e7bbc0e1e4929 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 29 Dec 2024 14:22:08 -0700 Subject: [PATCH 6/9] Replace JobGroup changes with more test code Don't need a new constructor when setters are already available for all the attributes used in the new constructor. --- .../java/jenkins/advancedqueue/JobGroup.java | 7 ------- .../PropertyBasedJobInclusionStrategyTest.java | 17 ++++++++++------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/main/java/jenkins/advancedqueue/JobGroup.java b/src/main/java/jenkins/advancedqueue/JobGroup.java index 64f594c6..15202053 100644 --- a/src/main/java/jenkins/advancedqueue/JobGroup.java +++ b/src/main/java/jenkins/advancedqueue/JobGroup.java @@ -31,7 +31,6 @@ import java.util.ArrayList; import java.util.List; import jenkins.advancedqueue.jobinclusion.JobInclusionStrategy; -import jenkins.advancedqueue.jobinclusion.strategy.PropertyBasedJobInclusionStrategy; import jenkins.advancedqueue.jobinclusion.strategy.ViewBasedJobInclusionStrategy; import jenkins.advancedqueue.priority.PriorityStrategy; import jenkins.model.Jenkins; @@ -48,12 +47,6 @@ */ public class JobGroup { - public JobGroup(String name, int priority, PropertyBasedJobInclusionStrategy jobGroupStrategy) { - this.description = name; - this.priority = priority; - this.jobGroupStrategy = jobGroupStrategy; - } - public static class PriorityStrategyHolder extends PriorityStrategy { private int id = 0; private PriorityStrategy priorityStrategy; diff --git a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java index 6e539911..f2590386 100644 --- a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java @@ -236,17 +236,20 @@ public void containsReturnsFalseForProjectWithoutPropertyAndCloudBeesFoldersEnab @Test public void getPropertyBasesJobGroupsReturnsNonNullListBoxModelWhenMultipleJobGroups() { - // Simulate multiple job groups - PriorityConfiguration.get() - .getJobGroups() - .add(new JobGroup("group1", 1, new PropertyBasedJobInclusionStrategy("group1"))); - PriorityConfiguration.get() - .getJobGroups() - .add(new JobGroup("group2", 2, new PropertyBasedJobInclusionStrategy("group2"))); + PriorityConfiguration.get().getJobGroups().add(createJobGroup("group1", 1)); + PriorityConfiguration.get().getJobGroups().add(createJobGroup("group2", 2)); ListBoxModel jobGroups = PropertyBasedJobInclusionStrategy.getPropertyBasesJobGroups(); assertNotNull(jobGroups); assertEquals(2, jobGroups.size()); assertThat(loggedMessages, is(empty())); } + + private JobGroup createJobGroup(String description, int priority) { + JobGroup group = new JobGroup(); + group.setDescription(description); + group.setPriority(priority); + group.setJobGroupStrategy(new PropertyBasedJobInclusionStrategy(description)); + return group; + } } From a87dd5263589ec9af7984a5b2db3daa0d301b241 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Sun, 29 Dec 2024 16:46:23 -0700 Subject: [PATCH 7/9] Rename getDescriptor and make it package protected for tests Reduce the amount of change in the production object by not creating a new method that overrides the method from the superclass. Limit the accessibility of the new method so that it is package protected and not available to classes outside its own package. Remove two duplicated test methods --- .../PropertyBasedJobInclusionStrategy.java | 5 ++-- ...PropertyBasedJobInclusionStrategyTest.java | 28 ++++--------------- 2 files changed, 9 insertions(+), 24 deletions(-) diff --git a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java index 804f0cb4..2035bd20 100644 --- a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java +++ b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java @@ -65,8 +65,9 @@ public PropertyBasedJobInclusionStrategyDescriptor() { } ; - @Override - public PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor getDescriptor() { + /* Package protected for testing */ + /* Intentionally does not override getDescriptor() from the super class */ + PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor getThisDescriptor() { return (PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor) super.getDescriptor(); } diff --git a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java index f2590386..1af0f6ac 100644 --- a/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java +++ b/src/test/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategyTest.java @@ -68,15 +68,6 @@ public void deleteProject() throws Exception { project.delete(); } - @Test - public void getDescriptor() { - PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor descriptor = - strategy.getDescriptor(); - assertNotNull(descriptor); - assertTrue(descriptor.getDisplayName().contains("Jobs")); - assertThat(loggedMessages, is(empty())); - } - @Test public void all() { List jobGroups = PriorityConfiguration.get().getJobGroups(); @@ -105,16 +96,9 @@ public void getPropertyBasesJobGroups() { } @Test - public void getDescriptorReturnsNonNullDescriptor() { - PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); - assertNotNull(descriptor); - assertThat(loggedMessages, is(empty())); - } - - @Test - public void getDescriptorDisplayNameContainsJobs() { + public void getDisplayNameContainsJobs() { PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor descriptor = - strategy.getDescriptor(); + strategy.getThisDescriptor(); assertTrue(descriptor.getDisplayName().contains("Jobs")); assertThat(loggedMessages, is(empty())); } @@ -175,7 +159,7 @@ public void containsReturnsFalseForProjectWithNonMatchingJobGroup() throws Excep @Test public void containsReturnsFalseWhenCloudBeesFoldersPluginNotEnabled() throws Exception { // Simulate CloudBees Folders plugin not enabled - PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getThisDescriptor(); descriptor.cloudbeesFolders = false; boolean result = strategy.contains(decisionLogger, project); @@ -197,7 +181,7 @@ public void getPropertyBasesJobGroupsReturnsEmptyListBoxModelWhenNoJobGroups() { @Test public void containsReturnsTrueForProjectWithMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { project.addProperty(new JobInclusionJobProperty(true, strategyName)); - PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getThisDescriptor(); descriptor.cloudbeesFolders = true; boolean result = strategy.contains(decisionLogger, project); @@ -212,7 +196,7 @@ public void containsReturnsTrueForProjectWithMatchingPropertyAndCloudBeesFolders @Test public void containsReturnsFalseForProjectWithNonMatchingPropertyAndCloudBeesFoldersEnabled() throws Exception { project.addProperty(new JobInclusionJobProperty(true, "nonMatchingGroup")); - PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getThisDescriptor(); descriptor.cloudbeesFolders = true; boolean result = strategy.contains(decisionLogger, project); @@ -226,7 +210,7 @@ public void containsReturnsFalseForProjectWithNonMatchingPropertyAndCloudBeesFol @Test public void containsReturnsFalseForProjectWithoutPropertyAndCloudBeesFoldersEnabled() throws Exception { - PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getDescriptor(); + PropertyBasedJobInclusionStrategyDescriptor descriptor = strategy.getThisDescriptor(); descriptor.cloudbeesFolders = true; boolean result = strategy.contains(decisionLogger, project); From 71b2acb47a41deebbc6df702b866754efa604162 Mon Sep 17 00:00:00 2001 From: Yash Pal Date: Mon, 30 Dec 2024 15:24:06 +0530 Subject: [PATCH 8/9] Improve documentation and add tests for PropertyBasedJobInclusionStrategy - Added detailed Javadoc comments for `getThisDescriptor` and `getDescriptor` methods. - Enhanced inline comments to clarify the use of `@SuppressWarnings("unchecked")`. --- .../advancedqueue/jobinclusion/JobInclusionStrategy.java | 2 ++ .../strategy/PropertyBasedJobInclusionStrategy.java | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/main/java/jenkins/advancedqueue/jobinclusion/JobInclusionStrategy.java b/src/main/java/jenkins/advancedqueue/jobinclusion/JobInclusionStrategy.java index e32d93d3..8cd99544 100644 --- a/src/main/java/jenkins/advancedqueue/jobinclusion/JobInclusionStrategy.java +++ b/src/main/java/jenkins/advancedqueue/jobinclusion/JobInclusionStrategy.java @@ -53,6 +53,8 @@ public String getDisplayName() { } ; + // Suppress unchecked cast warning because Jenkins.get().getDescriptor(this.getClass()) + // returns a Descriptor of the specific JobInclusionStrategy subclass, which is safe to cast @SuppressWarnings("unchecked") public Descriptor getDescriptor() { return Jenkins.get().getDescriptor(this.getClass()); diff --git a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java index 2035bd20..212b5896 100644 --- a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java +++ b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java @@ -67,6 +67,11 @@ public PropertyBasedJobInclusionStrategyDescriptor() { /* Package protected for testing */ /* Intentionally does not override getDescriptor() from the super class */ + /** + * Returns the descriptor for the PropertyBasedJobInclusionStrategy class. + * + * @return PropertyBasedJobInclusionStrategyDescriptor + */ PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor getThisDescriptor() { return (PropertyBasedJobInclusionStrategy.PropertyBasedJobInclusionStrategyDescriptor) super.getDescriptor(); } From cd2455adce187c9d24814f22f390d6e987edaf50 Mon Sep 17 00:00:00 2001 From: Mark Waite Date: Mon, 30 Dec 2024 06:38:48 -0700 Subject: [PATCH 9/9] Add package protected comment --- .../jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java index 212b5896..cb3c6088 100644 --- a/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java +++ b/src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java @@ -45,6 +45,7 @@ public class PropertyBasedJobInclusionStrategy extends JobInclusionStrategy { @Extension public static class PropertyBasedJobInclusionStrategyDescriptor extends Descriptor { + /* Package protected for testing */ boolean cloudbeesFolders = true; @Override