fix(scheduling): honour dependsOnGroups declared by @BeforeGroups

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.13.0)
+Fixed: GITHUB-2804: A dependsOnGroups declared by a @BeforeGroups method now orders the tests of the group that method runs before. @BeforeGroups and @AfterGroups methods are not nodes of the scheduling graph -- they are pulled dynamically, right before the first test method of a group they target -- and MethodHelper.topologicalSort leaves the group dependencies of a group configuration method alone for exactly that reason, so the dependency reached no scheduler at all: @BeforeGroups(value = "A", dependsOnGroups = "Z") ran the whole of group A, configuration included, before group Z had started. The graph now carries that dependency on the test methods of the target group, which is where it can be scheduled, so every method of Z runs before the configuration and the configuration before the first method of A, in parallel mode as well as sequentially. A group named by such a dependency but holding no method in the current <test> stays a no-op rather than becoming an error, as it has always been. What a failure in Z does is unchanged: TestInvoker decides skips from the test method's own dependsOnGroups, so a failing Z orders A after it without skipping it (Julien Herr)
 Fixed: GITHUB-299: The chronological panel of the HTML report now closes its last <div class="chronological-class">. The block was opened on each class transition and closed only on the following one, and XMLStringBuffer.toXML() hands back the buffer without closing what is still on its tag stack, so index.html carried one unclosed <div> for every suite it reported -- 61 opening tags against 60 closing ones for a two-class suite. Every closing tag after it then matched one element too shallow, which is what put the later suites' chronological panels inside the first suite's still-open block instead of beside it. ChronologicalPanel was the only panel doing this; the other six close everything they open (Julien Herr)
 Changed: org.testng.internal.IObject names its hash code accessor getObjectHashCodes(), and its static helper objectHashCodes(Object), where both used to say instance. The old names collided with the deprecated org.testng.IClass.getInstanceHashCodes(), so a single method body in ClassImpl, NoOpTestClass and TestClass served both a contract deprecated since 7.10.0 and a current one still reached through ITestNGMethod -- which made marking those bodies deprecated only half true. Pulling the two apart also showed that ClassImpl answered null before its objects were built, where IObject promises an array; the field starts empty now, which is what the only live caller already made of that null. org.testng.ITestNGMethod.getInstanceHashCodes() is a different contract and is untouched. They are listed under Possible backward incompatible changes below (Julien Herr)
 Fixed: GITHUB-2830: A parameter whose toString() throws no longer costs the run its reports. Rendering a value runs the user's code, and org.testng.internal.Utils.toString did not guard it, so a suite with one such parameter passed every test and then lost three reports at once: no testng-results.xml at all, no jq report, and emailable-report.html left at zero bytes. Nothing failed -- TestNG catches what an IReporter throws, prints it to stderr and moves on -- the files were simply never written. Utils.toString is now failsafe the way Utils.buildStackTrace already was, which covers every caller of it, each one being a report or a console line: a value that cannot render itself is written as com.example.Thing@1b6d3586, what Object.toString() answers for a class that does not override it. This is a behaviour change for the XML reports, where a run that produced an error now produces a value. An Error is caught as well as a RuntimeException, since the catch TestNG puts around a reporter covers only Exception and a toString() that recurses on itself would otherwise end the run. TestHTMLReporter is unchanged: its own GITHUB-2830 failover calls toString() directly rather than through Utils, and spells the same identity in decimal (Julien Herr)

testng-core/src/main/java/org/testng/TestRunner.java

