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

JENKINS-34005 - update tests for PipelineTriggersJobProperty #20

Merged
merged 8 commits into from Jul 29, 2016
Expand Up @@ -25,14 +25,22 @@
package org.jenkinsci.plugins.workflow.multibranch;

import hudson.model.BooleanParameterDefinition;
import hudson.model.Item;
import hudson.model.JobProperty;
import hudson.model.ParametersAction;
import hudson.model.ParametersDefinitionProperty;
import hudson.model.StringParameterValue;
import hudson.model.queue.QueueTaskFuture;
import hudson.tasks.LogRotator;

import java.io.ObjectStreamException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

import hudson.triggers.TimerTrigger;
import hudson.triggers.Trigger;
import hudson.triggers.TriggerDescriptor;
import jenkins.branch.BranchProperty;
import jenkins.branch.BranchSource;
import jenkins.branch.DefaultBranchPropertyStrategy;
Expand All @@ -44,6 +52,7 @@
import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.jenkinsci.plugins.workflow.job.WorkflowRun;
import org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty;
import org.jenkinsci.plugins.workflow.steps.StepConfigTester;
import static org.junit.Assert.*;

Expand All @@ -54,6 +63,9 @@
import org.jvnet.hudson.test.BuildWatcher;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.TestExtension;
import org.kohsuke.stapler.DataBoundConstructor;

import static org.jenkinsci.plugins.workflow.multibranch.WorkflowMultiBranchProjectTest.scheduleAndFindBranchProject;

