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-64846] Fix use of local declared variables inside matrix #415

Merged
merged 3 commits into from
Feb 12, 2021

Conversation

bitwiseman
Copy link
Contributor

@bitwiseman bitwiseman commented Feb 11, 2021

  • JENKINS issue(s):

  • Description:

    • We have script splitting turned on by default in most tests. The assumption as was that script splitting being turned on would break things, not make them work unexpectedly. Turns out that is not always the case. With the change to script splitting from 1.8.0, if script splitting is turned off local declared variables do not get retained appropriately in matrix.
  • Documentation changes:

    • Link to related jenkins.io PR or explanation for why doc change not needed
  • Users/aliases to notify:

We have script splitting turned on by default in most tests.  The assumption as was that
script splitting being turned one would break thing not make them work unexpectedly.

Turns out that is not always the case.
@bitwiseman
Copy link
Contributor Author

bitwiseman commented Feb 11, 2021

I'm reviewing existing tests to make sure this scenario is covered outside of matrix. It looks like it was not but also is not failing aside from matrix.

Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

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

Looks reasonable. I do wonder if we might want to make a profile in the POM for turning script splitting on and run the full suite of tests with and without script splitting on in CI?

@bitwiseman
Copy link
Contributor Author

@abayer
Sounds like a good idea. I plan to make script splitting the default behavior at some point, but until then we should cover both behaviors.

Copy link
Contributor

@car-roll car-roll left a comment

Choose a reason for hiding this comment

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

LGTM, though admittedly I'm just looking more for syntax errors as I'm not completely familiar with the code

@bitwiseman bitwiseman merged commit cea7cf3 into jenkinsci:master Feb 12, 2021
@bitwiseman bitwiseman deleted the JENKINS-64846 branch February 12, 2021 17:39
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<systemPropertyVariables>
<org.jenkinsci.plugins.pipeline.modeldefinition.parser.RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION>false</org.jenkinsci.plugins.pipeline.modeldefinition.parser.RuntimeASTTransformer.SCRIPT_SPLITTING_TRANSFORMATION>
Copy link
Member

Choose a reason for hiding this comment

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

I do not get it. Why not just set these flags directly in the code, at runtime, depending on the Java version? Or if you want to test in both modes, use @Parameterized etc. See jenkinsci/bom#451 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants