BeforeGroups should run before any matched test

CHANGES.txt

@@ -8,6 +8,7 @@ Fixed: GITHUB-2709: Testnames not working together with suites in suite (Martin
 Fixed: GITHUB-2704: IHookable and IConfigurable callback discrepancy (Krishnan Mahadevan)
 Fixed: GITHUB-2637: Upgrade to JDK11 as the minimum JDK requirements (Krishnan Mahadevan)
 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)
 
 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

@@ -1,14 +1,19 @@
 package org.testng.internal;
 
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+import java.util.stream.Collectors;
 import org.testng.ITestNGMethod;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
+import org.testng.log4testng.Logger;
 
 /**
  * This class wraps access to beforeGroups and afterGroups methods, since they are passed around the
@@ -21,8 +26,8 @@ public class ConfigurationGroupMethods {
   /** The list of beforeGroups methods keyed by the name of the group */
   private final Map<String, List<ITestNGMethod>> m_beforeGroupsMethods;
 
-  private final Set<String> beforeGroupsThatHaveAlreadyRun =
-      Collections.newSetFromMap(new ConcurrentHashMap<>());
+  private final Map<String, CountDownLatch> beforeGroupsThatHaveAlreadyRun =
+      new ConcurrentHashMap<>();
   private final Set<String> afterGroupsThatHaveAlreadyRun =
       Collections.newSetFromMap(new ConcurrentHashMap<>());
 
@@ -102,9 +107,17 @@ private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
     return result;
   }
 
-  public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String group) {
+  public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String[] groups) {
+    if (groups.length == 0) {
+      return Collections.emptyList();
+    }
+
     synchronized (beforeGroupsThatHaveAlreadyRun) {
-      return retrieve(beforeGroupsThatHaveAlreadyRun, m_beforeGroupsMethods, group);
+      return Arrays.stream(groups)
+          .map(t -> retrieve(beforeGroupsThatHaveAlreadyRun, m_beforeGroupsMethods, t))
+          .filter(Objects::nonNull)
+          .flatMap(Collection::stream)
+          .collect(Collectors.toList());
     }
   }
 
@@ -117,6 +130,7 @@ public List<ITestNGMethod> getAfterGroupMethodsForGroup(String group) {
   public void removeBeforeGroups(String[] groups) {
     for (String group : groups) {
       m_beforeGroupsMethods.remove(group);
+      beforeGroupsThatHaveAlreadyRun.get(group).countDown();
     }
   }
 
@@ -126,6 +140,21 @@ public void removeAfterGroups(Collection<String> groups) {
     }
   }
 
