-
Notifications
You must be signed in to change notification settings - Fork 257
Test needs to assume script splitting is enabled #417
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
Conversation
05008eb to
30d08ae
Compare
30d08ae to
051b530
Compare
jglick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine as a hotfix if it works. I suspect the basic problem is the abuse of POM profiles to control test execution. (To start with, recall that <activation> is ignored if you explicitly pass a list of profiles on the CLI!) Basically anything “creative” in a POM is a warning sign.
If this script splitting, whatever it is, behaves differently on Java 8 vs. 11, then better to have the test code itself decide whether to run or not. Or if you have some sort of feature flag, then use FlagRule to verify behavior with nondefault settings (which ensures that the flag is reset when the test finishes), and as needed use @Parameterized or other POJO idioms to run analogous tests with the flag both on and off.
| */ | ||
| @SuppressFBWarnings(value="SE_NO_SERIALVERSIONID") | ||
| class RuntimeASTTransformer { | ||
| public class RuntimeASTTransformer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guessing you do not actually want this to be public (except for test code?) so you can change it at will?
| public class RuntimeASTTransformer { | |
| @Restricted(NoExternalUse.class) // for test use only | |
| public class RuntimeASTTransformer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtnord
Actually, no. I considered that, but Groovy that is loaded and run in pipeline has difficulty with attributes. Making this public is not great, but not horrible.
jtnord
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it works 🤷
|
Follow on to e1ef424 . We can't sent environment variables in This skips this test if script splitting is not enabled - it requires it to work. |
This test failure made it past CI because the project uses the Java version to decide whether to enable script splitting or not. When run outside of CI, that choice is apparently not preserved.