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

Migrate snippet generator tests from workflow-cps #27

Merged
merged 6 commits into from Jul 16, 2019

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Jul 12, 2019

It looks like tests pass and the snippet generator works correctly even without this override, so I'm not sure what it's for. If it turns out it is necessary, I'd like to add a test and/comment explaining what it is here for.

EDIT: In the end, just migrating some tests from workflow-cps, see jenkinsci/workflow-cps-plugin#304.

@dwnusbaum dwnusbaum requested a review from jglick July 12, 2019 17:41
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Create a freestyle project parameterized with a boolean parameter named x. Create a Pipeline job. In the Pipeline Syntax page, select build and type in parameterized in the Project to Build field. A checkbox labeled x will appear, and Generate Pipeline Script will produce

build job: 'parameterized', parameters: [booleanParam(name: 'x', value: false)]

With the newInstance override removed, the generated script is

build 'parameterized'

@jglick
Copy link
Member

jglick commented Jul 12, 2019

I think https://github.com/jenkinsci/workflow-cps-plugin/blob/609bc51d4cca65e8c851cdbc05a3f0efd8d60638/src/test/java/org/jenkinsci/plugins/workflow/cps/SnippetizerTest.java#L203-L238 are the relevant tests. Possibly I forgot to move them from jenkinsci/pipeline-plugin#69 + jenkinsci/pipeline-plugin#190 during the 2.x split. workflow-cps generates a test JAR so SnippetizerTester can be used from anywhere.

@dwnusbaum
Copy link
Member Author

dwnusbaum commented Jul 12, 2019

Now I'm extra confused, because SnippitizerTest.buildTriggerStep seems to pass with these changes (but using the UI directly shows the problem you described). Maybe my SNAPSHOT versions are messed up locally. EDIT: Seems to work fine even moving the test here, maybe a subtle issue with SnippetizerTester.assertRoundTrip?

@dwnusbaum
Copy link
Member Author

I think https://github.com/jenkinsci/workflow-cps-plugin/blob/609bc51d4cca65e8c851cdbc05a3f0efd8d60638/src/test/java/org/jenkinsci/plugins/workflow/cps/SnippetizerTest.java#L203-L238 are the relevant tests

Ah, whoops, I missed those, was only looking at SnippetizerTest.buildTriggerStep, and they definitely catch the issue. I'll move them all here.

@dwnusbaum dwnusbaum changed the title Remove override of StepDescriptor.newInstance Migrate snippet generator tests from workflow-cps Jul 15, 2019
@dwnusbaum dwnusbaum merged commit fddc138 into jenkinsci:master Jul 16, 2019
@dwnusbaum dwnusbaum deleted the simplify-newInstance branch July 16, 2019 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants