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.

Sign up for GitHub

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

Jump to bottom

Remove --add-opens for test workers #20999

Merged
merged 1 commit into from Jun 22, 2022
Merged

Remove --add-opens for test workers #20999

merged 1 commit into from Jun 22, 2022

Conversation

jvandort
Copy link
Member

@jvandort jvandort commented Jun 14, 2022 •
edited

Fixes: #19771

  • Update ClassLoaderUtils to use public JDK APIs when listing a ClassLoader's package
  • Disable adding any --add-opens flags to test worker processes
  • Add tests to verify production code and test code behavior is equivalent
  • Verify users can opt back into previous behavior by adding --add-opens flags manually to test jvm args
  • Rework TestNG version coverage to align it closer to how Groovy versions are selected during tests

TODO:

  • Add Release Notes Entry
  • Add Upgrade Guide Entry

@jvandort jvandort added this to the 7.6 RC1 milestone Jun 14, 2022
@jvandort jvandort self-assigned this Jun 14, 2022
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch 2 times, most recently from 1d880b2 to 1501de2 Compare June 14, 2022 04:59
@gradle gradle deleted a comment from jvandort Jun 14, 2022
@gradle gradle deleted a comment from jvandort Jun 14, 2022
@gradle gradle deleted a comment from jvandort Jun 14, 2022
big-guy
big-guy requested changes Jun 14, 2022
View reviewed changes
Copy link
Member

@big-guy big-guy left a comment

Choose a reason for hiding this comment

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

I think we should add a more general note about upgrading to the upgrade guide that's not specific to TestNG.

IIUC, some projects may have code that was relying on these --add-opens to make their tests pass. We should provide some guidance about how to deal with that.

subprojects/base-services/src/main/java/org/gradle/internal/classloader/ClassLoaderUtils.java Show resolved Hide resolved
...Test/groovy/org/gradle/workers/internal/JdkIllegalReflectionTestWorkerIntegrationTest.groovy Outdated Show resolved Hide resolved
...Test/groovy/org/gradle/workers/internal/JdkIllegalReflectionTestWorkerIntegrationTest.groovy Outdated Show resolved Hide resolved
...Test/groovy/org/gradle/workers/internal/JdkIllegalReflectionTestWorkerIntegrationTest.groovy Outdated Show resolved Hide resolved
...Test/groovy/org/gradle/workers/internal/JdkIllegalReflectionTestWorkerIntegrationTest.groovy Outdated Show resolved Hide resolved
subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/fixture/TestNGCoverage.groovy Outdated Show resolved Hide resolved
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch 3 times, most recently from 89cefd1 to 61b16fc Compare June 15, 2022 05:24
@gradle gradle deleted a comment from jvandort Jun 15, 2022
@jvandort jvandort modified the milestones: 7.6 RC1, 7.5.1 Jun 15, 2022
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch from 61b16fc to f1ab865 Compare June 16, 2022 19:38
@gradle gradle deleted a comment from jvandort Jun 16, 2022
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch from f1ab865 to be65210 Compare June 16, 2022 19:50
@gradle gradle deleted a comment from jvandort Jun 16, 2022
@jvandort jvandort modified the milestones: 7.5.1, 7.5 RC3 Jun 16, 2022
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch from be65210 to 342474d Compare June 16, 2022 20:17
@gradle gradle deleted a comment from jvandort Jun 16, 2022
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch from 342474d to f31cc1e Compare June 16, 2022 21:28
@jvandort jvandort changed the base branch from master to release June 16, 2022 21:28
@gradle gradle deleted a comment from jvandort Jun 16, 2022
@gradle gradle deleted a comment from jvandort Jun 16, 2022
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch from f31cc1e to 5bf400d Compare June 16, 2022 22:47
@gradle gradle deleted a comment from jvandort Jun 21, 2022
@gradle gradle deleted a comment from jvandort Jun 21, 2022
@gradle gradle deleted a comment from bot-gradle Jun 21, 2022
@jvandort jvandort marked this pull request as ready for review June 21, 2022 16:20
@jvandort jvandort requested review from big-guy and tresat June 21, 2022 16:20
big-guy
big-guy requested changes Jun 21, 2022
View reviewed changes
Copy link
Member

@big-guy big-guy left a comment

Choose a reason for hiding this comment

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

Mostly some language suggestions and a test fix

