Streamline running Parallel Dataproviders+retries

Closes #2934

Author: krmahadevan

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-2934: Parallel Dataproviders & retries causes test result count to be skewed (Krishnan Mahadevan)
 Fixed: GITHUB-2925: Issue in ITestcontext.getAllTestMethods() with annotation @BeforeSuite (Krishnan Mahadevan)
 Fixed: GITHUB-2928: The constructor of TestRunner encountered NBC changes in 7.8.0 (Krishnan Mahadevan)
 Fixed: GITHUB-581: Parameters of nested test suites are overridden(Krishnan Mahadevan)

testng-core/src/main/java/org/testng/internal/BaseTestMethod.java

@@ -6,6 +6,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Callable;
@@ -817,7 +818,8 @@ private IRetryAnalyzer getRetryAnalyzerConsideringMethodParameters(ITestResult t
       this.m_retryAnalyzer = computeRetryAnalyzerInstanceToUse(tr);
       return this.m_retryAnalyzer;
     }
-    final String keyAsString = getSimpleName() + "#" + getParameterInvocationCount();
+
+    final String keyAsString = getSimpleName() + "#" + hashParameters(tr);
     return m_testMethodToRetryAnalyzer.computeIfAbsent(
         keyAsString,
         key -> {
@@ -827,6 +829,11 @@ private IRetryAnalyzer getRetryAnalyzerConsideringMethodParameters(ITestResult t
         });
   }
 
+  private int hashParameters(ITestResult itr) {
+    Object[] parameters = itr.getParameters();
+    return Objects.hash(parameters);
+  }
+
   private static boolean isNotParameterisedTest(ITestResult tr) {
     return Optional.ofNullable(tr.getParameters()).orElse(new Object[0]).length == 0;
   }

testng-core/src/main/java/org/testng/internal/invokers/ITestInvoker.java

@@ -1,6 +1,8 @@
 package org.testng.internal.invokers;
 
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.testng.IInvokedMethod;
 import org.testng.ITestContext;
 import org.testng.ITestNGMethod;
