Merge pull request #296 from gradle/pshevche/support-testng-engine

Support JUnit TestNG engine
gradle · Jul 15, 2024 · 899dc9a · 899dc9a
2 parents eef9b25 + f2ce44e
commit 899dc9a
Show file tree

Hide file tree

Showing 11 changed files with 341 additions and 43 deletions.
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
@@ -129,7 +129,7 @@ public void execute(JvmTestExecutionSpec spec, TestResultProcessor testResultPro
             } else if (result.lastRound) {
                 break;
             } else {
-                TestFramework retryTestFramework = testFrameworkStrategy.createRetrying(frameworkTemplate, spec.getTestFramework(), result.failedTests);
+                TestFramework retryTestFramework = testFrameworkStrategy.createRetrying(frameworkTemplate, spec.getTestFramework(), result.failedTests, result.testClassesSeenInCurrentRound);
                 testExecutionSpec = testExecutionSpecFor(retryTestFramework, spec);
                 retryTestResultProcessor.reset(++retryCount == maxRetries);
             }

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
@@ -33,7 +33,9 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.stream.Collectors;
 
+import static java.util.stream.Collectors.toMap;
 import static org.gradle.api.tasks.testing.TestResult.ResultType.SKIPPED;
 
 final class RetryTestResultProcessor implements TestResultProcessor {
@@ -91,6 +93,10 @@ public void started(TestDescriptorInternal descriptor, TestStartEvent testStartE
     @Override
     public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
         if (testId.equals(rootTestDescriptorId)) {
+            // nothing failed in the current round, but we have some un-retried tests
+            if (currentRoundFailedTests.isEmpty() && !previousRoundFailedTests.isEmpty()) {
+                ignoreExpectedUnretriedTests();
+            }
             if (!lastRun()) {
                 return;
             }
@@ -127,13 +133,24 @@ public void completed(Object testId, TestCompleteEvent testCompleteEvent) {
                         }
                     });
                 }
-
             }
         }
 
         delegate.completed(testId, testCompleteEvent);
     }
 
+    private void ignoreExpectedUnretriedTests() {
+        // check with the framework implementation if it is expected
+        Map<String, Set<String>> expectedUnretriedTests = previousRoundFailedTests.stream()
+            .collect(toMap(
+                Map.Entry::getKey,
+                entry -> entry.getValue().stream()
+                    .filter(test -> testFrameworkStrategy.isExpectedUnretriedTest(entry.getKey(), test))
+                    .collect(Collectors.toSet())
+            ));
+        expectedUnretriedTests.forEach((className, tests) -> previousRoundFailedTests.remove(className, tests::contains));
+    }
+
     private boolean isLifecycleFailure(String className, String name) {
         return testFrameworkStrategy.isLifecycleFailureTest(testsReader, className, name);
     }