+  private static List<ITestNGMethod> retrieve(
+      Map<String, CountDownLatch> tracker, Map<String, List<ITestNGMethod>> map, String group) {
+    if (tracker.containsKey(group)) {
+      try {
+        tracker.get(group).await();
+      } catch (InterruptedException handled) {
+        Logger.getLogger(ConfigurationGroupMethods.class).error(handled.getMessage(), handled);
+        Thread.currentThread().interrupt();
+      }
+      return Collections.emptyList();
+    }
+    tracker.put(group, new CountDownLatch(1));
+    return map.get(group);
+  }
+
   private static List<ITestNGMethod> retrieve(
       Set<String> tracker, Map<String, List<ITestNGMethod>> map, String group) {
     if (tracker.contains(group)) {

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

@@ -22,7 +22,6 @@
 import org.testng.SuiteRunState;
 import org.testng.TestNGException;
 import org.testng.annotations.IConfigurationAnnotation;
-import org.testng.collections.Lists;
 import org.testng.collections.Maps;
 import org.testng.collections.Sets;
 import org.testng.internal.*;
@@ -111,18 +110,14 @@ public boolean hasConfigurationFailureFor(
    * @param arguments - A {@link GroupConfigMethodArguments} object.
    */
   public void invokeBeforeGroupsConfigurations(GroupConfigMethodArguments arguments) {
-    List<ITestNGMethod> filteredMethods = Lists.newArrayList();
     String[] groups = arguments.getTestMethod().getGroups();
 
-    for (String group : groups) {
-      List<ITestNGMethod> methods =
-          arguments.getGroupMethods().getBeforeGroupMethodsForGroup(group);
-      if (methods != null) {
-        filteredMethods.addAll(methods);
-      }
-    }
+    ITestNGMethod[] beforeMethodsArray =
+        arguments
+            .getGroupMethods()
+            .getBeforeGroupMethodsForGroup(groups)
+            .toArray(new ITestNGMethod[0]);
 
-    ITestNGMethod[] beforeMethodsArray = filteredMethods.toArray(new ITestNGMethod[0]);
     //
     // Invoke the right groups methods
     //
@@ -131,20 +126,19 @@ public void invokeBeforeGroupsConfigurations(GroupConfigMethodArguments argument
           Arrays.stream(beforeMethodsArray)
               .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);
     }
 
     //

testng-core/src/test/java/test/beforegroups/BeforeGroupsTest.java

@@ -9,6 +9,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import org.testng.ITestResult;
 import org.testng.TestListenerAdapter;
 import org.testng.TestNG;
 import org.testng.annotations.Test;
@@ -26,6 +27,8 @@
 import test.beforegroups.issue1694.BaseClassWithBeforeGroups;
 import test.beforegroups.issue2229.AnotherTestClassSample;
 import test.beforegroups.issue2229.TestClassSample;
+import test.beforegroups.issue2359.ListenerAdapter;
+import test.beforegroups.issue2359.SampleFor2359;
 import test.beforegroups.issue346.SampleTestClass;
 
 public class BeforeGroupsTest extends SimpleBaseTest {
@@ -100,6 +103,28 @@ public void ensureBeforeGroupsAreInvokedByDefaultEvenWithoutGrouping() {
     assertThat(AnotherTestClassSample.logs).containsExactlyElementsOf(expectedLogs);
   }
 
+  @Test
+  public void ensureBeforeGroupIsRunBeforeFirstTestInParallelMethodLaunch() {
+    TestNG tng = new TestNG();
+
+    tng.setTestClasses(new Class[] {SampleFor2359.class});
+
+    // bug only exists when running parallel by instances with this flag set to false
+    tng.setParallel(XmlSuite.ParallelMode.METHODS);
+    tng.setThreadCount(3);
+
+    ListenerAdapter adapter = new ListenerAdapter();
+    tng.addListener(adapter);
+
+    tng.run();
+
+    ITestResult beforeGroup = adapter.getPassedConfiguration().iterator().next();
+    adapter
+        .getPassedTests()
+        .forEach(
+            t -> assertThat(t.getStartMillis()).isGreaterThanOrEqualTo(beforeGroup.getEndMillis()));
+  }
+
   private static void createXmlTest(XmlSuite xmlSuite, String name, String group) {
     XmlTest xmlTest = new XmlTest(xmlSuite);
     xmlTest.setName(name);

testng-core/src/test/java/test/beforegroups/issue2359/ListenerAdapter.java

@@ -0,0 +1,21 @@
+package test.beforegroups.issue2359;
+
+import java.util.Collection;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import org.testng.ITestResult;
+import org.testng.TestListenerAdapter;
+
+public class ListenerAdapter extends TestListenerAdapter {
+
+  private final Collection<ITestResult> passedConfiguration = new ConcurrentLinkedQueue<>();
+
+  @Override
+  public void onConfigurationSuccess(ITestResult itr) {
+    super.onConfigurationSuccess(itr);
+    this.passedConfiguration.add(itr);
+  }
+
+  public Collection<ITestResult> getPassedConfiguration() {
+    return passedConfiguration;
+  }
+}

testng-core/src/test/java/test/beforegroups/issue2359/SampleFor2359.java

@@ -0,0 +1,26 @@
+package test.beforegroups.issue2359;
+
+import java.util.concurrent.TimeUnit;
+import org.testng.annotations.BeforeGroups;
+import org.testng.annotations.Test;
+
+public class SampleFor2359 {
+
+  @BeforeGroups(groups = "testng2359")
+  public void beforeGroup() {
+    try {
+      TimeUnit.SECONDS.sleep(2);
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+    }
+  }
+
+  @Test(groups = "testng2359")
+  public void sampleTest1() {}
+
+  @Test(groups = "testng2359")
+  public void sampleTest2() {}
+
+  @Test(groups = "testng2359")
+  public void sampleTest3() {}
+}