Improve test coverage (#371)

* [Jenkins-69756] Improve test coverage

* Improve Test Coverage of NodeLabelParameter

* Broaden the temp offline eligibility tests

Start the Jenkins controller only once for the entire test so that it
runs faster.  This is more important on Windows than on Linux, but is not
harmful because the tests configure their needed Jenkins configuration
at the start of each test.

Use nodeName as the isEligible argument rather than label string.
Label string did not match a node so would always return false.
Remove the label declaration since it is not needed for these tests.

Confirm that the Jenkins controller is eligible when it has executors
and is not eligible when it does not have executors.

Check more details of the descriptors.

* Broaden LabelParameterDefinitionTest

Assert more values are set by the constructors

Use both deprecated constructors in a test and mark the test as deprecated
so that the compiler does not warn about the use of deprecated methods.

Test agent and controller doList and test a case that doList fails to
find the agent label.

* Test more getters and more branches of doListNodes

The doListNodes checks now cover failure cases

* More clear assertion in one test

* Test getNextLabels and deprecated constructors more deeply

The getNextLabels test needs JenkinsRule and an agent, so those are
added to the test.

The deprecated constructor tests now check more values that are assigned
during object construction.

* Expand assertions in NodeParameterDefinitionTest

Unexpected that myValue is not returned by parameterValue.getValue().
Seems to be a bug in the NodeParameterDefinition implementation.
NodeParameterDefinition declares a private field 'label' that receives
the value instead of it being stored in LabelParameterValue.label but
does not override the LabelParameterValue implementation of getValue().

Expanded coverage by using different triggerIfResult values that change
the behavior of the constructor.

Prefer agents in local variables rather than slaves.

---------

Co-authored-by: Mark Waite <[email protected]>

src/test/java/org/jvnet/jenkins/plugins/nodelabelparameter/IgnoreTempOfflineNodeEligibilityTest.java

@@ -0,0 +1,94 @@
+package org.jvnet.jenkins.plugins.nodelabelparameter;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.hasProperty;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import hudson.DescriptorExtensionList;
+import hudson.model.Node;
+import hudson.slaves.DumbSlave;
+import java.util.Random;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.IgnoreTempOfflineNodeEligibility;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.NodeEligibility;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.NodeEligibility.NodeEligibilityDescriptor;
+
+public class IgnoreTempOfflineNodeEligibilityTest {
+    @ClassRule
+    public static JenkinsRule j = new JenkinsRule();
+
+    private static DumbSlave onlineNode1;
+
+    @BeforeClass
+    public static void createAgent() throws Exception {
+        onlineNode1 = j.createOnlineSlave();
+    }
+
+    private final IgnoreTempOfflineNodeEligibility ignoreTempOfflineNodeEligibility =
+            new IgnoreTempOfflineNodeEligibility();
+    private final Random random = new Random();
+
+    @Test
+    public void testGetComputer() throws Exception {
+        // Does not matter if executors on the Jenkins controller are enabled
+        j.jenkins.setNumExecutors(random.nextBoolean() ? 1 : 0);
+
+        // Null node is never eligible
+        Node node = null;
+        assertFalse(ignoreTempOfflineNodeEligibility.isEligible(node));
+        // Non-existent node name is never eligible
+        assertFalse(ignoreTempOfflineNodeEligibility.isEligible("not-a-valid-node"));
+
+        // Online node is always eligible
+        assertTrue(ignoreTempOfflineNodeEligibility.isEligible(onlineNode1));
+        // Online node name is always eligible
+        assertTrue(ignoreTempOfflineNodeEligibility.isEligible(onlineNode1.getNodeName()));
+    }
+
+    @Test
+    public void testGetComputerWithControllerExecutor() throws Exception {
+        // Enable executors on the Jenkins controller
+        j.jenkins.setNumExecutors(1);
+
+        // Node of the Jenkins controller is eligible because it has executors
+        assertTrue(ignoreTempOfflineNodeEligibility.isEligible(j.jenkins));
+        // Empty node name is eligible because empty string matches the controller
+        assertTrue(ignoreTempOfflineNodeEligibility.isEligible(""));
+        // Node name of the Jenkins controller is eligible because it has executors
+        assertTrue(ignoreTempOfflineNodeEligibility.isEligible("built-in"));
+    }
+
+    @Test
+    public void testGetComputerWithoutControllerExecutor() throws Exception {
+        // Disable executors on the Jenkins controller
+        j.jenkins.setNumExecutors(0);
+
+        // Node of the Jenkins controller is not eligible because it has no executors
+        assertFalse(ignoreTempOfflineNodeEligibility.isEligible(j.jenkins));
+        // Empty node name is not eligible because empty string matches the controller
+        assertFalse(ignoreTempOfflineNodeEligibility.isEligible(""));
+        // Node name of the Jenkins controller is not eligible because it has no executors
+        assertFalse(ignoreTempOfflineNodeEligibility.isEligible("built-in"));
+    }
+
+    @Test
+    public void testGetDescriptor() {
+        NodeEligibilityDescriptor descriptor = ignoreTempOfflineNodeEligibility.getDescriptor();
+        assertThat(descriptor.getDisplayName(), is("Ignore Temp Offline Nodes"));
+    }
+
+    @Test
+    public void testAll() {
+        DescriptorExtensionList<NodeEligibility, NodeEligibilityDescriptor> descriptors = NodeEligibility.all();
+        assertThat(descriptors, hasItem(hasProperty("displayName", is("All Nodes"))));
+        assertThat(descriptors, hasItem(hasProperty("displayName", is("Ignore Offline Nodes"))));
+        assertThat(descriptors, hasItem(hasProperty("displayName", is("Ignore Temp Offline Nodes"))));
+        assertThat(descriptors.size(), is(3));
+    }
+}

src/test/java/org/jvnet/jenkins/plugins/nodelabelparameter/LabelParameterDefinitionTest.java

@@ -0,0 +1,151 @@
+package org.jvnet.jenkins.plugins.nodelabelparameter;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import hudson.model.labels.LabelAtom;
+import hudson.slaves.DumbSlave;
+import hudson.util.FormValidation;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.AllNodeEligibility;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.IgnoreOfflineNodeEligibility;
+
+public class LabelParameterDefinitionTest {
+
+    @ClassRule
+    public static JenkinsRule j = new JenkinsRule();
+
+    private static DumbSlave agent;
+
+    private static final String LABEL_NAME = "my-agent-label";
+    private static final LabelAtom label = new LabelAtom(LABEL_NAME);
+
+    @BeforeClass
+    public static void createAgent() throws Exception {
+        agent = j.createOnlineSlave(label);
+    }
+
+    @Test
+    @Deprecated
+    public void testNodeParameterDefinitionDeprecated() {
+        String name = "name";
+        String description = "The description";
+        String defaultValue = "built-in || master";
+        String triggerIfResult = "The triggerIfResult value";
+
+        LabelParameterDefinition nodeParameterDefinition =
+                new LabelParameterDefinition(name, description, defaultValue, true, true, triggerIfResult);
+
+        assertThat(nodeParameterDefinition.defaultValue, is(defaultValue));
+        assertThat(nodeParameterDefinition.getName(), is(name));
+        assertThat(nodeParameterDefinition.getDescription(), is(description));
+        assertThat(nodeParameterDefinition.getDefaultParameterValue().getLabel(), is(defaultValue));
+        assertThat(nodeParameterDefinition.getTriggerIfResult(), is(triggerIfResult));
+        assertThat(nodeParameterDefinition.getNodeEligibility(), is(instanceOf(IgnoreOfflineNodeEligibility.class)));
+        assertTrue(nodeParameterDefinition.isAllNodesMatchingLabel());
+    }
+
+    @Test
+    @Deprecated
+    public void testNodeParameterDefinitionDeprecated3Arg() {
+        String name = "name";
+        String description = "The description";
+        String defaultValue = "built-in || master";
+
+        LabelParameterDefinition nodeParameterDefinition =
+                new LabelParameterDefinition(name, description, defaultValue);
+
+        assertThat(nodeParameterDefinition.defaultValue, is(defaultValue));
+        assertThat(nodeParameterDefinition.getName(), is(name));
+        assertThat(nodeParameterDefinition.getDescription(), is(description));
+        assertThat(nodeParameterDefinition.getDefaultParameterValue().getLabel(), is(defaultValue));
+        assertThat(nodeParameterDefinition.getTriggerIfResult(), is("allCases"));
+        assertThat(nodeParameterDefinition.getNodeEligibility(), is(instanceOf(AllNodeEligibility.class)));
+        assertFalse(nodeParameterDefinition.isAllNodesMatchingLabel());
+    }
+
+    @Test
+    public void testNodeParameterDefinition() {
+        String name = "name";
+        String description = "The description";
+        String defaultValue = "built-in || master";
+        String triggerIfResult = "The triggerIfResult value";
+
+        LabelParameterDefinition nodeParameterDefinition =
+                new LabelParameterDefinition(name, description, defaultValue, false, null, triggerIfResult);
+
+        assertThat(nodeParameterDefinition.defaultValue, is(defaultValue));
+        assertThat(nodeParameterDefinition.getName(), is(name));
+        assertThat(nodeParameterDefinition.getDescription(), is(description));
+        assertThat(nodeParameterDefinition.getDefaultParameterValue().getLabel(), is(defaultValue));
+        assertThat(nodeParameterDefinition.getTriggerIfResult(), is(triggerIfResult));
+        assertThat(nodeParameterDefinition.getNodeEligibility(), is(instanceOf(AllNodeEligibility.class)));
+        assertFalse(nodeParameterDefinition.isAllNodesMatchingLabel());
+    }
+
+    @Test
+    public void testDoListNodesForAgentLabel() throws Exception {
+        LabelParameterDefinition.DescriptorImpl nodeParameterDefinition = new LabelParameterDefinition.DescriptorImpl();
+        assertThat(nodeParameterDefinition.getDefaultNodeEligibility(), is(instanceOf(AllNodeEligibility.class)));
+        FormValidation validation = nodeParameterDefinition.doListNodesForLabel(LABEL_NAME);
+        String msg = validation.getMessage();
+        assertThat(msg, allOf(containsString("Matching nodes"), containsString(agent.getNodeName())));
+    }
+
+    @Test
+    public void testDoListNodesForControllerLabel() throws Exception {
+        String controllerLabel = "built-in";
+        LabelParameterDefinition.DescriptorImpl nodeParameterDefinition = new LabelParameterDefinition.DescriptorImpl();
+        assertThat(nodeParameterDefinition.getDefaultNodeEligibility(), is(instanceOf(AllNodeEligibility.class)));
+        FormValidation validation = nodeParameterDefinition.doListNodesForLabel(controllerLabel);
+        String msg = validation.getMessage();
+        assertThat(msg, allOf(containsString("Matching nodes"), containsString(controllerLabel)));
+    }
+
+    @Test
+    public void testDoListNodesForNonExistentLabel() throws Exception {
+        String badLabel = "this-label-does-not-exist";
+        LabelParameterDefinition.DescriptorImpl nodeParameterDefinition = new LabelParameterDefinition.DescriptorImpl();
+        assertThat(nodeParameterDefinition.getDefaultNodeEligibility(), is(instanceOf(AllNodeEligibility.class)));
+        FormValidation validation = nodeParameterDefinition.doListNodesForLabel(badLabel);
+        String msg = validation.getMessage();
+        assertThat(
+                msg,
+                allOf(
+                        containsString("The label expression"),
+                        containsString(badLabel),
+                        containsString("does not match any node")));
+    }
+
+    @Test
+    public void testDoListNodesForBlankLabel() throws Exception {
+        String blankLabel = "";
+        LabelParameterDefinition.DescriptorImpl nodeParameterDefinition = new LabelParameterDefinition.DescriptorImpl();
+        FormValidation validation = nodeParameterDefinition.doListNodesForLabel(blankLabel);
+        String msg = validation.getMessage();
+        assertThat(msg, containsString("a label is required"));
+    }
+
+    @Test
+    public void testDoListNodesForInvalidLabelExpression() throws Exception {
+        String invalidLabel = "a||";
+        LabelParameterDefinition.DescriptorImpl nodeParameterDefinition = new LabelParameterDefinition.DescriptorImpl();
+        assertThat(nodeParameterDefinition.getDefaultNodeEligibility(), is(instanceOf(AllNodeEligibility.class)));
+        FormValidation validation = nodeParameterDefinition.doListNodesForLabel(invalidLabel);
+        String msg = validation.getMessage();
+        assertThat(
+                msg,
+                allOf(
+                        containsString("The label expression"),
+                        containsString(invalidLabel),
+                        containsString("is not valid")));
+    }
+}

src/test/java/org/jvnet/jenkins/plugins/nodelabelparameter/LabelParameterValueTest.java

@@ -23,12 +23,39 @@
  */
 package org.jvnet.jenkins.plugins.nodelabelparameter;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
+
+import hudson.slaves.DumbSlave;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Random;
 import nl.jqno.equalsverifier.EqualsVerifier;
 import nl.jqno.equalsverifier.Warning;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
 import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.AllNodeEligibility;
+import org.jvnet.jenkins.plugins.nodelabelparameter.node.NodeEligibility;
 
 public class LabelParameterValueTest {
 
+    @ClassRule
+    public static JenkinsRule j = new JenkinsRule();
+
+    private static DumbSlave agent;
+
+    @BeforeClass
+    public static void createAgent() throws Exception {
+        agent = j.createOnlineSlave();
+    }
+
+    private final Random random = new Random();
+
     public LabelParameterValueTest() {}
 
     @Test
@@ -39,4 +66,62 @@ public void testEqualsContract() {
                 .withIgnoredFields("description", "nextLabels")
                 .verify();
     }
+
+    @Test
+    @Deprecated
+    public void testLabelParameterValueDeprecated2ArgConstructor() {
+        String value = " my-label "; // Intentionally has leading and trailing spaces
+        String trimmedValue = value.trim();
+        // Use either value or trimmedValue randomly, does not change any assertion
+        LabelParameterValue labelParameterValue =
+                new LabelParameterValue("my-name", random.nextBoolean() ? value : trimmedValue);
+        assertThat(labelParameterValue.getName(), is("my-name"));
+        assertThat(labelParameterValue.getLabel(), is(trimmedValue));
+        assertThat(labelParameterValue.getDescription(), is(nullValue()));
+        assertThat(labelParameterValue.getNextLabels(), is(empty()));
+    }
+
+    @Test
+    @Deprecated
+    public void testLabelParameterValueDeprecated2ArgConstructorNullName() {
+        String value = " my-label "; // Intentionally has leading and trailing spaces
+        String trimmedValue = value.trim();
+        // Use either value or trimmedValue randomly, does not change any assertion
+        LabelParameterValue labelParameterValue =
+                new LabelParameterValue(null, random.nextBoolean() ? value : trimmedValue);
+        assertThat(labelParameterValue.getName(), is("NODELABEL"));
+        assertThat(labelParameterValue.getLabel(), is(trimmedValue));
+        assertThat(labelParameterValue.getDescription(), is(nullValue()));
+        assertThat(labelParameterValue.getNextLabels(), is(empty()));
+    }
+
+    @Test
+    @Deprecated
+    public void testLabelParameterValueDeprecated3ArgConstructor() {
+        String value = " my-label "; // Intentionally has leading and trailing spaces
+        String trimmedValue = value.trim();
+        String name = "my-name";
+        String description = "My description";
+        // Use either value or trimmedValue randomly, does not change any assertion
+        LabelParameterValue labelParameterValue =
+                new LabelParameterValue(name, description, random.nextBoolean() ? value : trimmedValue);
+        assertThat(labelParameterValue.getName(), is(name));
+        assertThat(labelParameterValue.getLabel(), is(trimmedValue));
+        assertThat(labelParameterValue.getDescription(), is(description));
+        assertThat(labelParameterValue.getNextLabels(), is(empty()));
+    }
+
+    @Test
+    public void testGetNextLabels() {
+        String name = "my-name";
+        List<String> extraNodeNames = Arrays.asList("built-in", "not-a-valid-node-name");
+        List<String> nodeNames = new ArrayList<>();
+        nodeNames.add(agent.getNodeName());
+        nodeNames.addAll(extraNodeNames);
+        NodeEligibility eligibility = new AllNodeEligibility();
+        LabelParameterValue labelParameterValue = new LabelParameterValue(name, nodeNames, eligibility);
+        assertThat(labelParameterValue.getName(), is(name));
+        assertThat(labelParameterValue.getLabel(), is(agent.getNodeName()));
+        assertThat(labelParameterValue.getNextLabels(), is(extraNodeNames));
+    }
 }