@@ -253,10 +270,12 @@ private boolean currentRoundFailedTestsExceedsMaxFailures() {
     }
 
     public RoundResult getResult() {
-        return new RoundResult(currentRoundFailedTests,
+        return new RoundResult(
+            currentRoundFailedTests,
             cleanedUpFailedTestsOfPreviousRound(),
             lastRun(),
-            hasRetryFilteredFailures
+            hasRetryFilteredFailures,
+            testClassesSeenInCurrentRound
         );
     }
 

diff --git a/plugin/src/main/java/org/gradle/testretry/internal/executer/RoundResult.java b/plugin/src/main/java/org/gradle/testretry/internal/executer/RoundResult.java
@@ -15,22 +15,27 @@
  */
 package org.gradle.testretry.internal.executer;
 
+import java.util.Set;
+
 final class RoundResult {
 
     final TestNames failedTests;
     final TestNames nonRetriedTests;
     final boolean lastRound;
     final boolean hasRetryFilteredFailures;
+    final Set<String> testClassesSeenInCurrentRound;
 
     RoundResult(
         TestNames failedTests,
         TestNames nonRetriedTests,
         boolean lastRound,
-        boolean hasRetryFilteredFailures
+        boolean hasRetryFilteredFailures,
+        Set<String> testClassesSeenInCurrentRound
     ) {
         this.failedTests = failedTests;
         this.nonRetriedTests = nonRetriedTests;
         this.lastRound = lastRound;
         this.hasRetryFilteredFailures = hasRetryFilteredFailures;
+        this.testClassesSeenInCurrentRound = testClassesSeenInCurrentRound;
     }
 }
diff --git a/...java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java b/...java/org/gradle/testretry/internal/executer/framework/BaseJunitTestFrameworkStrategy.java
@@ -36,6 +36,7 @@ abstract class BaseJunitTestFrameworkStrategy implements TestFrameworkStrategy {
 
     public static final Logger LOGGER = LoggerFactory.getLogger(JunitTestFrameworkStrategy.class);
     private static final Pattern PARAMETERIZED_SUFFIX_PATTERN = Pattern.compile("(?:\\([^)]*?\\))?(?:\\[[^]]*?])?$");
+    private static final String ERROR_SYNTHETIC_TESTNG_CLASS_NAME = "UnknownClass";
     static final Set<String> ERROR_SYNTHETIC_TEST_NAMES = Collections.unmodifiableSet(
         new HashSet<>(Arrays.asList(
             "classMethod",
@@ -50,14 +51,18 @@ public boolean isLifecycleFailureTest(TestsReader testsReader, String className,
         return ERROR_SYNTHETIC_TEST_NAMES.contains(testName);
     }
 
-    protected DefaultTestFilter testFilterFor(TestNames failedTests, boolean canRunParameterizedSpockMethods, TestFrameworkTemplate template) {
-        TestFilterBuilder filter = template.filterBuilder();
-        addFilters(filter, template.testsReader, failedTests, canRunParameterizedSpockMethods);
+    @Override
+    public boolean isExpectedUnretriedTest(String className, String test) {
+        return ERROR_SYNTHETIC_TESTNG_CLASS_NAME.equals(className);
+    }
 
+    protected DefaultTestFilter testFilterFor(TestNames failedTests, boolean canRunParameterizedSpockMethods, TestFrameworkTemplate template, Set<String> testClassesSeenInCurrentRound) {
+        TestFilterBuilder filter = template.filterBuilder();
+        addFilters(filter, template.testsReader, failedTests, canRunParameterizedSpockMethods, testClassesSeenInCurrentRound);
         return filter.build();
     }
 
-    protected void addFilters(TestFilterBuilder filters, TestsReader testsReader, TestNames failedTests, boolean canRunParameterizedSpockMethods) {
+    protected void addFilters(TestFilterBuilder filters, TestsReader testsReader, TestNames failedTests, boolean canRunParameterizedSpockMethods, Set<String> testClassesSeenInCurrentRound) {
         failedTests.stream()
             .forEach(entry -> {
                 String className = entry.getKey();
@@ -69,14 +74,28 @@ protected void addFilters(TestFilterBuilder filters, TestsReader testsReader, Te
                 }
 
                 if (tests.stream().anyMatch(ERROR_SYNTHETIC_TEST_NAMES::contains)) {
-                    filters.clazz(className);
+                    if (ERROR_SYNTHETIC_TESTNG_CLASS_NAME.equals(className)) {
+                        // Gradle can't properly attribute some of the TestNG lifecycle method failures to classes
+                        // Those are @BeforeTest, @AfterTest, @AfterClass methods
+                        // In case of @BeforeTest and @AfterTest, we should retry all classes belonging to the TestNG Test
+                        // However, we don't know this association and retry all classes instead
+                        // In case of @AfterClass, we should be able to retry just a single class
+                        // Due to erroneous reporting from TestNG we don't know which one it is
+                        testClassesSeenInCurrentRound.forEach(filters::clazz);
+                    } else {
+                        filters.clazz(className);
+                    }
                     return;
                 }
 
                 if (processSpockTest(filters, testsReader, canRunParameterizedSpockMethods, className, tests)) {
                     return;
                 }
 
+                if (processTestNGTest(filters, testsReader, className, tests)) {
+                    return;
+                }
+
                 tests.forEach(name -> addPotentiallyParameterizedSuffixed(filters, className, name));
             });
     }
@@ -113,11 +132,31 @@ private boolean processSpockTest(TestFilterBuilder filters, TestsReader testsRea
                 return true;
             }
         } catch (Throwable t) {
-            LOGGER.warn("Unable to determine if class " + className + " contains Spock @Unroll parameterizations", t);
+            LOGGER.warn("Unable to determine if class {} contains Spock @Unroll parameterizations", className, t);
         }
         return false;
     }
 
+    private boolean processTestNGTest(TestFilterBuilder filters, TestsReader testsReader, String className, Set<String> tests) {
+        try {
+            Optional<TestNgClassVisitor.ClassInfo> resultOpt = testsReader.readTestClassDirClass(className, TestNgClassVisitor::new);
+            if (resultOpt.isPresent()) {
+                TestNgClassVisitor.ClassInfo result = resultOpt.get();
+
+                tests.forEach(test -> {
+                    addPotentiallyParameterizedSuffixed(filters, className, test);
+                    result.dependsOn(test).forEach(dependency -> filters.test(className, dependency));
+                });
+
+                return true;
+            }
+        } catch (Throwable t) {
+            LOGGER.warn("Unable to determine dependency between methods of a TestNG test class {}", className, t);
+        }
+
+        return false;
+    }
+
     private void addPotentiallyParameterizedSuffixed(TestFilterBuilder filters, String className, String name) {
         // It's a common pattern to add all the parameters on the end of a literal method name with []
         // The regex takes care of removing trailing (...) or (...)[...], for e.g. the following cases

diff --git a/...in/java/org/gradle/testretry/internal/executer/framework/Junit5TestFrameworkStrategy.java b/...in/java/org/gradle/testretry/internal/executer/framework/Junit5TestFrameworkStrategy.java
@@ -24,6 +24,7 @@
 import org.gradle.testretry.internal.executer.framework.TestFrameworkProvider.ProviderForCurrentGradleVersion;
 
 import java.lang.reflect.Constructor;
+import java.util.Set;
 
 import static org.gradle.testretry.internal.executer.framework.Junit5TestFrameworkStrategy.Junit5TestFrameworkProvider.testFrameworkProvider;
 import static org.gradle.testretry.internal.executer.framework.TestFrameworkStrategy.gradleVersionIsAtLeast;
@@ -37,8 +38,8 @@ public Junit5TestFrameworkStrategy(boolean isSpock2Used) {
     }
 
     @Override
-    public TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests) {
-        DefaultTestFilter failedTestsFilter = testFilterFor(failedTests, isSpock2Used, template);
+    public TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests, Set<String> testClassesSeenInCurrentRound) {
+        DefaultTestFilter failedTestsFilter = testFilterFor(failedTests, isSpock2Used, template, testClassesSeenInCurrentRound);
         return testFrameworkProvider(template, testFramework).testFrameworkFor(failedTestsFilter);
     }
 

diff --git a/...ain/java/org/gradle/testretry/internal/executer/framework/JunitTestFrameworkStrategy.java b/...ain/java/org/gradle/testretry/internal/executer/framework/JunitTestFrameworkStrategy.java
@@ -24,15 +24,16 @@
 import org.gradle.testretry.internal.executer.TestNames;
 
 import java.lang.reflect.Constructor;
+import java.util.Set;
 
 import static org.gradle.testretry.internal.executer.framework.JunitTestFrameworkStrategy.JunitTestFrameworkProvider.testFrameworkProvider;
 import static org.gradle.testretry.internal.executer.framework.TestFrameworkStrategy.gradleVersionIsAtLeast;
 
 final class JunitTestFrameworkStrategy extends BaseJunitTestFrameworkStrategy implements TestFrameworkStrategy {
 
     @Override
-    public TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests) {
-        DefaultTestFilter failedTestsFilter = testFilterFor(failedTests, true, template);
+    public TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests, Set<String> testClassesSeenInCurrentRound) {
+        DefaultTestFilter failedTestsFilter = testFilterFor(failedTests, true, template, testClassesSeenInCurrentRound);
         return testFrameworkProvider(template, testFramework).testFrameworkFor(failedTestsFilter);
     }
 

diff --git a/...src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java b/...src/main/java/org/gradle/testretry/internal/executer/framework/TestFrameworkStrategy.java
@@ -27,6 +27,7 @@
 
 import javax.annotation.Nullable;
 import java.io.File;
+import java.util.Set;
 import java.util.regex.Pattern;
 import java.util.stream.StreamSupport;
 
@@ -74,6 +75,9 @@ static boolean gradleVersionIsAtLeast(String version) {
 
     boolean isLifecycleFailureTest(TestsReader testsReader, String className, String testName);
 
-    TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests);
+    TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests, Set<String> testClassesSeenInCurrentRound);
 
+    default boolean isExpectedUnretriedTest(String className, String test) {
+        return false;
+    }
 }
diff --git a/...in/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java b/...in/java/org/gradle/testretry/internal/executer/framework/TestNgTestFrameworkStrategy.java
@@ -70,7 +70,7 @@ private boolean isLifecycleMethod(TestsReader testsReader, String testName, Test
     }
 
     @Override
-    public TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests) {
+    public TestFramework createRetrying(TestFrameworkTemplate template, TestFramework testFramework, TestNames failedTests, Set<String> testClassesSeenInCurrentRound) {
         DefaultTestFilter failedTestsFilter = testFilterFor(failedTests, template);
 
         return testFrameworkProvider(template, testFramework)