Streamline dependsOnMethods for configurations

Closes #550

CHANGES.txt

@@ -1,5 +1,6 @@
 Current
 Fixed: GITHUB-2844: Deprecate support for running Spock Tests (Krishnan Mahadevan)
+Fixed: GITHUB-550: Weird @BeforeMethod and @AfterMethod behaviour with dependsOnMethods (Krishnan Mahadevan)
 Fixed: GITHUB-893: TestNG should provide an Api which allow to find all dependent of a specific test (Krishnan Mahadevan)
 New: Added .yml file extension for yaml suite files, previously only .yaml was allowed for yaml (Steven Jubb)
 Fixed: GITHUB-141: regular expression in "dependsOnMethods" does not work (Krishnan Mahadevan)

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

@@ -10,6 +10,7 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Predicate;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -90,6 +91,40 @@ protected static ITestNGMethod[] findDependedUponMethods(
     return findDependedUponMethods(m, methodsArray);
   }
 
+  private static Pair<String, Predicate<ITestNGMethod>> filterToUse(ITestNGMethod m) {
+    if (m.isBeforeMethodConfiguration()) {
+      return new Pair<>("BeforeMethod", ITestNGMethod::isBeforeMethodConfiguration);
+    }
+    if (m.isAfterMethodConfiguration()) {
+      return new Pair<>("AfterMethod", ITestNGMethod::isAfterMethodConfiguration);
+    }
+    if (m.isBeforeClassConfiguration()) {
+      return new Pair<>("BeforeClass", ITestNGMethod::isBeforeClassConfiguration);
+    }
+    if (m.isAfterClassConfiguration()) {
+      return new Pair<>("AfterClass", ITestNGMethod::isAfterClassConfiguration);
+    }
+    if (m.isBeforeTestConfiguration()) {
+      return new Pair<>("BeforeTest", ITestNGMethod::isBeforeTestConfiguration);
+    }
+    if (m.isAfterTestConfiguration()) {
+      return new Pair<>("AfterTest", ITestNGMethod::isAfterTestConfiguration);
+    }
+    if (m.isBeforeSuiteConfiguration()) {
+      return new Pair<>("BeforeSuite", ITestNGMethod::isBeforeSuiteConfiguration);
+    }
+    if (m.isAfterSuiteConfiguration()) {
+      return new Pair<>("AfterSuite", ITestNGMethod::isAfterSuiteConfiguration);
+    }
+    if (m.isBeforeGroupsConfiguration()) {
+      return new Pair<>("BeforeGroups", ITestNGMethod::isBeforeGroupsConfiguration);
+    }
+    if (m.isAfterGroupsConfiguration()) {
+      return new Pair<>("AfterGroups", ITestNGMethod::isAfterGroupsConfiguration);
+    }
+    return new Pair<>("Test", ITestNGMethod::isTest);
+  }
+
   /**
    * Finds TestNG methods that the specified TestNG method depends upon
    *
@@ -104,6 +139,25 @@ public static ITestNGMethod[] findDependedUponMethods(ITestNGMethod m, ITestNGMe
             .filter(each -> Objects.isNull(each.getRealClass().getEnclosingClass()))
             .toArray(ITestNGMethod[]::new);
     String canonicalMethodName = calculateMethodCanonicalName(m);
+    Pair<String, Predicate<ITestNGMethod>> filterPair = filterToUse(m);
+    String annotationType = filterPair.first();
+    Predicate<ITestNGMethod> predicate = filterPair.second();
+
+    if (isConfigurationMethod(m)) {
+      methods =
+          Arrays.stream(incoming)
+              .filter(tm -> !tm.equals(m)) // exclude the current config method from the list
+              .filter(predicate) // include only similar config methods
+              .toArray(ITestNGMethod[]::new);
+
+      if (methods.length == 0) {
+        String msg =
+            String.format(
+                "None of the dependencies of the method %s are annotated with [@%s].",
+                canonicalMethodName, annotationType);
+        throw new TestNGException(msg);
+      }
+    }
 
     List<ITestNGMethod> vResult = Lists.newArrayList();
     String regexp = null;
@@ -142,11 +196,17 @@ public static ITestNGMethod[] findDependedUponMethods(ITestNGMethod m, ITestNGMe
         }
         Method maybeReferringTo = findMethodByName(m, regexp);
         if (maybeReferringTo != null) {
+          String suffix = " or not included.";
+          if (isConfigurationMethod(m)) {
+            suffix = ".";
+          }
           throw new TestNGException(
               canonicalMethodName
                   + "() is depending on method "
                   + maybeReferringTo
-                  + ", which is not annotated with @Test or not included.");
+                  + ", which is not annotated with @"
+                  + annotationType
+                  + suffix);
         }
         throw new TestNGException(
             canonicalMethodName + "() depends on nonexistent method " + regexp);

testng-core/src/test/java/test/dependent/DependentTest.java

@@ -32,6 +32,11 @@
 import test.dependent.issue141.TestClassSample;
 import test.dependent.issue2658.FailingClassSample;
 import test.dependent.issue2658.PassingClassSample;
+import test.dependent.issue550.ConfigDependencySample;
+import test.dependent.issue550.ConfigDependencyWithMismatchedLevelSample;
+import test.dependent.issue550.ConfigDependsOnTestAndConfigMethodSample;
+import test.dependent.issue550.ConfigDependsOnTestMethodSample;
+import test.dependent.issue550.OrderedResultsGatherer;
 import test.dependent.issue893.DependencyTrackingListener;
 import test.dependent.issue893.MultiLevelDependenciesTestClassSample;
 
@@ -378,6 +383,62 @@ public Object[][] getUpstreamTestData() {
     };
   }
 
+  @Test(description = "GITHUB-550", dataProvider = "configDependencyTestData")
+  public void testConfigDependencies(String expectedErrorMsg, Class<?> testClassToUse) {
+    TestNG testng = create(testClassToUse);
+    String actualErrorMsg = null;
+    try {
+      testng.run();
+    } catch (TestNGException e) {
+      actualErrorMsg = e.getMessage().replace("\n", "");
+    }
+    assertThat(actualErrorMsg).isEqualTo(expectedErrorMsg);
+  }
+
+  @Test(description = "GITHUB-550")
+  public void testConfigDependenciesHappyCase() {
+    TestNG testng = create(ConfigDependencySample.class);
+    OrderedResultsGatherer gatherer = new OrderedResultsGatherer();
+    testng.addListener(gatherer);
+    testng.run();
+    assertThat(gatherer.getStartTimes()).isSorted();
+  }
+
+  @DataProvider(name = "configDependencyTestData")
+  public Object[][] configDependencyTestData() {
+    String template1 = "None of the dependencies of the method %s.%s are annotated with [@%s].";
+    String template2 =
+        "%s.%s() is depending on method public void %s.%s(), " + "which is not annotated with @%s.";
+    return new Object[][] {
+      {
+        String.format(
+            template1,
+            ConfigDependencyWithMismatchedLevelSample.class.getCanonicalName(),
+            "beforeMethod",
+            "BeforeMethod"),
+        ConfigDependencyWithMismatchedLevelSample.class
+      },
+      {
+        String.format(
+            template2,
+            ConfigDependsOnTestAndConfigMethodSample.class.getCanonicalName(),
+            "beforeMethod",
+            ConfigDependsOnTestAndConfigMethodSample.class.getCanonicalName(),
+            "testMethod",
+            "BeforeMethod"),
+        ConfigDependsOnTestAndConfigMethodSample.class
+      },
+      {
+        String.format(
+            template1,
+            ConfigDependsOnTestMethodSample.class.getCanonicalName(),
+            "beforeMethod",
+            "BeforeMethod"),
+        ConfigDependsOnTestMethodSample.class
+      }
+    };
+  }
+
   public static class MethodNameCollector implements ITestListener {
 
     private static final Function<ITestResult, String> asString =

testng-core/src/test/java/test/dependent/issue550/ConfigDependencySample.java

@@ -0,0 +1,19 @@
+package test.dependent.issue550;
+
+import java.util.concurrent.TimeUnit;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class ConfigDependencySample {
+
+  @BeforeMethod(dependsOnMethods = "anotherBeforeMethod")
+  public void beforeMethod() {}
+
+  @BeforeMethod
+  public void anotherBeforeMethod() throws InterruptedException {
+    TimeUnit.MILLISECONDS.sleep(100);
+  }
+
+  @Test
+  public void testMethod() {}
+}

testng-core/src/test/java/test/dependent/issue550/ConfigDependencyWithMismatchedLevelSample.java

@@ -0,0 +1,17 @@
+package test.dependent.issue550;
+
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class ConfigDependencyWithMismatchedLevelSample {
+
+  @BeforeClass
+  public void beforeClass() {}
+
+  @BeforeMethod(dependsOnMethods = "beforeClass")
+  public void beforeMethod() {}
+
+  @Test
+  public void testMethod() {}
+}

testng-core/src/test/java/test/dependent/issue550/ConfigDependsOnTestAndConfigMethodSample.java

@@ -0,0 +1,23 @@
+package test.dependent.issue550;
+
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class ConfigDependsOnTestAndConfigMethodSample {
+
+  @BeforeClass
+  public void beforeClass() {}
+
+  @BeforeClass
+  public void anotherBeforeClass() {}
+
+  @BeforeMethod(dependsOnMethods = {"testMethod", "anotherBeforeMethod"})
+  public void beforeMethod() {}
+
+  @BeforeMethod
+  public void anotherBeforeMethod() {}
+
+  @Test
+  public void testMethod() {}
+}

testng-core/src/test/java/test/dependent/issue550/ConfigDependsOnTestMethodSample.java

@@ -0,0 +1,13 @@
+package test.dependent.issue550;
+
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class ConfigDependsOnTestMethodSample {
+
+  @BeforeMethod(dependsOnMethods = "testMethod")
+  public void beforeMethod() {}
+
+  @Test
+  public void testMethod() {}
+}

testng-core/src/test/java/test/dependent/issue550/OrderedResultsGatherer.java

@@ -0,0 +1,21 @@
+package test.dependent.issue550;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.testng.IInvokedMethod;
+import org.testng.IInvokedMethodListener;
+import org.testng.ITestResult;
+
+public class OrderedResultsGatherer implements IInvokedMethodListener {
+
+  List<Long> startTimes = new ArrayList<>();
+
+  public List<Long> getStartTimes() {
+    return startTimes;
+  }
+
+  @Override
+  public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
+    startTimes.add(testResult.getStartMillis());
+  }
+}

testng-core/src/test/java/test/dependent/issue550/TestClassSample.java

@@ -0,0 +1,13 @@
+package test.dependent.issue550;
+
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+public class TestClassSample {
+
+  @Test(dependsOnMethods = "a")
+  public void b() {}
+
+  @BeforeMethod
+  public void a() {}
+}