@Issue("JENKINS-30519")
Expand All @@ -66,11 +78,12 @@ public class JobPropertyStepTest {
@SuppressWarnings("rawtypes")
@Test public void configRoundTripParameters() throws Exception {
StepConfigTester tester = new StepConfigTester(r);
assertEquals(Collections.emptyList(), tester.configRoundTrip(new JobPropertyStep(Collections.<JobProperty>emptyList())).getProperties());
// PipelineTriggersJobProperty is now always going to be present, even if empty.
assertEquals(1, tester.configRoundTrip(new JobPropertyStep(Collections.<JobProperty>emptyList())).getProperties().size());
List<JobProperty> properties = tester.configRoundTrip(new JobPropertyStep(Collections.<JobProperty>singletonList(new ParametersDefinitionProperty(new BooleanParameterDefinition("flag", true, null))))).getProperties();
assertEquals(1, properties.size());
assertEquals(ParametersDefinitionProperty.class, properties.get(0).getClass());
ParametersDefinitionProperty pdp = (ParametersDefinitionProperty) properties.get(0);
assertEquals(2, properties.size());
ParametersDefinitionProperty pdp = getPropertyFromList(ParametersDefinitionProperty.class, properties);
assertNotNull(pdp);
assertEquals(1, pdp.getParameterDefinitions().size());
assertEquals(BooleanParameterDefinition.class, pdp.getParameterDefinitions().get(0).getClass());
BooleanParameterDefinition bpd = (BooleanParameterDefinition) pdp.getParameterDefinitions().get(0);
Expand All @@ -82,11 +95,12 @@ public class JobPropertyStepTest {
@SuppressWarnings("rawtypes")
@Test public void configRoundTripBuildDiscarder() throws Exception {
StepConfigTester tester = new StepConfigTester(r);
assertEquals(Collections.emptyList(), tester.configRoundTrip(new JobPropertyStep(Collections.<JobProperty>emptyList())).getProperties());
// PipelineTriggersJobProperty is now always going to be present, even if empty.
assertEquals(1, tester.configRoundTrip(new JobPropertyStep(Collections.<JobProperty>emptyList())).getProperties().size());
List<JobProperty> properties = tester.configRoundTrip(new JobPropertyStep(Collections.<JobProperty>singletonList(new BuildDiscarderProperty(new LogRotator(1, 2, -1, 3))))).getProperties();
assertEquals(1, properties.size());
assertEquals(BuildDiscarderProperty.class, properties.get(0).getClass());
BuildDiscarderProperty bdp = (BuildDiscarderProperty) properties.get(0);
assertEquals(2, properties.size());
BuildDiscarderProperty bdp = getPropertyFromList(BuildDiscarderProperty.class, properties);
assertNotNull(bdp);
BuildDiscarder strategy = bdp.getStrategy();
assertNotNull(strategy);
assertEquals(LogRotator.class, strategy.getClass());
Expand Down Expand Up @@ -185,4 +199,109 @@ public class JobPropertyStepTest {
r.assertBuildStatusSuccess(r.waitForCompletion(b4));
}

@Issue("JENKINS-34005")
@Test public void triggersProperty() throws Exception {
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
// Verify the base case behavior.
p.setDefinition(new CpsFlowDefinition("echo 'foo'"));
Copy link
Member

Choose a reason for hiding this comment

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

Actually just "" suffices.


assertTrue(p.getTriggers().isEmpty());

r.assertBuildStatusSuccess(p.scheduleBuild2(0));

// Make sure the triggers are still empty.
assertTrue(p.getTriggers().isEmpty());

// Now add a trigger.
p.setDefinition(new CpsFlowDefinition("properties([[$class: 'PipelineTriggersJobProperty',\n"
Copy link
Member

Choose a reason for hiding this comment

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

Since there is a @Symbol, I guess this will become pipelineTriggers(…) soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

...once we've gotten JENKINS-29922 fully landed, yeah - I think we're still going to want the properties step containing pipelineTriggers(...) and friends, though, since we wipe out any pre-existing properties currently...

Copy link
Member

Choose a reason for hiding this comment

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

🐜 This dependency has been released, so please update to it and use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that TimerTrigger does not have a @Symbol on it, so that's gonna have to still be the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

And at least for this moment, I'm not going to worry about having a @Symbol on MockTrigger either.

Copy link
Member

Choose a reason for hiding this comment

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

TimerTrigger does not have a @symbol on it

It does in Jenkins 2.2+. So use the switch idiom used in other test cases in this suite, and try running with

mvn -Djenkins.version=2.7.1 -Dtest=JobPropertyStepTest surefire:test

+ " triggers: [[$class: 'TimerTrigger', spec: '@daily'],\n"
Copy link
Member

Choose a reason for hiding this comment

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

Should I think wind up looking like

properties([
  pipelineTriggers([
    cron('@daily'),
    mock()
  ])
])

+ " [$class: 'MockTrigger']]]])\n"
+ "echo 'foo'"));

r.assertBuildStatusSuccess(p.scheduleBuild2(0));

assertEquals(2, p.getTriggers().size());

Copy link
Member

Choose a reason for hiding this comment

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

🐛 you are not verifying that start was called with the correct argument.

calling startTriggers() in PipelineTriggersJobProperty.setOwner causes...chaos. Way too many calls to startTriggers()

Yet this may be the only way you get that call done properly. Why would there be multiple calls to setOwner?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I have unraveled that mess. Will dig into the start argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I may not have. Will touch back on this tomorrow!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm - how do I check the start argument? Guess I need to find a Trigger that actually cares about the newInstance flag - TimerTrigger doesn't...

Copy link
Member

Choose a reason for hiding this comment

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

The upstream PR had such a Trigger, no?

PipelineTriggersJobProperty triggerProp = p.getTriggersJobProperty();

TimerTrigger timerTrigger = getTriggerFromList(TimerTrigger.class, triggerProp.getTriggers());

assertNotNull(timerTrigger);

assertEquals("@daily", timerTrigger.getSpec());

MockTrigger mockTrigger = getTriggerFromList(MockTrigger.class, triggerProp.getTriggers());

assertNotNull(mockTrigger);

assertTrue(mockTrigger.isStarted);

assertEquals("[false]", mockTrigger.calls.toString());

// Now run a properties step with a different property and verify that we still have a
// PipelineTriggersJobProperty, but with no triggers in it.
p.setDefinition(new CpsFlowDefinition("properties([[$class: 'DisableConcurrentBuildsJobProperty']])\n"
Copy link
Member

Choose a reason for hiding this comment

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

🐜 ditto, this has a symbol we can use now too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup yup.

+ "echo 'foo'"));

r.assertBuildStatusSuccess(p.scheduleBuild2(0));

assertNotNull(p.getTriggersJobProperty());

assertTrue(p.getTriggers().isEmpty());

}

private <T extends Trigger> T getTriggerFromList(Class<T> clazz, List<Trigger<?>> triggers) {
for (Trigger t : triggers) {
if (clazz.isInstance(t)) {
return clazz.cast(t);
}
}

return null;
}

private <T extends JobProperty> T getPropertyFromList(Class<T> clazz, List<JobProperty> properties) {
for (JobProperty p : properties) {
if (clazz.isInstance(p)) {
return clazz.cast(p);
}
}

return null;
}

public static class MockTrigger extends Trigger<Item> {

public transient List<Boolean> calls = new ArrayList<Boolean>();
public transient boolean isStarted = false;

@DataBoundConstructor
public MockTrigger() {}

@Override public void start(Item project, boolean newInstance) {
super.start(project, newInstance);
calls.add(newInstance);
isStarted = true;
}

@Override public void stop() {
super.stop();
isStarted = false;
}

@Override protected Object readResolve() throws ObjectStreamException {
calls = new ArrayList<Boolean>();
return super.readResolve();
}

@TestExtension
public static class DescriptorImpl extends TriggerDescriptor {

@Override public boolean isApplicable(Item item) {
return true;
}

}
}
}