fix possibilty that AfterGroups method is invoked before all tests

CHANGES.txt

@@ -11,6 +11,7 @@ Fixed: GITHUB-2637: Upgrade to JDK11 as the minimum JDK requirements (Krishnan M
 Fixed: GITHUB-2734: Keep the initial order of listeners (Andrei Solntsev)
 Fixed: GITHUB-2359: Testng @BeforeGroups is running in parallel with testcases in the group (Anton Velma)
 Fixed: Possible StringIndexOutOfBoundsException in XmlReporter (Anton Velma)
+Fixed: GITHUB-2754: @AfterGroups is executed for each "finished" group when it has multiple groups defined (Anton Velma)
 
 7.5
 Fixed: GITHUB-2701: Bump gradle version to 7.3.3 to support java17 build (ZhangJian He)

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

@@ -3,6 +3,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -11,6 +12,7 @@
 import java.util.concurrent.CountDownLatch;
 import java.util.stream.Collectors;
 import org.testng.ITestNGMethod;
+import org.testng.collections.CollectionUtils;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
 import org.testng.log4testng.Logger;
@@ -57,56 +59,6 @@ public Map<String, List<ITestNGMethod>> getAfterGroupsMethods() {
     return m_afterGroupsMethods;
   }
 
-  /**
-   * @param group The group name
-   * @param method The test method
-   * @return true if the passed method is the last to run for the group. This method is used to
-   *     figure out when is the right time to invoke afterGroups methods.
-   */
-  public boolean isLastMethodForGroup(String group, ITestNGMethod method) {
-
-    // If we have more invocation to do, this is not the last one yet
-    if (method.hasMoreInvocation()) {
-      return false;
-    }
-
-    // This Mutex ensures that this edit check runs sequentially for one ITestNGMethod
-    // method at a time because this object is being shared between all the ITestNGMethod objects.
-    synchronized (this) {
-      if (m_afterGroupsMap == null) {
-        m_afterGroupsMap = initializeAfterGroupsMap();
-      }
-
-      List<ITestNGMethod> methodsInGroup = m_afterGroupsMap.get(group);
-
-      if (null == methodsInGroup || methodsInGroup.isEmpty()) {
-        return false;
-      }
-
-      methodsInGroup.remove(method);
-
-      // Note:  == is not good enough here as we may work with ITestNGMethod clones
-      return methodsInGroup.isEmpty();
-    }
-  }
-
-  private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
-    Map<String, List<ITestNGMethod>> result = Maps.newConcurrentMap();
-    for (ITestNGMethod m : m_allMethods) {
-      String[] groups = m.getGroups();
-      for (String g : groups) {
-        List<ITestNGMethod> methodsInGroup = result.computeIfAbsent(g, key -> Lists.newArrayList());
-        methodsInGroup.add(m);
-      }
-    }
-
-    synchronized (afterGroupsThatHaveAlreadyRun) {
-      afterGroupsThatHaveAlreadyRun.clear();
-    }
-
-    return result;
-  }
-
   public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String[] groups) {
     if (groups.length == 0) {
       return Collections.emptyList();
@@ -121,10 +73,40 @@ public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String[] groups) {
     }
   }
 
