Fix: AfterGroups config annotation does not consider retries for tests (

#2690)

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-2684: AfterGroups config annotation does not consider retries for tests (Roman Morskyi)
 Fixed: GITHUB-2689: Yaml parser: implement loadClasses flag (Dzmitry Sankouski)
 Fixed: GITHUB-2676: NPE is triggered when working with ITestObjectFactory (Krishnan Mahadevan)
 Fixed: GITHUB-2674: Run onTestSkipped for each value from data provider (Krishnan Mahadevan)

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

@@ -508,52 +508,49 @@ private static void setTestStatus(ITestResult result, int status) {
   private static class StatusHolder {
 
     boolean handled = false;
+    int originalStatus;
     int status;
   }
 
-  private void handleInvocationResults(
+  private void handleInvocationResult(
       ITestNGMethod testMethod,
       ITestResult testResult,
       FailureContext failure,
       StatusHolder holder,
-      boolean wasResultUnaltered) {
-    //
-    // Go through all the results and create a TestResult for each of them
-    //
-    List<ITestResult> resultsToRetry = Lists.newArrayList();
-
-    Throwable ite = testResult.getThrowable();
-    int status =
-        computeTestStatusComparingTestResultAndStatusHolder(testResult, holder, wasResultUnaltered);
-    boolean handled = holder.handled;
-    IRetryAnalyzer retryAnalyzer = testMethod.getRetryAnalyzer(testResult);
-
-    boolean willRetry =
-        retryAnalyzer != null
-            && status == ITestResult.FAILURE
-            && failure.instances != null
-            && retryAnalyzer.retry(testResult);
+      boolean willRetry) {
 
     if (willRetry) {
-      resultsToRetry.add(testResult);
       Object instance = testResult.getInstance();
       if (!failure.instances.contains(instance)) {
         failure.instances.add(instance);
       }
       testResult.setStatus(ITestResult.SKIP);
       testResult.setWasRetried(true);
     } else {
-      testResult.setStatus(status);
-      if (status == ITestResult.FAILURE && !handled) {
+      testResult.setStatus(holder.status);
+      if (holder.status == ITestResult.FAILURE && !holder.handled) {
         int count = failure.count++;
         if (testMethod.isDataDriven()) {
           count = 0;
         }
-        handleException(ite, testMethod, testResult, count);
+        handleException(testResult.getThrowable(), testMethod, testResult, count);
       }
     }
   }
 
+  private boolean shouldRetryTestMethod(
+      ITestNGMethod testMethod,
+      ITestResult testResult,
+      FailureContext failure,
+      StatusHolder holder) {
+    IRetryAnalyzer retryAnalyzer = testMethod.getRetryAnalyzer(testResult);
+
+    return retryAnalyzer != null
+        && holder.status == ITestResult.FAILURE
+        && failure.instances != null
+        && retryAnalyzer.retry(testResult);
+  }
+
   // pass both paramValues and paramIndex to be thread safe in case parallel=true + dataprovider.
   private ITestResult invokeMethod(
       TestMethodArguments arguments, XmlSuite suite, FailureContext failureContext) {
@@ -597,7 +594,8 @@ private ITestResult invokeMethod(
       testResult.setMethod(arguments.getTestMethod());
       invokedMethod = new InvokedMethod(startTime, result);
       invokeListenersForSkippedTestResult(result, invokedMethod);
-      runAfterGroupsConfigurations(arguments, suite, testResult);
+      runAfterConfigurations(arguments, suite, testResult);
+      runAfterGroupsConfigurations(arguments);
 
       return result;
     }
@@ -699,11 +697,12 @@ private ITestResult invokeMethod(
       StatusHolder holder =
           considerExceptions(
               arguments.getTestMethod(), testResult, expectedExceptionClasses, failureContext);
-      int statusBeforeListenerInvocation = testResult.getStatus();
       runInvokedMethodListeners(AFTER_INVOCATION, invokedMethod, testResult);
-      boolean wasResultUnaltered = statusBeforeListenerInvocation == testResult.getStatus();
-      handleInvocationResults(
-          arguments.getTestMethod(), testResult, failureContext, holder, wasResultUnaltered);
+      updateStatusHolderAccordingToTestResult(testResult, holder);
+      boolean willRetryMethod =
+          shouldRetryTestMethod(arguments.getTestMethod(), testResult, failureContext, holder);
+      handleInvocationResult(
+          arguments.getTestMethod(), testResult, failureContext, holder, willRetryMethod);
 
       // If this method has a data provider and just failed, memorize the number
       // at which it failed.
@@ -730,7 +729,10 @@ private ITestResult invokeMethod(
 
       collectResults(arguments.getTestMethod(), testResult);
 
-      runAfterGroupsConfigurations(arguments, suite, testResult);
+      runAfterConfigurations(arguments, suite, testResult);
+      if (!willRetryMethod) {
+        runAfterGroupsConfigurations(arguments);
+      }
 
       // Reset the test result last. If we do this too early, Reporter.log()
       // invocations from listeners will be discarded
@@ -740,13 +742,15 @@ private ITestResult invokeMethod(
     return testResult;
   }
 
-  private void runAfterGroupsConfigurations(
+  private void runAfterConfigurations(
       TestMethodArguments arguments, XmlSuite suite, TestResult testResult) {
     ITestNGMethod[] teardownConfigMethods =
         TestNgMethodUtils.filterTeardownConfigurationMethods(
             arguments.getTestMethod(), arguments.getAfterMethods());
     runConfigMethods(arguments, suite, testResult, teardownConfigMethods);
+  }
 
+  private void runAfterGroupsConfigurations(TestMethodArguments arguments) {
     GroupConfigMethodArguments grpArgs =
         new GroupConfigMethodArguments.Builder()
             .forTestMethod(arguments.getTestMethod())
@@ -795,50 +799,51 @@ public ITestResult registerSkippedTestResult(
 
   private StatusHolder considerExceptions(
       ITestNGMethod tm,
-      ITestResult testresult,
+      ITestResult testResult,
       ExpectedExceptionsHolder exceptionsHolder,
       FailureContext failure) {
     StatusHolder holder = new StatusHolder();
-    holder.status = testresult.getStatus();
+    int status = testResult.getStatus();
     holder.handled = false;
 
-    Throwable ite = testresult.getThrowable();
-    if (holder.status == ITestResult.FAILURE && ite != null) {
+    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);
-          holder.status = ITestResult.SUCCESS;
+          testResult.setStatus(ITestResult.SUCCESS);
+          status = ITestResult.SUCCESS;
         } else {
           if (isSkipExceptionAndSkip(ite)) {
-            holder.status = ITestResult.SKIP;
+            status = ITestResult.SKIP;
           } else {
-            testresult.setThrowable(exceptionsHolder.wrongException(ite));
-            holder.status = ITestResult.FAILURE;
+            testResult.setThrowable(exceptionsHolder.wrongException(ite));
+            status = ITestResult.FAILURE;
           }
         }
       } else {
-        handleException(ite, tm, testresult, failure.count++);
+        handleException(ite, tm, testResult, failure.count++);
         holder.handled = true;
-        holder.status = testresult.getStatus();
+        status = testResult.getStatus();
       }
-    } else if (holder.status != ITestResult.SKIP && exceptionsHolder != null) {
+    } else if (status != ITestResult.SKIP && exceptionsHolder != null) {
       TestException exception = exceptionsHolder.noException(tm);
       if (exception != null) {
-        testresult.setThrowable(exception);
-        holder.status = ITestResult.FAILURE;
+        testResult.setThrowable(exception);
+        status = ITestResult.FAILURE;
       }
     }
+    holder.originalStatus = testResult.getStatus();
+    holder.status = status;
     return holder;
   }
 
-  private static int computeTestStatusComparingTestResultAndStatusHolder(
-      ITestResult testResult, StatusHolder holder, boolean wasResultUnaltered) {
-    if (wasResultUnaltered) {
-      return holder.status;
+  private static void updateStatusHolderAccordingToTestResult(
+      ITestResult testResult, StatusHolder holder) {
+    if (holder.originalStatus != testResult.getStatus()) {
+      holder.status = testResult.getStatus();
     }
-    return testResult.getStatus();
   }
 
   private class MethodInvocationAgent {

testng-core/src/test/java/test/retryAnalyzer/RetryAnalyzerTest.java

@@ -37,6 +37,7 @@
 import test.retryAnalyzer.issue1946.RetryAnalyzer;
 import test.retryAnalyzer.issue1946.TestclassSample1;
 import test.retryAnalyzer.issue1946.TestclassSample2;
+import test.retryAnalyzer.issue2684.SampleTestClassWithGroupConfigs;
 
 public class RetryAnalyzerTest extends SimpleBaseTest {
   @Test
@@ -257,6 +258,35 @@ public void testFailedRetryWithParameters() {
     Assert.assertEquals(RetryTestSample.count, 3);
   }
 
+  @Test(description = "GITHUB-2684")
+  public void testAfterConfigurationsInvokedAfterRetriedMethod() {
+    XmlSuite xmlSuite = createXmlSuite("2684_suite");
+    createXmlTest(xmlSuite, "2684_test", SampleTestClassWithGroupConfigs.class);
+    createXmlGroups(xmlSuite, "2684_group");
+    TestNG testng = create(xmlSuite);
+    InvokedMethodNameListener listener = new InvokedMethodNameListener();
+    testng.addListener(listener);
+    testng.run();
+
+    String[] expected = {
+      "beforeSuite",
+      "beforeTest",
+      "beforeClass",
+      "beforeGroups",
+      "beforeMethod",
+      "testMethod",
+      "afterMethod",
+      "beforeMethod",
+      "testMethod",
+      "afterMethod",
+      "afterGroups",
+      "afterClass",
+      "afterTest",
+      "afterSuite"
+    };
+    assertThat(listener.getInvokedMethodNames()).containsExactly(expected);
+  }
+
   private ITestResult runAssertions(Set<ITestResult> results, String methodName) {
     assertThat(results).hasSize(1);
     ITestResult firstResult = results.iterator().next();

testng-core/src/test/java/test/retryAnalyzer/issue2684/RerunAnalyzer.java

@@ -0,0 +1,20 @@
+package test.retryAnalyzer.issue2684;
+
+import org.testng.IRetryAnalyzer;
+import org.testng.ITestResult;
+
+public class RerunAnalyzer implements IRetryAnalyzer {
+
+  public static final int maxRetryCount = 1;
+  public static int secondTestRetryCount = 0;
+  private int retryCount = 0;
+
+  @Override
+  public boolean retry(ITestResult iTestResult) {
+    if (retryCount < maxRetryCount) {
+      retryCount++;
+      return true;
+    }
+    return false;
+  }
+}

testng-core/src/test/java/test/retryAnalyzer/issue2684/SampleTestClassWithGroupConfigs.java

@@ -0,0 +1,54 @@
+package test.retryAnalyzer.issue2684;
+
+import static org.testng.Assert.assertTrue;
+
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterGroups;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.AfterSuite;
+import org.testng.annotations.AfterTest;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeGroups;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.BeforeSuite;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.Test;
+
+public class SampleTestClassWithGroupConfigs {
+
+  @BeforeSuite(alwaysRun = true)
+  public void beforeSuite() {}
+
+  @BeforeTest(alwaysRun = true)
+  public void beforeTest() {}
+
+  @BeforeGroups("2684_group")
+  public void beforeGroups() {}
+
+  @BeforeClass(alwaysRun = true)
+  public void beforeClass() {}
+
+  @BeforeMethod(alwaysRun = true)
+  public void beforeMethod() {}
+
+  @Test(groups = "2684_group", retryAnalyzer = RerunAnalyzer.class)
+  public void testMethod() {
+    RerunAnalyzer.secondTestRetryCount++;
+    assertTrue(RerunAnalyzer.secondTestRetryCount > RerunAnalyzer.maxRetryCount);
+  }
+
+  @AfterMethod(alwaysRun = true)
+  public void afterMethod() {}
+
+  @AfterClass(alwaysRun = true)
+  public void afterClass() {}
+
+  @AfterGroups("2684_group")
+  public void afterGroups() {}
+
+  @AfterTest(alwaysRun = true)
+  public void afterTest() {}
+
+  @AfterSuite(alwaysRun = true)
+  public void afterSuite() {}
+}