Merge pull request #231 from gradle/snoopcheri/fix-class-level-retry-…

…with-gradle-5

Fix class-level retry with Gradle 5.0 and Suite engine or @nested test classes

plugin/src/main/java/org/gradle/testretry/internal/executer/RetryTestResultProcessor.java

@@ -25,10 +25,13 @@
 import org.gradle.testretry.internal.filter.ClassRetryMatcher;
 import org.gradle.testretry.internal.filter.RetryFilter;
 import org.gradle.testretry.internal.testsreader.TestsReader;
+import org.gradle.util.GradleVersion;
 
 import java.lang.reflect.Method;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 import static org.gradle.api.tasks.testing.TestResult.ResultType.SKIPPED;
 
@@ -47,6 +50,7 @@ final class RetryTestResultProcessor implements TestResultProcessor {
 
     private final Map<Object, TestDescriptorInternal> activeDescriptorsById = new HashMap<>();
 
+    private final Set<String> testClassesSeenInCurrentRound = new HashSet<>();
     private TestNames currentRoundFailedTests = new TestNames();
     private TestNames previousRoundFailedTests = new TestNames();
 
@@ -76,6 +80,7 @@ public void started(TestDescriptorInternal descriptor, TestStartEvent testStartE
             delegate.started(descriptor, testStartEvent);
         } else if (!descriptor.getId().equals(rootTestDescriptorId)) {
             activeDescriptorsById.put(descriptor.getId(), descriptor);
+            registerSeenTestClass(descriptor);
             delegate.started(descriptor, testStartEvent);
         }
     }
@@ -130,6 +135,14 @@ private boolean isLifecycleFailure(String className, String name) {
         return testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, name);
     }
 
