Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for JobInclusionJobProperty class #423

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
* @since 3.0
*/
public class PropertyBasedJobInclusionStrategy extends JobInclusionStrategy {
private static ListBoxModel jobGroups = new ListBoxModel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
private static ListBoxModel jobGroups = new ListBoxModel();


@Extension
public static class PropertyBasedJobInclusionStrategyDescriptor extends Descriptor<JobInclusionStrategy> {
Expand Down Expand Up @@ -119,4 +120,12 @@ public static ListBoxModel getPropertyBasesJobGroups() {
}
return strategies;
}

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

public static ListBoxModel getJobGroups() {
return jobGroups;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,53 +3,244 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import hudson.Launcher;
import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.model.StreamBuildListener;
import hudson.util.ListBoxModel;
import hudson.util.StreamTaskListener;
import java.io.IOException;
import java.io.PrintStream;
import java.nio.charset.StandardCharsets;
import jenkins.advancedqueue.PriorityConfiguration;
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 JobInclusionJobPropertyTest {
private JobInclusionJobProperty jobProperty;
private JobInclusionJobProperty.DescriptorImpl descriptor;

@ClassRule
public static JenkinsRule j = new JenkinsRule();

@Rule
public JenkinsRule jenkinsRule = new JenkinsRule();
public TestName testName = new TestName();

private JobInclusionJobProperty property;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

private FreeStyleProject project;
private FreeStyleBuild build;
private StreamBuildListener listener;
private Launcher launcher;

@Before
public void setup() {
jobProperty = new JobInclusionJobProperty(true, "TestJobGroup");
descriptor = new JobInclusionJobProperty.DescriptorImpl();
public void setUp() throws Exception {
property = new JobInclusionJobProperty(true, "testGroup");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

project = j.createFreeStyleProject("testFolder_" + testName.getMethodName());
build = j.buildAndAssertSuccess(project);
listener = new StreamBuildListener(new PrintStream(System.out), StandardCharsets.UTF_8);
launcher = new hudson.Launcher.LocalLauncher(StreamTaskListener.fromStdout());
}

@After
public void deleteProject() throws Exception {
project.delete();
}

@Test
public void getDescriptor() {
JobInclusionJobProperty.DescriptorImpl descriptor = property.getDescriptor();
assertNotNull(descriptor);
assertEquals("XXX", descriptor.getDisplayName());
assertTrue(descriptor instanceof JobInclusionJobProperty.DescriptorImpl);
}

@Test
public void getJobActions() {
// Setup: Add a job group to the PriorityConfiguration
PriorityConfiguration.get().getJobGroups().clear(); // Clear existing job groups
PropertyBasedJobInclusionStrategy.addJobGroup("testJobGroup", "testGroup");

// Retrieve job actions
ListBoxModel jobActions = PropertyBasedJobInclusionStrategy.getJobGroups();
assertNotNull(jobActions);
assertFalse(jobActions.isEmpty());

// Check the size of the list
assertEquals("Expected number of job actions", 1, jobActions.size());

// Check the properties of the first element
ListBoxModel.Option firstOption = jobActions.get(0);
assertNotNull(firstOption);
assertEquals("Expected display name", "testJobGroup", firstOption.name);
assertEquals("Expected value", "testGroup", firstOption.value);
}

@Test
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());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
}


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a deeper assertion than not null.

}

@Test
public void getJobGroupNameReturnsCorrectName() throws Exception {
FreeStyleProject myProject = j.createFreeStyleProject("test-project");
JobInclusionJobProperty jobProperty = new JobInclusionJobProperty(true, "testJobGroupName");
myProject.addProperty(jobProperty);
assertEquals("testJobGroupName", jobProperty.getJobGroupName());
}

@Test
public void getJobGroupNameReturnsNullWhenNotSet() {
// Create a JobInclusionJobProperty with useJobGroup set to false and jobGroupName set to null
JobInclusionJobProperty jobProperty = new JobInclusionJobProperty(false, null);
assertFalse(jobProperty.isUseJobGroup());
assertNull(jobProperty.getJobGroupName());

// Create a JobInclusionJobProperty with useJobGroup set to true and jobGroupName set to null
JobInclusionJobProperty jobPropertyTrue = new JobInclusionJobProperty(true, null);
assertTrue(jobPropertyTrue.isUseJobGroup());
assertNull(jobPropertyTrue.getJobGroupName());

// Create a JobInclusionJobProperty with useJobGroup set to false and jobGroupName set to a non-null value
JobInclusionJobProperty jobPropertyWithGroupName = new JobInclusionJobProperty(false, "testJobGroupName");
assertFalse(jobPropertyWithGroupName.isUseJobGroup());
assertEquals("testJobGroupName", jobPropertyWithGroupName.getJobGroupName());
}

@Test
public void getJobGroupNameTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertEquals("TestJobGroup", jobProperty.getJobGroupName());
public void isUseJobGroupReturnsCorrectValue() {
// Create a JobInclusionJobProperty with useJobGroup set to true
JobInclusionJobProperty jobProperty = new JobInclusionJobProperty(true, "groupName");
assertTrue(jobProperty.isUseJobGroup());
assertEquals("groupName", jobProperty.getJobGroupName());

// Create a JobInclusionJobProperty with useJobGroup set to false
JobInclusionJobProperty jobPropertyFalse = new JobInclusionJobProperty(false, "groupName");
assertFalse(jobPropertyFalse.isUseJobGroup());
assertEquals("groupName", jobPropertyFalse.getJobGroupName());

// Create a JobInclusionJobProperty with null jobGroupName
JobInclusionJobProperty jobPropertyNull = new JobInclusionJobProperty(true, null);
assertTrue(jobPropertyNull.isUseJobGroup());
assertNull(jobPropertyNull.getJobGroupName());
}

@Test
public void isUseJobGroupTest() {
// Create a JobInclusionJobProperty with useJobGroup set to true
JobInclusionJobProperty jobProperty = new JobInclusionJobProperty(true, "groupName1");
assertTrue(jobProperty.isUseJobGroup());
assertEquals("groupName1", jobProperty.getJobGroupName());

// Create a JobInclusionJobProperty with useJobGroup set to false
JobInclusionJobProperty jobPropertyFalse = new JobInclusionJobProperty(false, "groupName2");
assertFalse(jobPropertyFalse.isUseJobGroup());
assertEquals("groupName2", jobPropertyFalse.getJobGroupName());

// Create a JobInclusionJobProperty with null jobGroupName
JobInclusionJobProperty jobPropertyNull = new JobInclusionJobProperty(true, null);
assertTrue(jobPropertyNull.isUseJobGroup());
assertNull(jobPropertyNull.getJobGroupName());
}

@Test
public void getDescriptorTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertNotNull(jobProperty.getDescriptor());
public void getJobGroupNameReturnsNullWhenJobGroupNameNotSet() {
// Create a JobInclusionJobProperty with useJobGroup set to true and jobGroupName set to null
JobInclusionJobProperty jobProperty = new JobInclusionJobProperty(true, null);
assertTrue(jobProperty.isUseJobGroup());
assertNull(jobProperty.getJobGroupName());

// Create a JobInclusionJobProperty with useJobGroup set to false and jobGroupName set to null
JobInclusionJobProperty jobPropertyFalse = new JobInclusionJobProperty(false, null);
assertFalse(jobPropertyFalse.isUseJobGroup());
assertNull(jobPropertyFalse.getJobGroupName());

// Create a JobInclusionJobProperty with useJobGroup set to true and jobGroupName set to a non-null value
JobInclusionJobProperty jobPropertyWithGroupName = new JobInclusionJobProperty(true, "testJobGroupName");
assertTrue(jobPropertyWithGroupName.isUseJobGroup());
assertEquals("testJobGroupName", jobPropertyWithGroupName.getJobGroupName());
}

@Test
public void getDisplayNameTest() {
public void descriptorImplGetDisplayName() {
JobInclusionJobProperty.DescriptorImpl descriptor = new JobInclusionJobProperty.DescriptorImpl();
assertEquals("XXX", descriptor.getDisplayName());
}

@Test
public void getJobGroupsTest() {
assertNotNull(descriptor.getJobGroups());
public void getDescriptorTest() {
JobInclusionJobProperty jobProperty = new JobInclusionJobProperty(true, null);
JobInclusionJobProperty.DescriptorImpl descriptor = jobProperty.getDescriptor();

// Verify that the descriptor is not null
assertNotNull(descriptor);

// Verify the display name of the descriptor
assertEquals("XXX", descriptor.getDisplayName());

// Verify the type of the descriptor
assertTrue(descriptor instanceof JobInclusionJobProperty.DescriptorImpl);

// Verify the isUsed method of the descriptor
assertFalse(descriptor.isUsed());
}

@Test
public void isUsedTest() {
public void descriptorImplIsUsed() {
JobInclusionJobProperty.DescriptorImpl descriptor = new JobInclusionJobProperty.DescriptorImpl();
assertFalse(descriptor.isUsed());
}

@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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

}
Loading