-  public List<ITestNGMethod> getAfterGroupMethodsForGroup(String group) {
+  public List<ITestNGMethod> getAfterGroupMethods(ITestNGMethod testMethod) {
+    if (testMethod.hasMoreInvocation() || testMethod.getGroups().length == 0) {
+      return Collections.emptyList();
+    }
+
+    Set<String> methodGroups = new HashSet<>(Arrays.asList(testMethod.getGroups()));
+
     synchronized (afterGroupsThatHaveAlreadyRun) {
-      return retrieve(afterGroupsThatHaveAlreadyRun, m_afterGroupsMethods, group);
+      if (m_afterGroupsMap == null) {
+        m_afterGroupsMap = initializeAfterGroupsMap();
+      }
+
+      return methodGroups.stream()
+          .filter(t -> isLastMethodForGroup(t, testMethod))
+          .map(t -> retrieve(afterGroupsThatHaveAlreadyRun, m_afterGroupsMethods, t))
+          .filter(Objects::nonNull)
+          .flatMap(Collection::stream)
+          .filter(t -> isAfterGroupAllowedToRunAfterTestMethod(t, methodGroups))
+          .collect(Collectors.toList());
+    }
+  }
+
+  private boolean isAfterGroupAllowedToRunAfterTestMethod(
+      ITestNGMethod afterGroupMethod, Set<String> testMethodGroups) {
+    String[] afterGroupMethodGroups = afterGroupMethod.getAfterGroups();
+    if (afterGroupMethodGroups.length == 1
+        || testMethodGroups.containsAll(Arrays.asList(afterGroupMethodGroups))) {
+      return true;
     }
+    return Arrays.stream(afterGroupMethodGroups)
+        .allMatch(
+            t ->
+                testMethodGroups.contains(t)
+                    || !CollectionUtils.hasElements(m_afterGroupsMap.get(t)));
   }
 
   public void removeBeforeGroups(String[] groups) {
@@ -140,6 +122,42 @@ public void removeAfterGroups(Collection<String> groups) {
     }
   }
 
+  /**
+   * @param group The group name
+   * @param method The test method
+   * @return true if the passed method is the last to run for the group. This method is used to
+   *     figure out when is the right time to invoke afterGroups methods.
+   */
+  private boolean isLastMethodForGroup(String group, ITestNGMethod method) {
+    List<ITestNGMethod> methodsInGroup = m_afterGroupsMap.get(group);
+
+    if (null == methodsInGroup || methodsInGroup.isEmpty()) {
+      return true;
+    }
+
+    methodsInGroup.remove(method);
+
+    // Note:  == is not good enough here as we may work with ITestNGMethod clones
+    return methodsInGroup.isEmpty();
+  }
+
+  private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
+    Map<String, List<ITestNGMethod>> result = Maps.newConcurrentMap();
+    for (ITestNGMethod m : m_allMethods) {
+      String[] groups = m.getGroups();
+      for (String g : groups) {
+        List<ITestNGMethod> methodsInGroup = result.computeIfAbsent(g, key -> Lists.newArrayList());
+        methodsInGroup.add(m);
+      }
+    }
+
+    synchronized (afterGroupsThatHaveAlreadyRun) {
+      afterGroupsThatHaveAlreadyRun.clear();
+    }
+
+    return result;
+  }
+
   private static List<ITestNGMethod> retrieve(
       Map<String, CountDownLatch> tracker, Map<String, List<ITestNGMethod>> map, String group) {
     if (tracker.containsKey(group)) {

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

@@ -7,7 +7,7 @@
 import java.lang.reflect.InvocationTargetException;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.List;
+import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -24,7 +24,17 @@
 import org.testng.annotations.IConfigurationAnnotation;
 import org.testng.collections.Maps;
 import org.testng.collections.Sets;
-import org.testng.internal.*;
+import org.testng.internal.ClassHelper;
+import org.testng.internal.ConfigurationMethod;
+import org.testng.internal.ConstructorOrMethod;
+import org.testng.internal.IConfiguration;
+import org.testng.internal.ITestResultNotifier;
+import org.testng.internal.MethodHelper;
+import org.testng.internal.Parameters;
+import org.testng.internal.RuntimeBehavior;
+import org.testng.internal.TestListenerHelper;
+import org.testng.internal.TestResult;
+import org.testng.internal.Utils;
 import org.testng.internal.annotations.AnnotationHelper;
 import org.testng.internal.invokers.ConfigMethodArguments.Builder;
 import org.testng.internal.thread.ThreadUtil;
@@ -159,59 +169,30 @@ public void invokeAfterGroupsConfigurations(GroupConfigMethodArguments arguments
       return;
     }
 
-    // See if the currentMethod is the last method in any of the groups
-    // it belongs to
-    Map<String, String> filteredGroups = Maps.newHashMap();
-    String[] groups = arguments.getTestMethod().getGroups();
-    for (String group : groups) {
-      if (arguments.getGroupMethods().isLastMethodForGroup(group, arguments.getTestMethod())) {
-        filteredGroups.put(group, group);
-      }
-    }
-
-    if (filteredGroups.isEmpty()) {
-      return;
-    }
-
-    // The list of afterMethods to run
-    Map<ITestNGMethod, ITestNGMethod> afterMethods = Maps.newHashMap();
-
-    // Now filteredGroups contains all the groups for which we need to run the afterGroups
-    // method.  Find all the methods that correspond to these groups and invoke them.
-    for (String g : filteredGroups.values()) {
-      List<ITestNGMethod> methods = arguments.getGroupMethods().getAfterGroupMethodsForGroup(g);
-      // Note:  should put them in a map if we want to make sure the same afterGroups
-      // doesn't get run twice
-      if (methods != null) {
-        for (ITestNGMethod m : methods) {
-          afterMethods.put(m, m);
-        }
-      }
-    }
-
     // Got our afterMethods, invoke them
+    Set<String> filteredGroups = new HashSet<>();
     ITestNGMethod[] filteredConfigurations =
-        afterMethods.keySet().stream()
+        arguments.getGroupMethods().getAfterGroupMethods(arguments.getTestMethod()).stream()
+            .peek(t -> filteredGroups.addAll(Arrays.asList(t.getGroups())))
             .filter(ConfigInvoker::isGroupLevelConfigurationMethod)
             .toArray(ITestNGMethod[]::new);
-    if (filteredConfigurations.length == 0) {
-      return;
+    if (filteredConfigurations.length != 0) {
+      // don't pass the IClass or the instance as the method may be external
+      // the invocation must be similar to @BeforeTest/@BeforeSuite
+      ConfigMethodArguments configMethodArguments =
+          new Builder()
+              .usingConfigMethodsAs(filteredConfigurations)
+              .forSuite(arguments.getSuite())
+              .usingParameters(arguments.getParameters())
+              .usingInstance(arguments.getInstance())
+              .forTestMethod(arguments.getTestMethod())
+              .build();
+
+      invokeConfigurations(configMethodArguments);
     }
-    // don't pass the IClass or the instance as the method may be external
-    // the invocation must be similar to @BeforeTest/@BeforeSuite
-    ConfigMethodArguments configMethodArguments =
-        new Builder()
-            .usingConfigMethodsAs(filteredConfigurations)
-            .forSuite(arguments.getSuite())
-            .usingParameters(arguments.getParameters())
-            .usingInstance(arguments.getInstance())
-            .forTestMethod(arguments.getTestMethod())
-            .build();
-
-    invokeConfigurations(configMethodArguments);
 
     // Remove the groups so they don't get run again
-    arguments.getGroupMethods().removeAfterGroups(filteredGroups.keySet());
+    arguments.getGroupMethods().removeAfterGroups(filteredGroups);
   }
 
   public void invokeConfigurations(ConfigMethodArguments arguments) {

testng-core/src/test/java/test/aftergroups/AfterGroupsBehaviorTest.java

@@ -2,6 +2,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import org.testng.ITestResult;
 import org.testng.TestNG;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -12,6 +13,8 @@
 import test.aftergroups.issue165.TestclassSampleWithSkippedMember;
 import test.aftergroups.issue1880.LocalConfigListener;
 import test.aftergroups.issue1880.TestClassSample;
+import test.aftergroups.samples.MultipleGroupsSample;
+import test.beforegroups.issue2359.ListenerAdapter;
 
 public class AfterGroupsBehaviorTest extends SimpleBaseTest {
 
@@ -33,6 +36,24 @@ public Object[][] getData() {
     };
   }
 
+  @Test
+  public void ensureAfterGroupsInvokedAfterAllTestsWhenMultipleGroupsDefined() {
+    TestNG tng = new TestNG();
+    tng.setTestClasses(new Class[] {MultipleGroupsSample.class});
+
+    ListenerAdapter adapter = new ListenerAdapter();
+    tng.addListener(adapter);
+
+    tng.run();
+
+    assertThat(adapter.getPassedConfiguration()).hasSize(1);
+    ITestResult afterGroup = adapter.getPassedConfiguration().iterator().next();
+    adapter
+        .getPassedTests()
+        .forEach(
+            t -> assertThat(t.getEndMillis()).isLessThanOrEqualTo(afterGroup.getStartMillis()));
+  }
+
   private static void runTest(
       Class<?> clazz, String groups, boolean shouldContinue, String expected) {
     XmlSuite xmlsuite = createXmlSuite("sample_suite", "sample_test", clazz);

testng-core/src/test/java/test/aftergroups/samples/MultipleGroupsSample.java

@@ -0,0 +1,18 @@
+package test.aftergroups.samples;
+
+import org.testng.annotations.AfterGroups;
+import org.testng.annotations.Test;
+
+public class MultipleGroupsSample {
+
+  @AfterGroups(groups = {"group-1", "group-2", "not-defined"})
+  public void afterGroup() {}
+
+  @Test(groups = "group-1")
+  public void test1() {}
+
+  @Test(groups = "group-2")
+  public void test2() throws InterruptedException {
+    Thread.sleep(3000);
+  }
+}