Skip to content

Commit

Permalink
Extract jobGroup creation to a separate method
Browse files Browse the repository at this point in the history
Avoid duplicating code that is common between uses.

Use a job group name that is distinct for each test so that the single
JenkinsRule instance does not risk having the tests collide in their
use of the job group.
  • Loading branch information
MarkEWaite committed Jan 11, 2025
1 parent 1c2dbb4 commit 4ae7296
Showing 1 changed file with 20 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,17 @@
import hudson.model.ListView;
import java.io.IOException;
import java.util.List;
import java.util.Random;
import jenkins.advancedqueue.JobGroup;
import jenkins.advancedqueue.PriorityConfiguration;
import jenkins.advancedqueue.PrioritySorterConfiguration;
import jenkins.advancedqueue.jobinclusion.strategy.ViewBasedJobInclusionStrategy;
import net.sf.json.JSONObject;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestName;
import org.jvnet.hudson.test.JenkinsRule;
import org.kohsuke.stapler.StaplerRequest;

Expand All @@ -28,6 +31,9 @@ public class PriorityJobPropertyTest {
@ClassRule
public static JenkinsRule j = new JenkinsRule();

@Rule
public TestName testName = new TestName();

private static PriorityJobProperty property;
private static PriorityJobProperty.DescriptorImpl descriptor;
private static FreeStyleProject project;
Expand Down Expand Up @@ -67,6 +73,18 @@ public void descriptorImpl_getPrioritiesReturnsNonEmptyList() {
assertFalse(descriptor.getPriorities().isEmpty());
}

private final Random random = new Random();

private JobGroup createJobGroup() {
JobGroup jobGroup = new JobGroup();
jobGroup.setDescription("testGroup-" + testName.getMethodName());
jobGroup.setRunExclusive(random.nextBoolean());
jobGroup.setUsePriorityStrategies(random.nextBoolean());
jobGroup.setId(random.nextInt());
jobGroup.setJobGroupStrategy(new ViewBasedJobInclusionStrategy("existingView")); // Use the newly created view
return jobGroup;
}

@Test
public void descriptorImpl_isUsedReturnsTrueWhenJobGroupUsesPriorityStrategies() throws IOException {
// Initialize PrioritySorterConfiguration
Expand All @@ -86,15 +104,10 @@ public void descriptorImpl_isUsedReturnsTrueWhenJobGroupUsesPriorityStrategies()
assertNotNull(j.jenkins.getView("existingView"));

// Set up the PriorityJobProperty.DescriptorImpl
descriptor = new PriorityJobProperty.DescriptorImpl();
PriorityConfiguration configuration = PriorityConfiguration.get();
List<JobGroup> jobGroups = configuration.getJobGroups();
JobGroup jobGroup = new JobGroup();
jobGroup.setDescription("testGroup");
jobGroup.setRunExclusive(true);
JobGroup jobGroup = createJobGroup();
jobGroup.setUsePriorityStrategies(true);
jobGroup.setId(1);
jobGroup.setJobGroupStrategy(new ViewBasedJobInclusionStrategy("existingView")); // Use the newly created view

// Add a PriorityStrategyHolder with a JobPropertyStrategy to the JobGroup
JobPropertyStrategy jobPropertyStrategy = new JobPropertyStrategy();
Expand All @@ -114,12 +127,7 @@ public void descriptorImpl_isUsedReturnsFalseWhenJobGroupDoesNotUsePriorityStrat
descriptor = new PriorityJobProperty.DescriptorImpl();
PriorityConfiguration configuration = PriorityConfiguration.get();
List<JobGroup> jobGroups = configuration.getJobGroups();
JobGroup jobGroup = new JobGroup();
jobGroup.setDescription("testGroup");
jobGroup.setRunExclusive(false);
jobGroup.setUsePriorityStrategies(false);
jobGroup.setId(1);
jobGroup.setJobGroupStrategy(new ViewBasedJobInclusionStrategy("defaultView")); // Set a default strategy
JobGroup jobGroup = createJobGroup();
jobGroups.add(jobGroup);
configuration.setJobGroups(jobGroups);
assertFalse(descriptor.isUsed(project));
Expand Down

3 comments on commit 4ae7296

@yashpal2104
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @MarkEWaite you told to Use a job group name distinct for each test then why did you change
ViewBasedJobInclusionStrategy("existingView") and removed ViewBasedJobInclusionStrategy("defaultView")

@MarkEWaite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @MarkEWaite you told to Use a job group name distinct for each test then why did you change ViewBasedJobInclusionStrategy("existingView") and removed ViewBasedJobInclusionStrategy("defaultView")

Nice catch! For safety, the new view based job inclusion strategy should use a distinct name just as the job group does. I'll submit a pull request to fix that mistake.

@MarkEWaite
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please sign in to comment.