@@ -721,7 +721,11 @@ private void privateRun(XmlTest xmlTest) {
     IDynamicGraph<ITestNGMethod> graph =
         TimeUtils.computeAndShowTime(
             "DynamicGraphHelper.createDynamicGraph()",
-            () -> DynamicGraphHelper.createDynamicGraph(interceptedOrder, getCurrentXmlTest()));
+            () ->
+                DynamicGraphHelper.createDynamicGraph(
+                    interceptedOrder,
+                    getCurrentXmlTest(),
+                    requireGroupMethods().getBeforeGroupsMethods()));
 
     for (ITestNGMethod each : interceptedOrder) {
       if (each instanceof BaseTestMethod) {

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

@@ -2,8 +2,10 @@
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -24,11 +26,25 @@ private DynamicGraphHelper() {
     // Utility class. Defeat instantiation.
   }
 
+  /** The form for a {@code <test>} that holds no {@code @BeforeGroups} method. */
   public static DynamicGraph<ITestNGMethod> createDynamicGraph(
       ITestNGMethod[] methods, XmlTest xmlTest) {
+    return createDynamicGraph(methods, xmlTest, Collections.emptyMap());
+  }
+
+  /**
+   * @param beforeGroupsMethods - The {@code @BeforeGroups} methods of the {@code <test>}, keyed by
+   *     the group they run before, as {@link ConfigurationGroupMethods} holds them.
+   */
+  public static DynamicGraph<ITestNGMethod> createDynamicGraph(
+      ITestNGMethod[] methods,
+      XmlTest xmlTest,
+      Map<String, List<ITestNGMethod>> beforeGroupsMethods) {
     DynamicGraph<ITestNGMethod> result = new DynamicGraph<>();
 
     DependencyMap dependencyMap = new DependencyMap(methods);
+    Map<String, Map<String, List<ITestNGMethod>>> inheritedDependencies =
+        inheritedGroupDependencies(methods, beforeGroupsMethods);
 
     // Keep track of whether we have group dependencies. If we do, preserve-order needs
     // to be ignored since group dependencies create inter-class dependencies which can
@@ -76,6 +92,25 @@ public static DynamicGraph<ITestNGMethod> createDynamicGraph(
                                         m,
                                         ddm));
                       });
+
+              if (!inheritedDependencies.isEmpty()) {
+                for (String ownGroup : m.getGroups()) {
+                  Map<String, List<ITestNGMethod>> inherited =
+                      inheritedDependencies.getOrDefault(ownGroup, Collections.emptyMap());
+                  for (Map.Entry<String, List<ITestNGMethod>> each : inherited.entrySet()) {
+                    // Skip a group the method itself belongs to: making every member of a group
+                    // depend on the others is a cycle, not a dependency. The membership is decided
+                    // by the same expression that resolved the group, so a dependency written as a
+                    // pattern excludes the method the same way a plain name does.
+                    if (MethodGroupsHelper.belongsToGroup(m, each.getKey())) {
+                      continue;
+                    }
+                    hasDependencies.set(true);
+                    result.addEdges(
+                        TestRunner.PriorityWeight.dependsOnGroups.ordinal(), m, each.getValue());
+                  }
+                }
+              }
             });
 
     // Preserve order
@@ -112,6 +147,53 @@ public static DynamicGraph<ITestNGMethod> createDynamicGraph(
     return result;
   }
 
+  /**
+   * A {@code @BeforeGroups} method is not a node of this graph -- it is pulled dynamically, right
+   * before the first test method of a group it runs before, and {@code
+   * MethodHelper.topologicalSort} leaves the group dependencies of a group configuration method
+   * alone for that same reason. Its {@code dependsOnGroups} therefore has to be carried by the test
+   * methods of the group it runs before, which is the whole of GITHUB-2804. {@code @AfterGroups} is
+   * deliberately left out: it fires after the last method of its group, so the only edge that is
+   * sound for it is the same all-of-A-after-all-of-Z one, which is stronger than that annotation
+   * asks for and would reorder suites that pass today.
+   *
+   * <p>The group the configuration runs before is matched by name, which is how {@link
+   * ConfigurationGroupMethods#getBeforeGroupMethodsForGroup(String[])} picks it at invocation time.
+   *
+   * @return for each group a {@code @BeforeGroups} runs before, the test methods of every group
+   *     that configuration depends upon, keyed by the depended-upon group. A group holding no
+   *     method in this {@code <test>} is left out, so it stays the no-op it has always been.
+   */
+  private static Map<String, Map<String, List<ITestNGMethod>>> inheritedGroupDependencies(
+      ITestNGMethod[] methods, Map<String, List<ITestNGMethod>> beforeGroupsMethods) {
+    if (beforeGroupsMethods.isEmpty()) {
+      return Collections.emptyMap();
+    }
+    // Resolving a group name walks every method, so do it once per distinct name rather than once
+    // per method that inherits the dependency. DependencyMap indexes the same thing but throws on a
+    // group holding no method, where a @BeforeGroups naming one has always been a no-op.
+    Map<String, List<ITestNGMethod>> resolved = new HashMap<>();
+    Map<String, Map<String, List<ITestNGMethod>>> result = new HashMap<>();
+    for (Map.Entry<String, List<ITestNGMethod>> each : beforeGroupsMethods.entrySet()) {
+      Map<String, List<ITestNGMethod>> inherited = new LinkedHashMap<>();
+      for (ITestNGMethod configMethod : each.getValue()) {
+        for (String dependency : configMethod.getGroupsDependedUpon()) {
+          List<ITestNGMethod> targets =
+              resolved.computeIfAbsent(
+                  dependency,
+                  d -> Arrays.asList(MethodGroupsHelper.findMethodsThatBelongToGroup(methods, d)));
+          if (!targets.isEmpty()) {
+            inherited.put(dependency, targets);
+          }
+        }
+      }
+      if (!inherited.isEmpty()) {
+        result.put(each.getKey(), inherited);
+      }
+    }
+    return Collections.unmodifiableMap(result);
+  }
+
   private static Comparator<XmlClass> classComparator() {
     return Comparator.comparingInt(XmlClass::getIndex);
   }

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

