Add test for JobInclusionJobProperty class #423

yashpal2104 · 2024-12-28T12:30:16Z

Testing done

Add test for JobInclusionJobProperty class

Fixes JENKINS-69757

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira

yashpal2104 · 2024-12-28T17:38:04Z

Any feedback would be appreciated 👍🏻. Thanks

MarkEWaite

Assertions need to be strengthened. Many changes could happen that would still satisfy the not null assertion.

MarkEWaite · 2024-12-28T19:54:01Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+
+    @Test
+    public void getDescriptor() {
+        assertNotNull(property.getDescriptor());


I'd like a deeper assertion than not null.

MarkEWaite · 2024-12-28T19:54:05Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+    @Test
+    public void getJobAction() {
+        // Assuming getJobAction returns some action
+        assertNotNull(property.getJobActions(project));


I'd like a deeper assertion than not null.

MarkEWaite · 2024-12-28T19:54:10Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+    @Test
+    public void getJobActions() {
+        // Assuming getJobActions returns a list of actions
+        assertNotNull(property.getJobActions(project));


I'd like a deeper assertion than not null.

MarkEWaite · 2024-12-28T19:55:05Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+    @Test
+    public void getRequiredMonitorService() {
+        // Assuming getRequiredMonitorService returns some service
+        assertNotNull(property.getRequiredMonitorService());


I'd like a deeper assertion than not null.

MarkEWaite · 2024-12-28T19:55:09Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+    @Test
+    public void getProjectActions() {
+        // Assuming getProjectAction returns some project action
+        assertNotNull(property.getProjectActions(project));


I'd like a deeper assertion than not null.

yashpal2104 · 2024-12-28T20:02:06Z

Right I will deepen the tests for extensive testing next thing in the morning.

…perty for extensive tests - Introduce `addJobGroup` method to add job groups with display name and value. - Introduce `getJobGroups` method to retrieve the list of job groups.

… into job-inclusion-job-property-test # Conflicts: # src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java

- Introduce `addJobGroup` method to add job groups with display name and value. - Introduce `getJobGroups` method to retrieve the list of job groups.

yashpal2104

Do let me know about these changes I made, I added a constructor to add and get JobGroups to extensively test and deepen the assertions in PropertyBasedJobInclusionStrategy class

…ertions

MarkEWaite

Thanks for the pull request. I spent several hours with it yesterday. I did not see the benefit of the changes to the production object. The tests that use those changes seem to only test the changes themselves, not any of the already existing functionality.

I've added several other comments as well.

Note that some of the comments propose changes that are incomplete. You'll need to complete the changes on a local copy from your integrated development environment, because they propose to rename a field of the test object. Rename a field is easier from an IDE.

MarkEWaite · 2024-12-30T13:25:41Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

-    public JenkinsRule jenkinsRule = new JenkinsRule();
+    public TestName testName = new TestName();
+
+    private JobInclusionJobProperty property;


Please reduce the differences to the existing test class by changing this to jobProperty in all its usages.

Suggested change

private JobInclusionJobProperty property;

private JobInclusionJobProperty jobProperty;

MarkEWaite · 2024-12-30T13:26:52Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

-        jobProperty = new JobInclusionJobProperty(true, "TestJobGroup");
-        descriptor = new JobInclusionJobProperty.DescriptorImpl();
+    public void setUp() throws Exception {
+        property = new JobInclusionJobProperty(true, "testGroup");


Please reduce this difference to the existing test class, since changing the name of the property does not help the test and it makes the review of differences more difficult.

Suggested change

property = new JobInclusionJobProperty(true, "testGroup");

jobProperty = new JobInclusionJobProperty(true, "TestJobGroup");

MarkEWaite · 2024-12-30T13:28:21Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+    public void prebuild() {
+        // Assuming prebuild performs some pre-build actions
+        assertTrue(property.prebuild(build, listener));
+
+        // Additional assertions to verify the state after prebuild
+        assertNotNull(build);
+        assertNotNull(listener);
+        assertEquals(
+                "testFolder_" + testName.getMethodName(), build.getProject().getName());
+    }
+
+    @Test
+    public void perform() throws IOException, InterruptedException {
+        // Assuming perform executes some actions
+        assertTrue(property.perform(build, launcher, listener));
+
+        // Additional assertions to verify the state after perform
+        assertNotNull(build);
+        assertNotNull(launcher);
+        assertNotNull(listener);
+        assertEquals(
+                "testFolder_" + testName.getMethodName(), build.getProject().getName());
+    }


There are methods that are inherited from the parent object. I'd rather not have tests of those methods in this class. Leave that test to the parent object. That will also allow you to remove the declarations of launcher and listener that are only used in these two tests.

Suggested change

public void prebuild() {

// Assuming prebuild performs some pre-build actions

assertTrue(property.prebuild(build, listener));

// Additional assertions to verify the state after prebuild

assertNotNull(build);

assertNotNull(listener);

assertEquals(

"testFolder_" + testName.getMethodName(), build.getProject().getName());

}

@Test

public void perform() throws IOException, InterruptedException {

// Assuming perform executes some actions

assertTrue(property.perform(build, launcher, listener));

// Additional assertions to verify the state after perform

assertNotNull(build);

assertNotNull(launcher);

assertNotNull(listener);

assertEquals(

"testFolder_" + testName.getMethodName(), build.getProject().getName());

}

MarkEWaite · 2024-12-30T13:29:23Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

    }

    @Test
-    public void getJobGroupNameTest() {


Please don't delete the existing tests (like getJobGroupNameTest). Your pull request should add to the tests rather than replacing existing tests.

MarkEWaite · 2024-12-30T13:30:00Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

    }

    @Test
-    public void getDescriptorTest() {


Another case of deleting a test. I don't see the benefit of deleting this test.

MarkEWaite · 2024-12-30T13:31:04Z

src/test/java/jenkins/advancedqueue/jobinclusion/strategy/JobInclusionJobPropertyTest.java

+    @Test
+    public void getJobGroupsTest() {
+        JobInclusionJobProperty.DescriptorImpl descriptor = new JobInclusionJobProperty.DescriptorImpl();
+        ListBoxModel jobGroups = descriptor.getJobGroups();
+
+        jobGroups.add("testJobGroup", "testGroup");
+        // Verify that the jobGroups is not null
+        assertNotNull(jobGroups);
+
+        // Verify that the jobGroups is not empty
+        assertFalse(jobGroups.isEmpty());
+
+        // Verify the size of the jobGroups
+        assertEquals("Expected number of job groups", 1, jobGroups.size());
+
+        // Verify the properties of the first job group
+        ListBoxModel.Option firstOption = jobGroups.get(0);
+        assertNotNull(firstOption);
+        assertEquals("Expected display name", "testJobGroup", firstOption.name);
+        assertEquals("Expected value", "testGroup", firstOption.value);
+    }


This test seems to be only testing the additions to the JobInclusionJobProperty object. As far as I can tell, those additions are not needed.

Suggested change

@Test

public void getJobGroupsTest() {

JobInclusionJobProperty.DescriptorImpl descriptor = new JobInclusionJobProperty.DescriptorImpl();

ListBoxModel jobGroups = descriptor.getJobGroups();

jobGroups.add("testJobGroup", "testGroup");

// Verify that the jobGroups is not null

assertNotNull(jobGroups);

// Verify that the jobGroups is not empty

assertFalse(jobGroups.isEmpty());

// Verify the size of the jobGroups

assertEquals("Expected number of job groups", 1, jobGroups.size());

// Verify the properties of the first job group

ListBoxModel.Option firstOption = jobGroups.get(0);

assertNotNull(firstOption);

assertEquals("Expected display name", "testJobGroup", firstOption.name);

assertEquals("Expected value", "testGroup", firstOption.value);

}

MarkEWaite · 2024-12-30T13:31:38Z

...main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java

@@ -41,6 +41,7 @@
 * @since 3.0
 */
 public class PropertyBasedJobInclusionStrategy extends JobInclusionStrategy {
+    private static ListBoxModel jobGroups = new ListBoxModel();


As far as I can tell, this addition is not needed.

Suggested change

private static ListBoxModel jobGroups = new ListBoxModel();

MarkEWaite · 2024-12-30T13:32:00Z

...main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java

+
+    public static void addJobGroup(String displayName, String value) {
+        jobGroups.add(displayName, value);
+    }
+
+    public static ListBoxModel getJobGroups() {
+        return jobGroups;
+    }


As far as I can tell, this addition is not needed.

Suggested change

public static void addJobGroup(String displayName, String value) {

jobGroups.add(displayName, value);

}

public static ListBoxModel getJobGroups() {

return jobGroups;

}

MarkEWaite

Thanks!

yashpal2104 · 2025-01-01T17:34:59Z

Hi @MarkEWaite ,

Thank you for taking the time to review my pull request and provide detailed feedback. I truly appreciate the effort, especially during the holidays.

Thanks again for your guidance!

Add test for JobInclusionJobProperty class

36c051b

yashpal2104 requested a review from a team as a code owner December 28, 2024 12:30

github-actions bot added the tests Automated test addition or improvement label Dec 28, 2024

MarkEWaite added 2 commits December 28, 2024 10:15

Merge branch 'master' into job-inclusion-job-property-test

1907f53

Remove unused code from test

435a167

Merge branch 'master' into job-inclusion-job-property-test

95a20a7

MarkEWaite requested changes Dec 28, 2024

View reviewed changes

MarkEWaite and others added 8 commits December 28, 2024 13:18

Merge branch 'master' into job-inclusion-job-property-test

b342fc1

Merge branch 'master' into job-inclusion-job-property-test

d161d4d

Merge branch 'master' into job-inclusion-job-property-test

b1d8e33

Add static methods to manage and add job groups in JobInclusionJobPro…

fbaa413

…perty for extensive tests - Introduce `addJobGroup` method to add job groups with display name and value. - Introduce `getJobGroups` method to retrieve the list of job groups.

Add static methods to manage and add job groups in JobInclusionJobPro…

a3de303

…perty for extensive tests - Introduce `addJobGroup` method to add job groups with display name and value. - Introduce `getJobGroups` method to retrieve the list of job groups.

Merge remote-tracking branch 'origin/job-inclusion-job-property-test'…

7c9da76

… into job-inclusion-job-property-test # Conflicts: # src/main/java/jenkins/advancedqueue/jobinclusion/strategy/PropertyBasedJobInclusionStrategy.java

Add extensive test assertions

e414328

- Introduce `addJobGroup` method to add job groups with display name and value. - Introduce `getJobGroups` method to retrieve the list of job groups.

Add extensive test assertions

66d4611

- Introduce `addJobGroup` method to add job groups with display name and value. - Introduce `getJobGroups` method to retrieve the list of job groups.

yashpal2104 commented Dec 29, 2024

View reviewed changes

Removed getRequiredMonitorService method since not able to deepen ass…

fa8f326

…ertions

MarkEWaite requested changes Dec 30, 2024

View reviewed changes

yashpal2104 and others added 5 commits December 30, 2024 20:37

Added necessary changes made by review

f574e59

Added necessary changes made by review

0171109

Merge branch 'master' into job-inclusion-job-property-test

d9bb6b6

Reduce diffs to master branch

6dc0488

Reduce diffs to master branch

97489d8

MarkEWaite self-requested a review January 1, 2025 17:00

MarkEWaite approved these changes Jan 1, 2025

View reviewed changes

MarkEWaite enabled auto-merge (squash) January 1, 2025 17:00

MarkEWaite merged commit 5bad03a into jenkinsci:master Jan 1, 2025
17 of 18 checks passed

yashpal2104 deleted the job-inclusion-job-property-test branch January 1, 2025 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for JobInclusionJobProperty class #423

Add test for JobInclusionJobProperty class #423

yashpal2104 commented Dec 28, 2024

yashpal2104 commented Dec 28, 2024

MarkEWaite left a comment

MarkEWaite Dec 28, 2024

MarkEWaite Dec 28, 2024

MarkEWaite Dec 28, 2024

MarkEWaite Dec 28, 2024

MarkEWaite Dec 28, 2024

yashpal2104 commented Dec 28, 2024

yashpal2104 left a comment •

edited

Loading

MarkEWaite left a comment •

edited

Loading

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite Dec 30, 2024

MarkEWaite left a comment

yashpal2104 commented Jan 1, 2025

	private JobInclusionJobProperty property;
	private JobInclusionJobProperty jobProperty;

	property = new JobInclusionJobProperty(true, "testGroup");
	jobProperty = new JobInclusionJobProperty(true, "TestJobGroup");

Add test for JobInclusionJobProperty class #423

Add test for JobInclusionJobProperty class #423

Conversation

yashpal2104 commented Dec 28, 2024

Testing done

Submitter checklist

yashpal2104 commented Dec 28, 2024

MarkEWaite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashpal2104 commented Dec 28, 2024

yashpal2104 left a comment • edited Loading

Choose a reason for hiding this comment

MarkEWaite left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

yashpal2104 commented Jan 1, 2025

yashpal2104 left a comment •

edited

Loading

MarkEWaite left a comment •

edited

Loading