@@ -14,9 +16,9 @@ public interface ITestInvoker {
 
   class FailureContext {
 
-    int count = 0;
+    AtomicInteger count = new AtomicInteger(0);
     List<Object> instances = Lists.newArrayList();
-    boolean representsRetriedMethod = false;
+    AtomicBoolean representsRetriedMethod = new AtomicBoolean(false);
   }
 
   List<ITestResult> invokeTestMethods(

testng-core/src/main/java/org/testng/internal/invokers/MethodRunner.java

@@ -59,13 +59,14 @@ public List<ITestResult> runInSequence(
           result.addAll(tmpResults);
         } else {
           List<ITestResult> retryResults = Lists.newArrayList();
-          failure = testInvoker.retryFailed(tmArguments, retryResults, failure.count, context);
+          failure =
+              testInvoker.retryFailed(tmArguments, retryResults, failure.count.get(), context);
           result.addAll(retryResults);
         }
 
         // If we have a failure, skip all the
         // other invocationCounts
-        if (failure.count > 0
+        if (failure.count.get() > 0
             && (skipFailedInvocationCounts
                 || tmArguments.getTestMethod().skipFailedInvocations())) {
           while (invocationCount.getAndDecrement() > 0) {
@@ -122,7 +123,7 @@ public List<ITestResult> runInParallel(
               context,
               skipFailedInvocationCounts,
               invocationCount.get(),
-              failure.count,
+              failure.count.get(),
               testInvoker.getNotifier());
       workers.add(w);
       // testng387: increment the param index in the bag.

testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java

@@ -234,8 +234,8 @@ public FailureContext retryFailed(
       int failureCount,
       ITestContext testContext) {
     FailureContext failure = new FailureContext();
-    failure.count = failureCount;
-    failure.representsRetriedMethod = true;
+    failure.count.set(failureCount);
+    failure.representsRetriedMethod.set(true);
     do {
       failure.instances = Lists.newArrayList();
       Object[] parameterValues = arguments.getParameterValues();
@@ -533,7 +533,7 @@ private void handleInvocationResult(
     } else {
       testResult.setStatus(holder.status);
       if (holder.status == ITestResult.FAILURE && !holder.handled) {
-        int count = failure.count++;
+        int count = failure.count.getAndIncrement();
         if (testMethod.isDataDriven()) {
           count = 0;
         }
@@ -580,7 +580,7 @@ private ITestResult invokeMethod(
     long startTime = System.currentTimeMillis();
     InvokedMethod invokedMethod = new InvokedMethod(startTime, testResult);
 
-    if (!failureContext.representsRetriedMethod
+    if (!failureContext.representsRetriedMethod.get()
         && invoker.hasConfigurationFailureFor(
             arguments.getTestMethod(),
             arguments.getTestMethod().getGroups(),
@@ -827,46 +827,50 @@ public ITestResult registerSkippedTestResult(
     return result;
   }
 
+  private static final Object LOCK = new Object();
+
   private StatusHolder considerExceptions(
       ITestNGMethod tm,
       ITestResult testResult,
       ExpectedExceptionsHolder exceptionsHolder,
       FailureContext failure) {
-    StatusHolder holder = new StatusHolder();
-    int status = testResult.getStatus();
-    holder.handled = false;
-
-    Throwable ite = testResult.getThrowable();
-    if (status == ITestResult.FAILURE && ite != null) {
-
-      //  Invocation caused an exception, see if the method was annotated with @ExpectedException
-      if (exceptionsHolder != null) {
-        if (exceptionsHolder.isExpectedException(ite)) {
-          testResult.setStatus(ITestResult.SUCCESS);
-          status = ITestResult.SUCCESS;
-        } else {
-          if (isSkipExceptionAndSkip(ite)) {
-            status = ITestResult.SKIP;
+    synchronized (LOCK) {
+      StatusHolder holder = new StatusHolder();
+      int status = testResult.getStatus();
+      holder.handled = false;
+
+      Throwable ite = testResult.getThrowable();
+      if (status == ITestResult.FAILURE && ite != null) {
+
+        //  Invocation caused an exception, see if the method was annotated with @ExpectedException
+        if (exceptionsHolder != null) {
+          if (exceptionsHolder.isExpectedException(ite)) {
+            testResult.setStatus(ITestResult.SUCCESS);
+            status = ITestResult.SUCCESS;
           } else {
-            testResult.setThrowable(exceptionsHolder.wrongException(ite));
-            status = ITestResult.FAILURE;
+            if (isSkipExceptionAndSkip(ite)) {
+              status = ITestResult.SKIP;
+            } else {
+              testResult.setThrowable(exceptionsHolder.wrongException(ite));
+              status = ITestResult.FAILURE;
+            }
           }
+        } else {
+          handleException(ite, tm, testResult, failure.count.getAndIncrement());
+          holder.handled = true;
+          status = testResult.getStatus();
+        }
+      } else if (status != ITestResult.SKIP && exceptionsHolder != null) {
+        TestException exception = exceptionsHolder.noException(tm);
+        if (exception != null) {
+          testResult.setThrowable(exception);
+          status = ITestResult.FAILURE;
         }
-      } else {
-        handleException(ite, tm, testResult, failure.count++);
-        holder.handled = true;
-        status = testResult.getStatus();
-      }
-    } else if (status != ITestResult.SKIP && exceptionsHolder != null) {
-      TestException exception = exceptionsHolder.noException(tm);
-      if (exception != null) {
-        testResult.setThrowable(exception);
-        status = ITestResult.FAILURE;
       }
+      holder.originalStatus = testResult.getStatus();
+      holder.status = status;
+      return holder;
     }
-    holder.originalStatus = testResult.getStatus();
-    holder.status = status;
-    return holder;
   }
 
   private static void updateStatusHolderAccordingToTestResult(

testng-core/src/main/java/org/testng/internal/invokers/TestMethodWithDataProviderMethodWorker.java

@@ -74,7 +74,7 @@ public List<ITestResult> call() {
     XmlSuite suite = m_testContext.getSuite().getXmlSuite();
 
     final ITestInvoker.FailureContext failure = new ITestInvoker.FailureContext();
-    failure.count = m_failureCount;
+    failure.count.set(m_failureCount);
     try {
       tmpResults.add(
           m_testInvoker.invokeTestMethod(
@@ -92,15 +92,16 @@ public List<ITestResult> call() {
               suite,
               failure));
     } finally {
-      m_failureCount = failure.count;
+      m_failureCount = failure.count.get();
       if (failure.instances.isEmpty()) {
         m_testResults.addAll(tmpResults);
       } else {
         for (Object instance : failure.instances) {
           List<ITestResult> retryResults = Lists.newArrayList();
 
           m_failureCount =
-              m_testInvoker.retryFailed(
+              m_testInvoker
+                  .retryFailed(
                       new Builder()
                           .usingInstance(instance)
                           .forTestMethod(m_testMethod)
@@ -115,7 +116,8 @@ public List<ITestResult> call() {
                       retryResults,
                       m_failureCount,
                       m_testContext)
-                  .count;
+                  .count
+                  .get();
           m_testResults.addAll(retryResults);
         }
       }

testng-core/src/test/java/test/dataprovider/DataProviderTest.java

@@ -47,6 +47,9 @@
 import test.dataprovider.issue2819.TestClassUsingDataProviderRetrySample;
 import test.dataprovider.issue2819.TestClassWithMultipleRetryImplSample;
 import test.dataprovider.issue2888.SkipDataProviderSample;
+import test.dataprovider.issue2934.TestCaseSample;
+import test.dataprovider.issue2934.TestCaseSample.CoreListener;
+import test.dataprovider.issue2934.TestCaseSample.ToggleDataProvider;
 
 public class DataProviderTest extends SimpleBaseTest {
 
@@ -592,4 +595,78 @@ public void ensureParametersCopiedOnConfigFailures() {
     testNG.run();
     assertThat(listener.getParameters()).containsExactlyElementsOf(Arrays.asList(1, 2, 3, 4, 5));
   }
+
+  @Test(description = "GITHUB-2934")
+  public void ensureParallelDataProviderWithRetryAnalyserWorks() {
+    runTest(true);
+  }
+
+  @Test(description = "GITHUB-2934")
+  public void ensureSequentialDataProviderWithRetryAnalyserWorks() {
+    runTest(false);
+  }
+
+  private static void runTest(boolean isParallel) {
+    TestNG testng = create(TestCaseSample.class);
+    CoreListener listener = new CoreListener();
+    ToggleDataProvider transformer = new ToggleDataProvider(isParallel);
+    testng.addListener(listener);
+    testng.addListener(transformer);
+    testng.setVerbose(2);
+    testng.run();
+    SoftAssertions softly = new SoftAssertions();
+
+    softly
+        .assertThat(listener.totalTests())
+        .withFailMessage("We should have had 9 test results in total.")
+        .isEqualTo(9);
+
+    // Assert passed tests
+    softly
+        .assertThat(listener.getPassedTests())
+        .withFailMessage("We should have had ONLY 1 passed test.")
+        .hasSize(1);
+    listener
+        .getPassedTests()
+        .forEach(
+            each ->
+                softly
+                    .assertThat(each.wasRetried())
+                    .withFailMessage(
+                        "Passed test " + stringify(each) + " should NOT have been retried")
+                    .isFalse());
+
+    // Assert failed tests
+    assertThat(listener.getFailedTests())
+        .withFailMessage("We should have had 2 failed tests.")
+        .hasSize(2);
+    listener
+        .getFailedTests()
+        .forEach(
+            each ->
+                softly
+                    .assertThat(each.wasRetried())
+                    .withFailMessage(
+                        "Failed test " + stringify(each) + " should NOT have been retried")
+                    .isFalse());
+
+    // Assert skipped tests
+    assertThat(listener.getSkippedTests())
+        .withFailMessage("We should have had 6 skipped tests due to retries.")
+        .hasSize(6);
+    listener
+        .getSkippedTests()
+        .forEach(
+            each ->
+                softly
+                    .assertThat(each.wasRetried())
+                    .withFailMessage(
+                        "Skipped test " + stringify(each) + " should have been retried")
+                    .isTrue());
+    softly.assertAll();
+  }
+
+  private static String stringify(ITestResult itr) {
+    return "(" + Arrays.toString(itr.getParameters()) + ")";
+  }
 }