Streamline dependencies for configurations

Closes #3000

Author: krmahadevan

CHANGES.txt

@@ -1,5 +1,6 @@
 Current (7.10.0)
 
+Fixed: GITHUB-3000: Method predecessors lookup and/or method sorting is broken in certain inheritance and naming setups (Krishnan Mahadevan)
 Fixed: GITHUB-3095: Super class annotated with ITestNGListenerFactory makes derived test class throw TestNGException on execution (Krishnan Mahadevan)
 Fixed: GITHUB-3081: Discrepancy with combination of (Shared Thread pool and Method Interceptor) (Krishnan Mahadevan)
 Fixed: GITHUB-2381: Controlling the inclusion of the listener at runtime (Krishnan Mahadevan)

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

@@ -7,6 +7,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicReference;
@@ -325,6 +326,7 @@ private static Graph<ITestNGMethod> topologicalSort(
     Map<Object, List<ITestNGMethod>> testInstances = sortMethodsByInstance(methods);
 
     XmlTest xmlTest = null;
+
     for (ITestNGMethod m : methods) {
       if (xmlTest == null) {
         xmlTest = m.getXmlTest();
@@ -358,13 +360,12 @@ private static Graph<ITestNGMethod> topologicalSort(
           !(m.isBeforeGroupsConfiguration() || m.isAfterGroupsConfiguration());
       boolean isGroupAgnosticConfigMethod = !m.isTest() && anyConfigExceptGroupConfigs;
       if (isGroupAgnosticConfigMethod) {
-        String[] groupsDependedUpon = m.getGroupsDependedUpon();
-        if (groupsDependedUpon.length > 0) {
-          for (String group : groupsDependedUpon) {
-            ITestNGMethod[] methodsThatBelongToGroup =
-                MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group);
-            predecessors.addAll(Arrays.asList(methodsThatBelongToGroup));
-          }
+        String[] groupsDependedUpon =
+            Optional.ofNullable(m.getGroupsDependedUpon()).orElse(new String[0]);
+        for (String group : groupsDependedUpon) {
+          ITestNGMethod[] methodsThatBelongToGroup =
+              MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group);
+          predecessors.addAll(Arrays.asList(methodsThatBelongToGroup));
         }
       }
 
@@ -380,6 +381,31 @@ private static Graph<ITestNGMethod> topologicalSort(
     return result;
   }
 
+  private static Comparator<ITestNGMethod> bubbleUpIndependentMethodsFirst() {
+    return (a, b) -> {
+      boolean aIsIndependent = isIndependent(a);
+      boolean bIsIndependent = isIndependent(b);
+      if (aIsIndependent && bIsIndependent) {
+        // Both a and b are independent methods. So treat them as equal.
+        return 0;
+      }
+      if (aIsIndependent) {
+        // First method is independent. So a should come before b.
+        return -1;
+      }
+      if (bIsIndependent) {
+        // Second method is independent. So a should come after b.
+        return 1;
+      }
+      // Both a and b are dependent methods. So treat them as equal
+      return 0;
+    };
+  }
+
+  private static boolean isIndependent(ITestNGMethod tm) {
+    return tm.getMethodsDependedUpon().length == 0 && tm.getGroupsDependedUpon().length == 0;
+  }
+
   /**
    * This method is used to create a map of test instances and their associated method(s) . Used to
    * decrease the scope to only a methods instance when trying to find method dependencies.
@@ -442,6 +468,18 @@ private static List<ITestNGMethod> sortMethods(
               || m.isBeforeSuiteConfiguration()
               || m.isBeforeTestConfiguration();
       MethodInheritance.fixMethodInheritance(allMethodsArray, before);
+
+      // In-case the user has ended up using "dependsOn" on configurations, then sometimes
+      // TestNG ends up finding the configuration methods in such a way that, it can cause
+      // un-expected failures. This usually happens due to the fully qualified method names
+      // acting up. So let's re-order the methods such that, the independent configurations always
+      // bubble up to the top and the ones that have dependencies get pushed to the bottom.
+      // That way, when we do a topologicalSort sort, the logic would work fine.
+
+      allMethodsArray =
+          Arrays.stream(allMethodsArray)
+              .sorted(bubbleUpIndependentMethodsFirst())
+              .toArray(ITestNGMethod[]::new);
     }
 
     topologicalSort(allMethodsArray, sl, pl, comparator);

testng-core/src/test/java/test/configuration/BeforeClassTest.java

@@ -4,13 +4,18 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.stream.Collectors;
+import org.testng.IReporter;
+import org.testng.ISuite;
+import org.testng.ITestResult;
 import org.testng.TestNG;
 import org.testng.annotations.Test;
 import org.testng.xml.XmlSuite;
 import org.testng.xml.XmlSuite.ParallelMode;
 import test.SimpleBaseTest;
 import test.configuration.issue1035.InvocationTracker;
 import test.configuration.issue1035.MyFactory;
+import test.configuration.issue3000.TestClassSample;
 
 public class BeforeClassTest extends SimpleBaseTest {
 
@@ -48,4 +53,30 @@ public void ensureBeforeClassGetsCalledConcurrentlyWhenWorkingWithFactories() {
       previousThreadId = current.getThreadId();
     }
   }
+
+  @Test(description = "GITHUB-3000")
+  public void ensureIndependentConfigurationsAlwaysRunFirstWhenUsingDependencies() {
+    TestNG testng = create(TestClassSample.class);
+    testng.setVerbose(2);
+
+    List<ITestResult> failures = new ArrayList<>();
+    testng.addListener(
+        new IReporter() {
+          @Override
+          public void generateReport(
+              List<XmlSuite> xmlSuites, List<ISuite> suites, String outputDirectory) {
+            List<ITestResult> filtered =
+                suites.stream()
+                    .flatMap(it -> it.getResults().values().stream())
+                    .flatMap(
+                        it ->
+                            it.getTestContext().getFailedConfigurations().getAllResults().stream())
+                    .collect(Collectors.toList());
+            failures.addAll(filtered);
+          }
+        });
+    testng.run();
+    assertThat(testng.getStatus()).isZero();
+    assertThat(failures).isEmpty();
+  }
 }

testng-core/src/test/java/test/configuration/issue3000/MyBaseTestSample.java

@@ -0,0 +1,21 @@
+package test.configuration.issue3000;
+
+import org.testng.annotations.BeforeClass;
+
+abstract class MyBaseTestSample implements MyInterface {
+  protected Object dependency;
+
+  public void setDependency(Object ignored) {}
+
+  @BeforeClass
+  public void setupDependency() {
+    dependency = new Object();
+  }
+
+  // Had to add the "__" to this method (This is not how it looks like in the sample provided
+  // in the GitHub issue). A combination of the fully qualified method names is what causes
+  // this method to be first found in the configuration methods by TestNG and so it causes
+  // the issue.
+  @BeforeClass(dependsOnMethods = "setupDependency")
+  public void __setupAdditionalDependency_() {}
+}

testng-core/src/test/java/test/configuration/issue3000/MyInterface.java

@@ -0,0 +1,10 @@
+package test.configuration.issue3000;
+
+@SuppressWarnings("unused")
+interface MyInterface {
+  void setDependency(Object dependency);
+
+  default Object getDependency() {
+    return null;
+  }
+}

testng-core/src/test/java/test/configuration/issue3000/TestClassSample.java

@@ -0,0 +1,17 @@
+package test.configuration.issue3000;
+
+import static org.testng.Assert.assertNotNull;
+
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+public class TestClassSample extends MyBaseTestSample {
+
+  @BeforeClass
+  public void beforeClass() {
+    assertNotNull(dependency);
+  }
+
+  @Test
+  public void test() {}
+}