+    private void registerSeenTestClass(TestDescriptorInternal descriptor) {
+        String maybeTestClassName = descriptor.getClassName();
+
+        if (maybeTestClassName != null) {
+            testClassesSeenInCurrentRound.add(maybeTestClassName);
+        }
+    }
+
     private void addRetry(String className, String name) {
         if (classRetryMatcher.retryWholeClass(className)) {
             currentRoundFailedTests.addClass(className);
@@ -207,7 +220,53 @@ private boolean currentRoundFailedTestsExceedsMaxFailures() {
     }
 
     public RoundResult getResult() {
-        return new RoundResult(currentRoundFailedTests, previousRoundFailedTests, lastRun(), hasRetryFilteredFailures);
+        return new RoundResult(currentRoundFailedTests,
+            cleanedUpFailedTestsOfPreviousRound(),
+            lastRun(),
+            hasRetryFilteredFailures
+        );
+    }
+
+    /**
+     * When running tests via the JUnit's suite engine or using {@code @Nested} test classes,
+     * Gradle 5.0 does not report the intermediate test class nodes. This leads to a problem when such
+     * test classes are configured to be retried on class-level, as we cannot properly remove them
+     * from the previous round of failed tests without a proper test event from Gradle.
+     * <p/>
+     * For Gradle 5.0, we manually remove all test classes with no registers test methods,
+     * if we saw the test class during this round. We assume here, that those entries are
+     * just here because of the missing event from Gradle for the intermediate node.
+     * <p/>
+     * For Gradle version 5.1 and above we don't do this, as we expect events for those intermediate
+     * nodes.
+     * <p/>
+     * This solution is not perfect but still allows users to use classRetry with Gradle 5
+     * together with the suite engine and/or nested test classes.
+     *
+     * @return cleaned up failed test names of previous round
+     */
+    private TestNames cleanedUpFailedTestsOfPreviousRound() {
+        boolean isGradle50 = GradleVersion.current().getBaseVersion().equals(GradleVersion.version("5.0"));
+
+        if (isGradle50 && !testClassesSeenInCurrentRound.isEmpty() && previousRoundFailedTests.hasClassesWithoutTestNames()) {
+            TestNames testNames = new TestNames();
+            previousRoundFailedTests.stream().forEach(entry -> {
+                String testClass = entry.getKey();
+                Set<String> testMethods = entry.getValue();
+
+                if (testMethods.isEmpty()) {
+                    if (!testClassesSeenInCurrentRound.contains(testClass)) {
+                        testNames.addClass(testClass);
+                    }
+                } else {
+                    testNames.addAll(testClass, testMethods);
+                }
+            });
+
+            return testNames;
+        }
+
+        return previousRoundFailedTests;
     }
 
     public void reset(boolean lastRetry) {
@@ -216,6 +275,7 @@ public void reset(boolean lastRetry) {
         }
 
         this.lastRetry = lastRetry;
+        this.testClassesSeenInCurrentRound.clear();
         this.previousRoundFailedTests = currentRoundFailedTests;
         this.currentRoundFailedTests = new TestNames();
         this.activeDescriptorsById.clear();

plugin/src/main/java/org/gradle/testretry/internal/executer/TestNames.java

@@ -32,6 +32,10 @@ public void add(String className, String testName) {
         map.computeIfAbsent(className, ignored -> new HashSet<>()).add(testName);
     }
 
+    public void addAll(String className, Set<String> testNames) {
+        map.computeIfAbsent(className, ignored -> new HashSet<>()).addAll(testNames);
+    }
+
     public void addClass(String className) {
         map.put(className, emptySet());
     }
@@ -62,6 +66,11 @@ public boolean remove(String className, String testName) {
         }
     }
 
+    public boolean hasClassesWithoutTestNames() {
+        return map.values().stream()
+            .anyMatch(Set::isEmpty);
+    }
+
     public Stream<Map.Entry<String, Set<String>>> stream() {
         return map.entrySet().stream();
     }

plugin/src/test/groovy/org/gradle/testretry/internal/executer/TestNamesTest.groovy

@@ -0,0 +1,71 @@
+/*
+ * Copyright 2023 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.gradle.testretry.internal.executer
+
+import spock.lang.Specification
+import spock.lang.Subject
+
+class TestNamesTest extends Specification {
+
+    @Subject
+    def testNames = new TestNames()
+
+    def "removing testName works properly"() {
+        given:
+        testNames.addAll("TestClass", ["test1()", "test2()", "test10()"] as Set)
+
+        when:
+        testNames.remove("TestClass", "test2()")
+
+        then:
+        methodsFor("TestClass") ==~ ["test1()", "test10()"]
+    }
+
+    def "removing testName via predicate works properly"() {
+        given:
+        testNames.addAll("TestClass", ["test1()", "test2()", "test10()"] as Set)
+
+        when:
+        testNames.remove("TestClass", testMethod -> testMethod.contains("test1"))
+
+        then:
+        methodsFor("TestClass") ==~ ["test2()"]
+    }
+
+    def "hasClassesWithoutTestNames works properly"() {
+        when:
+        testNames.add("TestClass", "test()")
+
+        then:
+        !testNames.hasClassesWithoutTestNames()
+
+        when:
+        testNames.addClass("TestClassWithNoMethods")
+
+        then:
+        testNames.hasClassesWithoutTestNames()
+    }
+
+    private Set<String> methodsFor(String testClass) {
+        def entry = testNames.stream()
+            .filter { it.key == testClass }
+            .findFirst()
+
+        assert entry.isPresent()
+
+        entry.get().value as Set
+    }
+}

plugin/src/test/groovy/org/gradle/testretry/testframework/JUnit5FuncTest.groovy

@@ -17,20 +17,36 @@ package org.gradle.testretry.testframework
 
 import org.gradle.testretry.AbstractFrameworkFuncTest
 
+import javax.annotation.Nullable
+
 class JUnit5FuncTest extends AbstractFrameworkFuncTest {
     @Override
     String getLanguagePlugin() {
         return 'java'
     }
 
-    protected String afterClassErrorTestMethodName(String gradleVersion) {
+    private static String afterClassErrorTestMethodName(String gradleVersion) {
         gradleVersion == "5.0" ? "classMethod" : "executionError"
     }
 
-    protected String beforeClassErrorTestMethodName(String gradleVersion) {
+    private static String beforeClassErrorTestMethodName(String gradleVersion) {
         gradleVersion == "5.0" ? "classMethod" : "initializationError"
     }
 
+    private static String classAndMethodForSuite(String className, String testName, String gradleVersion) {
+        gradleVersion == "5.0" ? "${className}.${testName}" : "${className} > ${testName}"
+    }
+
+    private static String classAndMethodForNested(String parentClassName, @Nullable String nestedClassName, String testName, String gradleVersion) {
+        if (nestedClassName == null) {
+            "${parentClassName} > ${testName}"
+        } else {
+            gradleVersion == "5.0"
+                ? "${nestedClassName}.${testName}"
+                : "${nestedClassName} > ${testName}"
+        }
+    }
+
     def "handles failure in #lifecycle - exhaustive #exhaust (gradle version #gradleVersion)"(String gradleVersion, String lifecycle, boolean exhaust) {
         given:
         buildFile << """
@@ -518,14 +534,15 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
 
         then:
         with(result.output) {
-            it.count('Test1 > testOk() PASSED') == 2
-            it.count('Test1 > testFlaky() FAILED') == 1
-            it.count('Test1 > testFlaky() PASSED') == 1
+            it.count("${classAndMethodForSuite("Test1", "testOk()", gradleVersion)} PASSED") == 2
+            it.count("${classAndMethodForSuite("Test1", "testFlaky()", gradleVersion)} FAILED") == 1
+            it.count("${classAndMethodForSuite("Test1", "testFlaky()", gradleVersion)} PASSED") == 1
 
             // Test2 is retried on method level
-            it.count('Test2 > testOk() PASSED') == 1
-            it.count('Test2 > testFlaky() FAILED') == 1
-            it.count('Test2 > testFlaky() PASSED') == 1
+            it.count("${classAndMethodForSuite("Test2", "testOk()", gradleVersion)} PASSED") == 1
+            it.count("${classAndMethodForSuite("Test2", "testFlaky()", gradleVersion)} FAILED") == 1
+            it.count("${classAndMethodForSuite("Test2", "testFlaky()", gradleVersion)} PASSED") == 1
+
         }
 
         where:
@@ -580,14 +597,14 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
         then:
         with(result.output) {
             // only failing methods of TopLevelTest should be retried
-            it.count('TopLevelTest > testOk() PASSED') == 1
-            it.count('TopLevelTest > testFlaky() FAILED') == 1
-            it.count('TopLevelTest > testFlaky() PASSED') == 1
+            it.count("${classAndMethodForNested('TopLevelTest', null, 'testOk()', gradleVersion)} PASSED") == 1
+            it.count("${classAndMethodForNested('TopLevelTest', null, 'testFlaky()', gradleVersion)} FAILED") == 1
+            it.count("${classAndMethodForNested('TopLevelTest', null, 'testFlaky()', gradleVersion)} PASSED") == 1
 
             // all methods of NestedTest1 should be retried
-            it.count('NestedTest > testOk() PASSED') == 2
-            it.count('NestedTest > testFlaky() FAILED') == 1
-            it.count('NestedTest > testFlaky() PASSED') == 1
+            it.count("${classAndMethodForNested('TopLevelTest', 'NestedTest', 'testOk()', gradleVersion)} PASSED") == 2
+            it.count("${classAndMethodForNested('TopLevelTest', 'NestedTest', 'testFlaky()', gradleVersion)} FAILED") == 1
+            it.count("${classAndMethodForNested('TopLevelTest', 'NestedTest', 'testFlaky()', gradleVersion)} PASSED") == 1
         }
 
         where:
@@ -644,13 +661,13 @@ class JUnit5FuncTest extends AbstractFrameworkFuncTest {
         then:
         with(result.output) {
             // all methods of TopLevelTest are rerun
-            it.count('TopLevelTest > testOk() PASSED') == 2
-            it.count('TopLevelTest > testFlaky() FAILED') == 1
-            it.count('TopLevelTest > testFlaky() PASSED') == 1
+            it.count("${classAndMethodForNested('TopLevelTest', null, 'testOk()', gradleVersion)} PASSED") == 2
+            it.count("${classAndMethodForNested('TopLevelTest', null, 'testFlaky()', gradleVersion)} FAILED") == 1
+            it.count("${classAndMethodForNested('TopLevelTest', null, 'testFlaky()', gradleVersion)} PASSED") == 1
 
             // all methods of nested classes are retried
-            it.count('NestedTest1 > testOk() PASSED') == 2
-            it.count('NestedTest2 > testOk() PASSED') == 2
+            it.count("${classAndMethodForNested('TopLevelTest', 'NestedTest1', 'testOk()', gradleVersion)} PASSED") == 2
+            it.count("${classAndMethodForNested('TopLevelTest', 'NestedTest2', 'testOk()', gradleVersion)} PASSED") == 2
         }
 
         where: