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

Accessing test options no longer warns, but locks in test framework selection #21750

Closed
wants to merge 19 commits into from
Closed

Accessing test options no longer warns, but locks in test framework selection #21750

wants to merge 19 commits into from

Conversation

tresat
Copy link
Member

@tresat tresat commented Aug 30, 2022 •
edited

Fixes #21747

This undoes #19058 for Gradle 8.0.

@tresat tresat added this to the 8.0 RC1 milestone Aug 30, 2022
@tresat tresat self-assigned this Aug 30, 2022
@tresat tresat requested a review from a team August 31, 2022 14:55
@tresat tresat marked this pull request as ready for review August 31, 2022 14:55
@tresat tresat requested a review from a team as a code owner August 31, 2022 14:55
@tresat tresat removed the request for review from a team August 31, 2022 14:55
@gradle gradle deleted a comment from tresat Aug 31, 2022
@jvandort
Copy link
Member

jvandort commented Sep 6, 2022

This comment here seems related:
https://github.com/gradle/gradle/blob/master/subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java#L152-L155

@tresat
Copy link
Member Author

tresat commented Sep 12, 2022

This comment here seems related: https://github.com/gradle/gradle/blob/master/subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java#L152-L155

Nice catch. Yes, I'll remove that map for a (very minor) optimization.

@tresat
Update error message format when attempting to improperly set test fr…
45363ed 
…amework after options are already set

- Also provide each test framework with a display name property for user-facing references.
@tresat
Optimize framework creation now that reseting framework after options…
1f54a04 
… set is blocked elsewhere
@gradle gradle deleted a comment from tresat Sep 12, 2022
@gradle gradle deleted a comment from tresat Sep 12, 2022
@tresat
Restore map lookup functionality which is needed
d21e077 
- Add comment explaining why.
- Add tests to ensure behavior remains correct.
@tresat
Remove unused field
2ec207e
@gradle gradle deleted a comment from tresat Sep 13, 2022
@gradle gradle deleted a comment from tresat Sep 13, 2022
@tresat
Expose a property from the Test task to check if options are already …
3be7020 
…accessed; check this when changing framework in a suite

- We need to check not just when changing the test framework within a test task, but when changing the test framework for a suite
if the associated test tasks for that suite have had options accessed.  Otherwise, the test task will silently continue to use the options
and framework first configured without switching to the new ones.
- Create new test class to demonstrate misconfigurations of options.
@gradle gradle deleted a comment from tresat Sep 13, 2022
@gradle gradle deleted a comment from tresat Sep 14, 2022
@tresat
Copy link
Member Author

tresat commented Sep 14, 2022

I think I’ve now covered all the cases where you could cause issues by changing the test framework after setting options. This ended up being a little more complex than we had anticipated.

jvandort
jvandort approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@jvandort jvandort left a comment

Choose a reason for hiding this comment

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

Good call on adding all the new tests. Mostly just nitpicks here and there but LGTM

...ojects/testing-jvm/src/integTest/groovy/org/gradle/testing/TestOptionsIntegrationSpec.groovy Outdated Show resolved Hide resolved
...ojects/testing-jvm/src/integTest/groovy/org/gradle/testing/TestOptionsIntegrationSpec.groovy Outdated Show resolved Hide resolved
...ojects/testing-jvm/src/integTest/groovy/org/gradle/testing/TestOptionsIntegrationSpec.groovy Outdated Show resolved Hide resolved
...grationSpec/reExecutesWhenPropertyIsChanged/src/integTest/java/org/gradle/SomeTestClass.java Show resolved Hide resolved
...ting-jvm/src/integTest/groovy/org/gradle/testing/testsuites/TestSuitesIntegrationTest.groovy Outdated Show resolved Hide resolved
...ting-jvm/src/integTest/groovy/org/gradle/testing/junit/JUnitCategoriesIntegrationSpec.groovy Outdated Show resolved Hide resolved
subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java Show resolved Hide resolved
subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java Outdated Show resolved Hide resolved
@tresat
Clarify tests
2624e9b 
- Remove margin bar in multiline strings.
- Make naming of migrated method consistent.
- Remove extraneous comments.
@tresat
Update copyright dates in new resource files
eb633d7
@tresat
Also test excluding categories
9441e5e
@tresat
Consolidate addditional options related tests, removing a redundant test
467f607
@tresat
Remove unused imports
aa64e52
@tresat
Print name of test framework the user is attempting to set in test su…
8360456 
…ite in error message if attempt fails due to test options already being accessed
@gradle gradle deleted a comment from tresat Sep 14, 2022
@tresat
Copy link
Member Author

tresat commented Sep 14, 2022

@bot-gradle test this

@gradle gradle deleted a comment from tresat Sep 14, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered the following builds for you:

@marcphilipp marcphilipp removed their request for review September 16, 2022 09:39
@tresat
Merge branch 'master' into tt/80/deprecations/use-test-framework
d501ea3 
# Conflicts:
#	subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java
#	subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/TestFramework.java
#	subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/junit/JUnitTestFramework.java
#	subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/junitplatform/JUnitPlatformTestFramework.java
#	subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestFramework.java
@tresat
Copy link
Member Author

tresat commented Sep 16, 2022

@bot-gradle test this

@gradle gradle deleted a comment from tresat Sep 16, 2022
@bot-gradle
Copy link
Collaborator

OK, I've already triggered the following builds for you:

@tresat tresat closed this Oct 11, 2022
@tresat tresat deleted the tt/80/deprecations/use-test-framework branch September 18, 2023 21:51
@ov7a ov7a removed this from the 8.0 RC1 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcphilipp marcphilipp marcphilipp left review comments

@jvandort jvandort jvandort approved these changes

@big-guy big-guy Awaiting requested review from big-guy

Assignees

@tresat tresat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Test.useTestFramework to throw error upon improper usage
6 participants
@tresat @jvandort @bot-gradle @marcphilipp @big-guy @ov7a