Merge pull request #177 from gradle/pshevche/flaky-setup-prevents-met…

…hod-retry Handle class setup failures preventing previously failed methods from retrying
gradle · Feb 27, 2023 · f585427 · f585427
2 parents b44f055 + 0ce11ad
commit f585427
Show file tree

Hide file tree

Showing 13 changed files with 362 additions and 23 deletions.
diff --git a/README.adoc b/README.adoc
@@ -279,13 +279,13 @@ Other versions are likely to work as well, but are not tested.
 |4.13.2
 
 |JUnit5
-|5.8.2
+|5.9.2
 
 |Spock
-|2.0-groovy-3.0
+|2.3-groovy-3.0
 
 |TestNG
-|7.4.0
+|7.5
 |===
 
 === Parameterized tests

diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestExecuter.java
@@ -138,7 +138,7 @@ public void execute(JvmTestExecutionSpec spec, TestResultProcessor testResultPro
 
     public void failWithNonRetriedTestsIfAny() {
         if (extension.getSimulateNotRetryableTest() || hasNonRetriedTests()) {
-            throw new IllegalStateException("org.gradle.test-retry was unable to retry the following test methods, which is unexpected. Please file a bug report at https://github.com/gradle/test-retry-gradle-plugin/issues" +
+            throw new IllegalStateException("The following test methods could not be retried, which is unexpected. Please file a bug report at https://github.com/gradle/test-retry-gradle-plugin/issues" +
                 lastResult.nonRetriedTests.stream()
                     .flatMap(entry -> entry.getValue().stream().map(methodName -> "   " + entry.getKey() + "#" + methodName))
                     .collect(Collectors.joining("\n", "\n", "\n")));

diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java
@@ -97,9 +97,21 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
                     addRetry(className, name);
                 }
 
+                // class-level lifecycle failures do not guarantee that all methods that failed in the previous round will be re-executed (e.g. due to class setup failure)
+                // in this case, we retry the entire class, so we ignore method-level failures for the next round
+                // we keep all lifecycle failures from previous round to make sure we report them as passed later on
+                if (isLifecycleFailure(className, name)) {
+                    previousRoundFailedTests.remove(className, n -> {
+                        if (isLifecycleFailure(className, n)) {
+                            addRetry(className, n);
+                        }
+                        return true;
+                    });
+                }
+
                 if (isClassDescriptor(descriptor)) {
                     previousRoundFailedTests.remove(className, n -> {
-                        if (testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, n)) {
+                        if (isLifecycleFailure(className, n)) {
                             emitFakePassedEvent(descriptor, testCompleteEvent, n);
                             return true;
                         } else {
@@ -114,6 +126,10 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
         delegate.completed(testId, testCompleteEvent);
     }
 
+    private boolean isLifecycleFailure(String className, String name) {
+        return testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, name);
+    }
+
     private void addRetry(String className, String name) {
         if (classRetryMatcher.retryWholeClass(className)) {
             currentRoundFailedTests.addClass(className);

diff --git a/plugin/src/test/groovy/org/gradle/testretry/AbstractPluginFuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/AbstractPluginFuncTest.groovy
@@ -51,7 +51,7 @@ abstract class AbstractPluginFuncTest extends Specification {
     }
 
     String markerFileExistsCheck(String id = "id") {
-        "Files.exists(Paths.get(\"build/marker.file.${StringEscapeUtils.escapeJava(id)}\"))"
+        """Files.exists(Paths.get("build/marker.file.${StringEscapeUtils.escapeJava(id)}"))"""
     }
 
     String flakyAssertClass() {
@@ -78,6 +78,24 @@ abstract class AbstractPluginFuncTest extends Specification {
                         throw new java.io.UncheckedIOException(e);
                     }
                 }
+
+                public static void flakyAssertPassFailPass(String id) {
+                    Path marker = Paths.get("build/marker.file." + id);
+                    try {
+                        if (Files.exists(marker)) {
+                            int counter = Integer.parseInt(new String(Files.readAllBytes(marker)));
+                            ++counter;
+                            Files.write(marker, Integer.toString(counter).getBytes());
+                            if (counter == 1) {
+                                throw new RuntimeException("fail me!");
+                            }
+                        } else {
+                            Files.write(marker, "0".getBytes());
+                        }
+                    } catch (java.io.IOException e) {
+                        throw new java.io.UncheckedIOException(e);
+                    }
+                }
             }
         """
     }
@@ -129,7 +147,11 @@ abstract class AbstractPluginFuncTest extends Specification {
     }
 
     static String flakyAssert(String id = "id", int failures = 1) {
-        return "acme.FlakyAssert.flakyAssert(\"${StringEscapeUtils.escapeJava(id)}\", $failures);"
+        return """acme.FlakyAssert.flakyAssert("${StringEscapeUtils.escapeJava(id)}", $failures);"""
+    }
+
+    static String flakyAssertPassFailPass(String id = "id") {
+        return """acme.FlakyAssert.flakyAssertPassFailPass("${StringEscapeUtils.escapeJava(id)}");"""
     }
 
     @SuppressWarnings("GroovyAssignabilityCheck")

diff --git a/plugin/src/test/groovy/org/gradle/testretry/testframework/JUnit4FuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/testframework/JUnit4FuncTest.groovy
@@ -590,4 +590,104 @@ class JUnit4FuncTest extends AbstractFrameworkFuncTest {
         where:
         gradleVersion << GRADLE_VERSIONS_UNDER_TEST
     }
+
+    def "handles flaky setup that prevents the retries of initially failed methods (gradle version #gradleVersion)"() {
+        given:
+        buildFile << """
+            test.retry.maxRetries = 2
+        """
+
+        and:
+        writeJavaTestSource """
+            package acme;
+
+            public class FlakySetupAndMethodTest {
+                @org.junit.BeforeClass
+                public static void setup() {
+                    ${flakyAssertPassFailPass("setup")}
+                }
+
+                @org.junit.Test
+                public void flakyTest() {
+                    ${flakyAssert("method")}
+                }
+
+                @org.junit.Test
+                public void successfulTest() {
+                }
+            }
+        """
+
+        when:
+        def result = gradleRunner(gradleVersion).build()
+
+        then:
+        with(result.output) {
+            it.count('flakyTest FAILED') == 1
+            it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
+            it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+            it.count('flakyTest PASSED') == 1
+            it.count('successfulTest PASSED') == 2
+        }
+
+        where:
+        gradleVersion << GRADLE_VERSIONS_UNDER_TEST
+    }
+
+    def "handles setup failure after cleanup failure (gradle version #gradleVersion)"() {
+        given:
+        buildFile << """
+            test.retry.maxRetries = 2
+        """
+
+        and:
+        writeJavaTestSource """
+            package acme;
+
+            public class FlakySetupAndCleanupTest {
+                @org.junit.BeforeClass
+                public static void setup() {
+                    ${flakyAssertPassFailPass("setup")}
+                }
+
+                @org.junit.AfterClass
+                public static void cleanup() {
+                    ${flakyAssert("cleanup")}
+                }
+
+                @org.junit.Test
+                public void flakyTest() {
+                    ${flakyAssert("method")}
+                }
+
+                @org.junit.Test
+                public void successfulTest() {
+                }
+            }
+        """
+
+        when:
+        def result = gradleRunner(gradleVersion).build()
+
+        then:
+        def differentiatesBetweenSetupAndCleanupMethods = beforeClassErrorTestMethodName(gradleVersion) != afterClassErrorTestMethodName(gradleVersion)
+        with(result.output) {
+            it.count('flakyTest FAILED') == 1
+            it.count('flakyTest PASSED') == 1
+            it.count('successfulTest PASSED') == 2
+
+            if (differentiatesBetweenSetupAndCleanupMethods) {
+                it.count("${afterClassErrorTestMethodName(gradleVersion)} FAILED") == 1
+                it.count("${afterClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+            } else {
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 2
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+            }
+        }
+
+        where:
+        gradleVersion << GRADLE_VERSIONS_UNDER_TEST
+    }
 }
diff --git a/...n/src/test/groovy/org/gradle/testretry/testframework/JUnit4ViaJUnitVintageFuncTest.groovy b/...n/src/test/groovy/org/gradle/testretry/testframework/JUnit4ViaJUnitVintageFuncTest.groovy
@@ -41,8 +41,8 @@ class JUnit4ViaJUnitVintageFuncTest extends JUnit4FuncTest {
         return '''
             dependencies {
                 testImplementation "junit:junit:4.13.2"
-                testImplementation "org.junit.jupiter:junit-jupiter-api:5.8.2"
-                testRuntimeOnly "org.junit.vintage:junit-vintage-engine:5.8.2"
+                testImplementation "org.junit.jupiter:junit-jupiter-api:5.9.2"
+                testRuntimeOnly "org.junit.vintage:junit-vintage-engine:5.9.2"
             }
 
             test {

diff --git a/plugin/src/test/groovy/org/gradle/testretry/testframework/JUnit5FuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/testframework/JUnit5FuncTest.groovy
@@ -359,6 +359,106 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
         gradleVersion << GRADLE_VERSIONS_UNDER_TEST
     }
 
+    def "handles flaky setup that prevents the retries of initially failed methods (gradle version #gradleVersion)"() {
+        given:
+        buildFile << """
+            test.retry.maxRetries = 2
+        """
+
+        and:
+        writeJavaTestSource """
+            package acme;
+
+            public class FlakySetupAndMethodTest {
+                @org.junit.jupiter.api.BeforeAll
+                public static void setup() {
+                    ${flakyAssertPassFailPass("setup")}
+                }
+
+                @org.junit.jupiter.api.Test
+                public void flakyTest() {
+                    ${flakyAssert("method")}
+                }
+
+                @org.junit.jupiter.api.Test
+                public void successfulTest() {
+                }
+            }
+        """
+
+        when:
+        def result = gradleRunner(gradleVersion).build()
+
+        then:
+        with(result.output) {
+            it.count('flakyTest() FAILED') == 1
+            it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
+            it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+            it.count('flakyTest() PASSED') == 1
+            it.count('successfulTest() PASSED') == 2
+        }
+
+        where:
+        gradleVersion << GRADLE_VERSIONS_UNDER_TEST
+    }
+
+    def "handles setup failure after cleanup failure (gradle version #gradleVersion)"() {
+        given:
+        buildFile << """
+            test.retry.maxRetries = 2
+        """
+
+        and:
+        writeJavaTestSource """
+            package acme;
+
+            public class FlakySetupAndMethodTest {
+                @org.junit.jupiter.api.BeforeAll
+                public static void setup() {
+                    ${flakyAssertPassFailPass("setup")}
+                }
+
+                @org.junit.jupiter.api.AfterAll
+                public static void cleanup() {
+                    ${flakyAssert("cleanup")}
+                }
+
+                @org.junit.jupiter.api.Test
+                public void flakyTest() {
+                    ${flakyAssert("method")}
+                }
+
+                @org.junit.jupiter.api.Test
+                public void successfulTest() {
+                }
+            }
+        """
+
+        when:
+        def result = gradleRunner(gradleVersion).build()
+
+        then:
+        def differentiatesBetweenSetupAndCleanupMethods = beforeClassErrorTestMethodName(gradleVersion) != afterClassErrorTestMethodName(gradleVersion)
+        with(result.output) {
+            it.count('flakyTest() FAILED') == 1
+            it.count('flakyTest() PASSED') == 1
+            it.count('successfulTest() PASSED') == 2
+
+            if (differentiatesBetweenSetupAndCleanupMethods) {
+                it.count("${afterClassErrorTestMethodName(gradleVersion)} FAILED") == 1
+                it.count("${afterClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 1
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+            } else {
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} FAILED") == 2
+                it.count("${beforeClassErrorTestMethodName(gradleVersion)} PASSED") == 1
+            }
+        }
+
+        where:
+        gradleVersion << GRADLE_VERSIONS_UNDER_TEST
+    }
+
     String reportedTestName(String testName) {
         testName + "()"
     }
@@ -367,9 +467,9 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
     protected String buildConfiguration() {
         return """
             dependencies {
-                testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'
-                testImplementation 'org.junit.jupiter:junit-jupiter-params:5.8.2'
-                testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.2'
+                testImplementation 'org.junit.jupiter:junit-jupiter-api:5.9.2'
+                testImplementation 'org.junit.jupiter:junit-jupiter-params:5.9.2'
+                testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.9.2'
             }
             test {
                 useJUnitPlatform()

diff --git a/plugin/src/test/groovy/org/gradle/testretry/testframework/Spock2FuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/testframework/Spock2FuncTest.groovy
@@ -38,7 +38,7 @@ class Spock2FuncTest extends SpockBaseJunit5FuncTest {
     protected String buildConfiguration() {
         return """
             dependencies {
-                implementation 'org.spockframework:spock-core:2.0-groovy-3.0'
+                implementation 'org.spockframework:spock-core:2.3-groovy-3.0'
             }
             test {
                 useJUnitPlatform()

diff --git a/plugin/src/test/groovy/org/gradle/testretry/testframework/SpockBaseJunit5FuncTest.groovy b/plugin/src/test/groovy/org/gradle/testretry/testframework/SpockBaseJunit5FuncTest.groovy
@@ -40,7 +40,7 @@ abstract class SpockBaseJunit5FuncTest extends SpockFuncTest {
     protected String buildConfiguration() {
         return """
             dependencies {
-                implementation 'org.spockframework:spock-core:2.0-groovy-3.0'
+                implementation 'org.spockframework:spock-core:2.3-groovy-3.0'
             }
             test {
                 useJUnitPlatform()