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

Conversation

Projects
None yet
3 participants
@abayer
Copy link
Member

commented Jul 14, 2016

JENKINS-34005

Depends on jenkinsci/workflow-job-plugin#9

  • Updates existing tests to deal with the fact that there should always be a PipelineTriggersJobProperty on a WorkflowJob, even if it has no triggers in it.
  • Added a new test specifically for PipelineTriggersJobProperty.

cc @reviewbybees esp @jglick

@reviewbybees

This comment has been minimized.

Copy link

commented Jul 14, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2016

Oh, and I deployed a SNAPSHOT of jenkinsci/workflow-job-plugin#9 so these tests should work.

@abayer abayer referenced this pull request Jul 14, 2016

Merged

JENKINS-34005 - Convert WorkflowJob.triggers to a JobProperty #9

6 of 6 tasks complete
r.assertBuildStatusSuccess(p.scheduleBuild2(0));

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

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

🐛 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?

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

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

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

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

This comment has been minimized.

Copy link
@abayer

abayer Jul 26, 2016

Author Member

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...

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

The upstream PR had such a Trigger, no?

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2016

Retriggering build - last run used a stale snapshot of workflow-job.

@abayer abayer closed this Jul 26, 2016

@abayer abayer reopened this Jul 26, 2016

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

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

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

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

This comment has been minimized.

Copy link
@abayer

abayer Jul 27, 2016

Author Member

...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...

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

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

This comment has been minimized.

Copy link
@abayer

abayer Jul 28, 2016

Author Member

Will do.

This comment has been minimized.

Copy link
@abayer

abayer Jul 28, 2016

Author Member

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

This comment has been minimized.

Copy link
@abayer

abayer Jul 28, 2016

Author Member

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

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

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
Merge branch 'master' into jenkins-34005
Conflicts:
	src/test/java/org/jenkinsci/plugins/workflow/multibranch/JobPropertyStepTest.java

// 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"

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

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

This comment has been minimized.

Copy link
@abayer

abayer Jul 28, 2016

Author Member

Yup yup.

pom.xml Outdated
@@ -186,6 +186,13 @@ THE SOFTWARE.
<scope>test</scope>
</dependency>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>workflow-job</artifactId>
<version>2.4-SNAPSHOT</version>

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

You can make this 2.4 now.


// Now add a trigger.
p.setDefinition(new CpsFlowDefinition("properties([[$class: 'PipelineTriggersJobProperty',\n"
+ " triggers: [[$class: 'TimerTrigger', spec: '@daily'],\n"

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

Should I think wind up looking like

properties([
  pipelineTriggers([
    cron('@daily'),
    mock()
  ])
])
LogRotator.LRDescriptor.class.isAnnotationPresent(Symbol.class); // "logRotator"
LogRotator.LRDescriptor.class.isAnnotationPresent(Symbol.class) && // "logRotator"
TimerTrigger.DescriptorImpl.class.isAnnotationPresent(Symbol.class) && // "disableConcurrentBuilds"
PipelineTriggersJobProperty.DescriptorImpl.class.isAnnotationPresent(Symbol.class); // "pipelineTriggers"

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

🐛 this last clause is unconditionally true, since we declare that dependency. This constant is about symbols present in core.

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

Merge against master to pick up #24.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2016

Merged.

EDIT: Forgot to remove that last clause - one more time...

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

This comment has been minimized.

Copy link
@jglick

jglick Jul 28, 2016

Member

Actually just "" suffices.

@jglick

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

🐝

@jglick jglick merged commit 9699373 into jenkinsci:master Jul 29, 2016

1 check was pending

Jenkins Jenkins is validating pull request ...
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.