...plugin-development/src/main/java/org/gradle/plugin/devel/plugins/JavaGradlePluginPlugin.java Outdated Show resolved Hide resolved
.../integTest/groovy/org/gradle/testing/testng/TestNGLoggingOutputCaptureIntegrationTest.groovy Outdated Show resolved Hide resolved
subprojects/docs/src/docs/release/notes.md Outdated Show resolved Hide resolved
subprojects/docs/src/docs/release/notes.md Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
subprojects/docs/src/docs/userguide/migration/upgrading_version_7.adoc Outdated Show resolved Hide resolved
@jvandort jvandort requested a review from big-guy June 21, 2022 21:25
@jvandort
Remove --add-opens for test workers
4b1028e 
* Update ClassLoaderUtils to use public JDK APIs when listing a ClassLoader's package
* Disable adding any --add-opens flags to test worker processes
* Add tests to verify production code and test code behavior is equivilent
* Verify users can opt back into previous behavior by adding --add-opens flags manually to test jvm args
* Improved multi-jdk support for TestNG Tests
* Add release notes and upgrade guide

Addressed action items
@jvandort jvandort force-pushed the jvandort/remove-add-opens-for-test-workers branch 2 times, most recently from f8b07a7 to 4b1028e Compare June 22, 2022 00:55
@gradle gradle deleted a comment from jvandort Jun 22, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle
Copy link
Collaborator

Pre-tested commit build failed.

@blindpirate
Copy link
Collaborator

@jvandort I'm fixing this.

@bot-gradle test and merge

@gradle gradle deleted a comment from jvandort Jun 22, 2022
@gradle gradle deleted a comment from jvandort Jun 22, 2022
@bot-gradle
Copy link
Collaborator

Your PR is queued. See the queue page for details.

@bot-gradle
Copy link
Collaborator

OK, I've already triggered a build for you.

@bot-gradle bot-gradle merged commit 678f907 into release Jun 22, 2022
@blindpirate blindpirate deleted the jvandort/remove-add-opens-for-test-workers branch June 22, 2022 07:05
leonard84 added a commit to spockframework/spock that referenced this pull request Aug 31, 2022
@leonard84
Fix test failures with Java 17 + Gradle 7.5.1
8ede7c3 
Prior to version 7.5, Gradle added some `--add-opens` flags,
that hid potential issues with tests, for Java 16+.

gradle/gradle#20999
@vlsi vlsi mentioned this pull request Jan 10, 2023
Merged
leonard84 added a commit to leonard84/spock that referenced this pull request Jan 26, 2023
@leonard84
Fix test failures with Java 17 + Gradle 7.6
12867f4 
Prior to version 7.5, Gradle added some `--add-opens` flags,
that hid potential issues with tests, for Java 16+.

gradle/gradle#20999
leonard84 added a commit to leonard84/spock that referenced this pull request Feb 16, 2023
@leonard84
Fix test failures with Java 17 + Gradle 7.6
2cb065e 
Prior to version 7.5, Gradle added some `--add-opens` flags,
that hid potential issues with tests, for Java 16+.

gradle/gradle#20999
leonard84 added a commit to leonard84/spock that referenced this pull request Feb 16, 2023
@leonard84
Fix test failures with Java 17 + Gradle 7.6
1847d75 
Prior to version 7.5, Gradle added some `--add-opens` flags,
that hid potential issues with tests, for Java 16+.

gradle/gradle#20999
leonard84 added a commit to leonard84/spock that referenced this pull request Feb 16, 2023
@leonard84
Fix test failures with Java 17 + Gradle 7.6
4d7be3a 
Prior to version 7.5, Gradle added some `--add-opens` flags,
that hid potential issues with tests, for Java 16+.

gradle/gradle#20999
leonard84 added a commit to spockframework/spock that referenced this pull request Feb 16, 2023
@leonard84
Fix test failures with Java 17 + Gradle 7.6
10c0675 
Prior to version 7.5, Gradle added some `--add-opens` flags,
that hid potential issues with tests, for Java 16+.

gradle/gradle#20999
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@big-guy big-guy big-guy approved these changes

@ljacomet ljacomet Awaiting requested review from ljacomet

@tresat tresat Awaiting requested review from tresat

Assignees

@jvandort jvandort

Labels
None yet
Projects
None yet
Milestone
7.5 RC3
Development

Successfully merging this pull request may close these issues.

Gradle worker should not open any packages during tests
5 participants
@jvandort @bot-gradle @blindpirate @tresat @big-guy