Restore Previous Test Options Behavior #19080

tresat · 2021-11-23T00:10:38Z

Fixes #19058

Context

See review comments for more details on what this addresses.

… accessed prior to test framework selection.

…s force test re-running.

…from a TestSuite even when that property is queried multiple times.

… and expect new deprecation warning to fix broken tests.

… in suites consistently.

tresat · 2021-11-23T00:13:33Z

...-testing/src/main/groovy/org/gradle/integtests/fixtures/executer/AbstractGradleExecuter.java

@@ -1380,6 +1381,15 @@ private void validate(String output, String displayName) {
                }
            }

+            private boolean removeFirstExpectedDeprecationWarning(final Predicate<String> condition) {


I don't think there was/is a reason for the remove all behavior - I think it just wasn't a problem since there was only 1 deprecation of any given type tested for at once. This change supports duplicates, where the same deprecation warning text appears multiple times within a single build. This is needed now with the new Options configuration warning added in this PR.

tresat · 2021-11-23T00:15:14Z

...ting-jvm/src/integTest/groovy/org/gradle/testing/junit/JUnitCategoriesIntegrationSpec.groovy

@@ -122,6 +122,7 @@ public class MyTest {
        |testing {
        |   suites {
        |       test {
+        |           useJUnit()


The changes to this test are to produce a much cleaner test buildScript, the previous one, captured from a breakpoint for each run in this test, had some misleading and irrelevant duplicated content.

tresat · 2021-11-23T00:15:34Z

...jvm/src/integTest/groovy/org/gradle/testing/testng/TestNGUpToDateCheckIntegrationTest.groovy

@@ -45,7 +45,26 @@ class TestNGUpToDateCheckIntegrationTest extends AbstractIntegrationSpec {
    @Issue('https://github.com/gradle/gradle/issues/4924')
    def 'test task is up-to-date when #property is changed because it should not impact output'() {
        given:
-        TestNGCoverage.enableTestNG(buildFile)
+        buildScript """


More extraneous info in buildScript cleanup here.

tresat · 2021-11-23T00:16:10Z

...ting-jvm/src/integTest/groovy/org/gradle/testing/testsuites/TestSuitesIntegrationTest.groovy

@@ -472,8 +473,11 @@ class TestSuitesIntegrationTest extends AbstractIntegrationSpec {
            }
        """

+        executer.expectDocumentedDeprecationWarning("Accessing test options prior to setting test framework has been deprecated. This is scheduled to be removed in Gradle 8.0.")


This is expected twice.

What is the benefit of having twice the same deprecation warning printed?

tresat · 2021-11-23T00:17:26Z

...ting-jvm/src/integTest/groovy/org/gradle/testing/testsuites/TestSuitesIntegrationTest.groovy

@@ -592,4 +596,78 @@ class TestSuitesIntegrationTest extends AbstractIntegrationSpec {
        expect:
        succeeds("mytest", "assertNoTestClasses")
    }
+
+    def "multiple getTestingFramework() calls on a test suite return same instance"() {


We could possibly delete these tests, but I like that we have something that will make the failure obvious if we ever revert by accident to the old behavior of new testingFramework instance per call.

tresat · 2021-11-23T00:18:42Z

...oriesIntegrationSpec/reExecutesWhenPropertyIsChanged/src/test/java/org/gradle/CategoryB.java

+
+package org.gradle;
+
+public interface CategoryB {


Testing from to Category A should work just as well as testing from Category A to Category B; but this would have sped the debugging a little so I think it's worth keeping a second category here.

tresat · 2021-11-23T00:20:34Z

subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java

@@ -944,13 +949,17 @@ public TestFramework testFramework(@Nullable Closure testFrameworkConfigure) {
     *
     * @return The test framework options.
     */
-    @Internal


This was an unnoticed regression - this needs to remain @Nested here. For JUnit 4 alone - changing the Categorys included or excluded should force the Test task to rerun and NOT result in an UP-TO-DATE. This was only passing by accident.

…il everywhere without these assumptions.

…uccess/failure locally here, as on CI.

tresat · 2021-11-23T00:57:25Z

@bot-gradle test this

bot-gradle · 2021-11-23T00:57:56Z

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

tresat · 2021-11-23T01:00:01Z

subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/TestTaskIntegrationTest.groovy

@@ -282,6 +282,152 @@ class TestTaskIntegrationTest extends JUnitMultiVersionIntegrationSpec {
        extraArgs << [[], ["--tests", "MyTest"]]
    }

+    def "options set prior to setting same test framework will warn and have no effect"() {
+        ignoreWhenJUnitPlatform()


I missed the need to run this class using the integMultiVersionTest locally until it failed on CI. Need to ensure proper testing framework is being used.

DPUkyle

Approved with some nitpicky comments - thank you for the good comments/documentation in the Test task.

subprojects/plugins/src/main/java/org/gradle/api/plugins/jvm/internal/DefaultJvmTestSuite.java

subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/TestTaskIntegrationTest.groovy

tresat · 2021-11-23T02:02:52Z

@bot-gradle test this

bot-gradle · 2021-11-23T02:03:00Z

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

ljacomet

⚠️ This needs to be done against release as it should be part of 7.3.1.
Otherwise looks good overall. A few comments / questions below though:

ljacomet · 2021-11-23T08:21:06Z

...ting-jvm/src/integTest/groovy/org/gradle/testing/testsuites/TestSuitesIntegrationTest.groovy

@@ -472,8 +473,11 @@ class TestSuitesIntegrationTest extends AbstractIntegrationSpec {
            }
        """

+        executer.expectDocumentedDeprecationWarning("Accessing test options prior to setting test framework has been deprecated. This is scheduled to be removed in Gradle 8.0.")


What is the benefit of having twice the same deprecation warning printed?

subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/Test.java

tresat · 2021-11-23T13:15:32Z

Migrating to a new PR here: #19087 and closing this issue. Will address comments there.

tresat added 6 commits November 22, 2021 10:39

feat: Revert to warning instead of failing fast when test options are…

9f7c6ba

… accessed prior to test framework selection.

test: Reduce test scope to debug.

d520052

fix: Make options @nested, simplify tests checking that option change…

ac89b43

…s force test re-running.

fix: Always return the same TestFramework instance as the convention …

f0d4f8f

…from a TestSuite even when that property is queried multiple times.

test: Change behavior expectations - (can re-configure testFramework)…

d8d1b1f

… and expect new deprecation warning to fix broken tests.

test: Add tests to confirm same instance of testing framework is used…

efc24a5

… in suites consistently.

tresat requested a review from a team as a code owner November 23, 2021 00:10

bot-gradle added this to the 7.4 RC1 milestone Nov 23, 2021

tresat self-assigned this Nov 23, 2021

tresat added @core Issue owned by GBT Core in:test-suites Work related to the JvmTestSuite Plugin labels Nov 23, 2021

tresat modified the milestones: 7.4 RC1, 7.3.1 Nov 23, 2021

tresat commented Nov 23, 2021

View reviewed changes

tresat requested review from big-guy, DPUkyle and ljacomet and removed request for a team November 23, 2021 00:21

test: Locally, this passed. On CI, it failed. Seems like it should fa…

7670525

…il everywhere without these assumptions.

gradle deleted a comment from tresat Nov 23, 2021

test: Need to run with the integMultiVersionTest runner to see true s…

8ca8551

…uccess/failure locally here, as on CI.

gradle deleted a comment from tresat Nov 23, 2021

tresat commented Nov 23, 2021

View reviewed changes

DPUkyle approved these changes Nov 23, 2021

View reviewed changes

test: Fix test assumption to work on CI.

a153e3f

gradle deleted a comment from tresat Nov 23, 2021

ljacomet requested changes Nov 23, 2021

View reviewed changes

tresat mentioned this pull request Nov 23, 2021

Restore Previous Test Options Behavior (for Release Branch) #19087

Merged

tresat closed this Nov 23, 2021

tresat deleted the tt/74/restore-previous-test-options-behavior branch September 18, 2023 21:48

ov7a removed this from the 7.3.1 milestone Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore Previous Test Options Behavior #19080

Restore Previous Test Options Behavior #19080

tresat commented Nov 23, 2021 •

edited

tresat Nov 23, 2021

tresat Nov 23, 2021

tresat Nov 23, 2021

tresat Nov 23, 2021

ljacomet Nov 23, 2021

tresat Nov 23, 2021

tresat Nov 23, 2021

tresat Nov 23, 2021

tresat commented Nov 23, 2021

bot-gradle commented Nov 23, 2021

tresat Nov 23, 2021

DPUkyle left a comment

tresat commented Nov 23, 2021

bot-gradle commented Nov 23, 2021

ljacomet left a comment

ljacomet Nov 23, 2021

tresat commented Nov 23, 2021

Restore Previous Test Options Behavior #19080

Restore Previous Test Options Behavior #19080

Conversation

tresat commented Nov 23, 2021 • edited

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresat commented Nov 23, 2021

bot-gradle commented Nov 23, 2021

Choose a reason for hiding this comment

DPUkyle left a comment

Choose a reason for hiding this comment

tresat commented Nov 23, 2021

bot-gradle commented Nov 23, 2021

ljacomet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresat commented Nov 23, 2021

tresat commented Nov 23, 2021 •

edited