@@ -10,7 +10,6 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Predicate;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -259,9 +258,23 @@ public static ITestNGMethod[] findMethodsThatBelongToGroup(
   protected static ITestNGMethod[] findMethodsThatBelongToGroup(
       ITestNGMethod[] methods, String groupRegexp) {
     final Pattern pattern = getPattern(groupRegexp);
-    Predicate<ITestNGMethod> matchingGroups =
-        tm -> Arrays.stream(tm.getGroups()).anyMatch(group -> isMatch(pattern, group));
-    return Arrays.stream(methods).filter(matchingGroups).toArray(ITestNGMethod[]::new);
+    return Arrays.stream(methods)
+        .filter(tm -> belongsToGroup(tm, pattern))
+        .toArray(ITestNGMethod[]::new);
+  }
+
+  /**
+   * @param groupRegexp regex representing the group, as {@code dependsOnGroups} spells it
+   * @return whether {@code method} belongs to a group the expression matches. This is the
+   *     membership test {@link #findMethodsThatBelongToGroup(ITestNGMethod[], String)} filters on,
+   *     for a caller that needs to ask it of a single method.
+   */
+  protected static boolean belongsToGroup(ITestNGMethod method, String groupRegexp) {
+    return belongsToGroup(method, getPattern(groupRegexp));
+  }
+
+  private static boolean belongsToGroup(ITestNGMethod method, Pattern pattern) {
+    return Arrays.stream(method.getGroups()).anyMatch(group -> isMatch(pattern, group));
   }
 
   private static Boolean isMatch(Pattern pattern, String group) {

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

@@ -34,7 +34,7 @@
 /** Collection of helper methods to help sort and arrange methods. */
 public class MethodHelper {
   private static final Map<ITestNGMethod[], Graph<ITestNGMethod>> GRAPH_CACHE =
       new ConcurrentHashMap<>();
   private static final Map<Method, String> CANONICAL_NAME_CACHE = new ConcurrentHashMap<>();
   private static final Map<Pair<String, String>, Boolean> MATCH_CACHE = new ConcurrentHashMap<>();
 
@@ -353,6 +353,9 @@
         }
         predecessors.addAll(Arrays.asList(methodsNamed));
       }
+      // A group configuration method is left out: it is resolved against configuration methods of
+      // its own kind here, where a group names test methods. DynamicGraphHelper is what carries a
+      // @BeforeGroups dependsOnGroups over to the tests of the group it runs before.
       boolean anyConfigExceptGroupConfigs =
           !(m.isBeforeGroupsConfiguration() || m.isAfterGroupsConfiguration());
       boolean isGroupAgnosticConfigMethod = !m.isTest() && anyConfigExceptGroupConfigs;
@@ -497,7 +500,7 @@
     return result;
   }
 
   /** @return A sorted array containing all the methods 'method' depends on */
   public static List<ITestNGMethod> getMethodsDependedUpon(
       ITestNGMethod method, ITestNGMethod[] methods, Comparator<ITestNGMethod> comparator) {
     Graph<ITestNGMethod> g = GRAPH_CACHE.get(methods);

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

@@ -28,6 +28,8 @@
 import test.beforegroups.issue2229.TestClassSample;
 import test.beforegroups.issue2359.ListenerAdapter;
 import test.beforegroups.issue2359.SampleFor2359;
+import test.beforegroups.issue2804.GroupDependencySample;
+import test.beforegroups.issue2804.GroupPatternDependencySample;
 import test.beforegroups.issue346.SampleTestClass;
 
 public class BeforeGroupsTest extends SimpleBaseTest {
@@ -124,6 +126,39 @@ public void ensureBeforeGroupIsRunBeforeFirstTestInParallelMethodLaunch() {
             t -> assertThat(t.getStartMillis()).isGreaterThanOrEqualTo(beforeGroup.getEndMillis()));
   }
 
+  @Test(description = "GITHUB-2804")
+  public void ensureDependsOnGroupsOfBeforeGroupsOrdersTheGroupItRunsBefore() {
+    InvokedMethodNameListener listener = run(GroupDependencySample.class);
+
+    // Recorded in afterInvocation, so this is the order in which the methods completed.
+    assertThat(listener.getMethodsForTestClass(GroupDependencySample.class))
+        .containsExactly("z1", "z2", "setUpA", "a1", "a2");
+  }
+
+  @Test(description = "GITHUB-2804")
+  public void ensureDependsOnGroupsOfBeforeGroupsIsHonouredInParallelMode() {
+    XmlSuite xmlSuite = createXmlSuite("2804_suite", "2804_test", GroupDependencySample.class);
+    xmlSuite.setParallel(XmlSuite.ParallelMode.METHODS);
+    xmlSuite.setThreadCount(4);
+
+    InvokedMethodNameListener listener = run(xmlSuite);
+
+    assertThat(listener.getMethodsForTestClass(GroupDependencySample.class))
+        .containsExactlyInAnyOrder("z1", "z2", "setUpA", "a1", "a2")
+        .containsSubsequence("z1", "setUpA", "a1")
+        .containsSubsequence("z2", "setUpA", "a2");
+  }
+
+  @Test(description = "GITHUB-2804")
+  public void ensureABeforeGroupsDependencyWrittenAsAPatternDoesNotSelfDepend() {
+    InvokedMethodNameListener listener = run(GroupPatternDependencySample.class);
+
+    assertThat(listener.getMethodsForTestClass(GroupPatternDependencySample.class))
+        .containsExactlyInAnyOrder("setUpA", "az1", "az2", "z3")
+        .containsSubsequence("setUpA", "az1")
+        .containsSubsequence("setUpA", "az2");
+  }
+
   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/issue2804/GroupDependencySample.java

@@ -0,0 +1,27 @@
+package test.beforegroups.issue2804;
+
+import org.testng.annotations.BeforeGroups;
+import org.testng.annotations.Test;
+
+/**
+ * GITHUB-2804 (and the sample contributed by PR #2025): the priorities make the natural ordering
+ * prefer group {@code A} over group {@code Z}, so the only thing that can put {@code Z} first is
+ * the dependency declared by {@link #setUpA()}.
+ */
+public class GroupDependencySample {
+
+  @Test(groups = "A", priority = 1)
+  public void a1() {}
+
+  @Test(groups = "A", priority = 2)
+  public void a2() {}
+
+  @BeforeGroups(value = "A", dependsOnGroups = "Z")
+  public void setUpA() {}
+
+  @Test(groups = "Z", priority = 3)
+  public void z1() {}
+
+  @Test(groups = "Z", priority = 4)
+  public void z2() {}
+}

testng-core/src/test/java/test/beforegroups/issue2804/GroupPatternDependencySample.java

@@ -0,0 +1,28 @@
+package test.beforegroups.issue2804;
+
+import org.testng.annotations.BeforeGroups;
+import org.testng.annotations.Test;
+
+/**
+ * GITHUB-2804: {@code dependsOnGroups} names a group by regular expression, and {@link #az1()} and
+ * {@link #az2()} belong to both the group the configuration runs before and a group that expression
+ * matches.
+ */
+public class GroupPatternDependencySample {
+
+  @BeforeGroups(value = "A", dependsOnGroups = "Z.*")
+  public void setUpA() {}
+
+  @Test(
+      groups = {"A", "Z1"},
+      priority = 1)
+  public void az1() {}
+
+  @Test(
+      groups = {"A", "Z1"},
+      priority = 2)
+  public void az2() {}
+
+  @Test(groups = "Z2", priority = 3)
+  public void z3() {}
+}