Fix Config invocation order for inheritance

CHANGES.txt

@@ -1,5 +1,6 @@
 Current
-Fixes: GITHUB-2493: Avoid NPE from TextReporter execution when a dataprovider method provides null (baflQA)
+Fixed: GITHUB-2489: Hierarchical base- and test-class @AfterClass methods out of order using groups (Krishnan Mahadevan)
+Fixed: GITHUB-2493: Avoid NPE from TextReporter execution when a dataprovider method provides null (baflQA)
 Fixed: GITHUB-2483: Asymmetric not equals (cdalexndr)
 Fixed: GITHUB-2486: assertSame/assertNotSame broken after GITHUB-2296 (Vitalii Diravka)
 

src/main/java/org/testng/TestClass.java

@@ -30,7 +30,7 @@ class TestClass extends NoOpTestClass implements ITestClass, ITestClassConfigInf
   private XmlClass xmlClass;
   private final String m_errorMsgPrefix;
 
-  private Map<String, List<ITestNGMethod>> beforeClassConfig = new HashMap<>();
+  private final Map<String, List<ITestNGMethod>> beforeClassConfig = new HashMap<>();
 
   @Override
   public List<ITestNGMethod> getAllBeforeClassMethods() {
@@ -155,26 +155,26 @@ private void initMethods() {
               testMethodFinder.getBeforeTestConfigurationMethods(m_testClass),
               annotationFinder,
               true,
-              instance);
+              instance, this.xmlTest);
       m_afterTestConfMethods =
           ConfigurationMethod.createTestConfigurationMethods(
               testMethodFinder.getAfterTestConfigurationMethods(m_testClass),
               annotationFinder,
               false,
-              instance);
+              instance, this.xmlTest);
       m_beforeClassMethods =
           ConfigurationMethod.createClassConfigurationMethods(
               testMethodFinder.getBeforeClassMethods(m_testClass),
               annotationFinder,
               true,
-              instance);
+              instance, xmlTest);
       beforeClassConfig.put(instance.toString(), Arrays.asList(m_beforeClassMethods));
       m_afterClassMethods =
           ConfigurationMethod.createClassConfigurationMethods(
               testMethodFinder.getAfterClassMethods(m_testClass),
               annotationFinder,
               false,
-              instance);
+              instance, xmlTest);
       m_beforeGroupsMethods =
           ConfigurationMethod.createBeforeConfigurationMethods(
               testMethodFinder.getBeforeGroupsConfigurationMethods(m_testClass),
@@ -189,10 +189,10 @@ private void initMethods() {
               instance);
       m_beforeTestMethods =
           ConfigurationMethod.createTestMethodConfigurationMethods(
-              testMethodFinder.getBeforeTestMethods(m_testClass), annotationFinder, true, instance);
+              testMethodFinder.getBeforeTestMethods(m_testClass), annotationFinder, true, instance, xmlTest);
       m_afterTestMethods =
           ConfigurationMethod.createTestMethodConfigurationMethods(
-              testMethodFinder.getAfterTestMethods(m_testClass), annotationFinder, false, instance);
+              testMethodFinder.getAfterTestMethods(m_testClass), annotationFinder, false, instance, xmlTest);
     }
   }
 

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

@@ -148,7 +148,7 @@ public class TestRunner
   private final IResultMap m_failedButWithinSuccessPercentageTests = new ResultMap();
   private final IResultMap m_skippedTests = new ResultMap();
 
-  private final RunInfo m_runInfo = new RunInfo();
+  private final RunInfo m_runInfo = new RunInfo(this::getCurrentXmlTest);
 
   // The host where this test was run, or null if run locally
   private String m_host;

src/main/java/org/testng/internal/ConfigurationMethod.java

@@ -27,6 +27,7 @@
 import org.testng.internal.annotations.IBeforeSuite;
 import org.testng.internal.annotations.IBeforeTest;
 import org.testng.log4testng.Logger;
+import org.testng.xml.XmlTest;
 
 public class ConfigurationMethod extends BaseTestMethod {
 
@@ -96,7 +97,8 @@ public ConfigurationMethod(
       boolean isAfterMethod,
       String[] beforeGroups,
       String[] afterGroups,
-      Object instance) {
+      Object instance,
+      XmlTest xmlTest) {
     this(
         com,
         annotationFinder,
@@ -112,6 +114,7 @@ public ConfigurationMethod(
         afterGroups,
         true,
         instance);
+    setXmlTest(xmlTest);
   }
 
   private static ITestNGMethod[] createMethods(
@@ -125,7 +128,8 @@ private static ITestNGMethod[] createMethods(
       boolean isAfterClass,
       boolean isBeforeMethod,
       boolean isAfterMethod,
-      Object instance) {
+      Object instance,
+      XmlTest xmlTest) {
     List<ITestNGMethod> result = Lists.newArrayList();
     for (ITestNGMethod method : methods) {
       if (Modifier.isStatic(method.getConstructorOrMethod().getMethod().getModifiers())) {
@@ -148,7 +152,7 @@ private static ITestNGMethod[] createMethods(
               isAfterMethod,
               new String[0],
               new String[0],
-              instance));
+              instance, xmlTest));
     }
 
     return result.toArray(new ITestNGMethod[0]);
@@ -171,14 +175,14 @@ public static ITestNGMethod[] createSuiteConfigurationMethods(
         false,
         false,
         false,
-        instance);
+        instance, null);
   }
 
   public static ITestNGMethod[] createTestConfigurationMethods(
       ITestNGMethod[] methods,
       IAnnotationFinder annotationFinder,
       boolean isBefore,
-      Object instance) {
+      Object instance, XmlTest xmlTest) {
     return createMethods(
         methods,
         annotationFinder,
@@ -190,14 +194,14 @@ public static ITestNGMethod[] createTestConfigurationMethods(
         false,
         false,
         false,
-        instance);
+        instance, xmlTest);
   }
 
   public static ITestNGMethod[] createClassConfigurationMethods(
       ITestNGMethod[] methods,
       IAnnotationFinder annotationFinder,
       boolean isBefore,
-      Object instance) {
+      Object instance, XmlTest xmlTest) {
     return createMethods(
         methods,
         annotationFinder,
@@ -209,7 +213,7 @@ public static ITestNGMethod[] createClassConfigurationMethods(
         !isBefore,
         false,
         false,
-        instance);
+        instance, xmlTest);
   }
 
   public static ITestNGMethod[] createBeforeConfigurationMethods(
@@ -233,7 +237,7 @@ public static ITestNGMethod[] createBeforeConfigurationMethods(
               false,
               isBefore ? methods[i].getBeforeGroups() : new String[0],
               new String[0],
-              instance);
+              instance, null);
     }
 
     return result;
@@ -259,14 +263,15 @@ public static ITestNGMethod[] createAfterConfigurationMethods(
             false,
             new String[0],
             isBefore ? m.getBeforeGroups() : m.getAfterGroups(),
-            instance)).toArray(ITestNGMethod[]::new);
+            instance, null))
+        .toArray(ITestNGMethod[]::new);
   }
 
   public static ITestNGMethod[] createTestMethodConfigurationMethods(
       ITestNGMethod[] methods,
       IAnnotationFinder annotationFinder,
       boolean isBefore,
-      Object instance) {
+      Object instance, XmlTest xmlTest) {
     return createMethods(
         methods,
         annotationFinder,
@@ -278,7 +283,8 @@ public static ITestNGMethod[] createTestMethodConfigurationMethods(
         false,
         isBefore,
         !isBefore,
-        instance);
+        instance,
+        xmlTest);
   }
 
   /** @return Returns the isAfterClassConfiguration. */

src/main/java/org/testng/internal/FactoryMethod.java

@@ -123,7 +123,7 @@ private void init(Object instance, IAnnotationFinder annotationFinder, Construct
   private static String[] getAllGroups(
       Class<?> declaringClass, XmlTest xmlTest, IAnnotationFinder annotationFinder) {
     // Find the groups of the factory => all groups of all test methods
-    ITestMethodFinder testMethodFinder = new TestNGMethodFinder(new RunInfo(), annotationFinder);
+    ITestMethodFinder testMethodFinder = new TestNGMethodFinder(new RunInfo(()-> xmlTest), annotationFinder);
     ITestNGMethod[] testMethods = testMethodFinder.getTestMethods(declaringClass, xmlTest);
     Set<String> groups = new HashSet<>();
     for (ITestNGMethod method : testMethods) {

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

@@ -13,7 +13,6 @@
 import java.util.regex.Pattern;
 
 import java.util.stream.Collectors;
-import org.testng.IInvokedMethod;
 import org.testng.IMethodInstance;
 import org.testng.ITestClass;
 import org.testng.ITestNGMethod;
@@ -27,6 +26,7 @@
 import org.testng.internal.annotations.IAnnotationFinder;
 import org.testng.internal.collections.Pair;
 import org.testng.util.TimeUtils;
+import org.testng.xml.XmlTest;
 
 /**
  * Collection of helper methods to help sort and arrange methods.
@@ -263,7 +263,11 @@ 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();
+      }
       result.addNode(m);
 
       List<ITestNGMethod> predecessors = Lists.newArrayList();
@@ -289,12 +293,14 @@ private static Graph<ITestNGMethod> topologicalSort(
         }
         predecessors.addAll(Arrays.asList(methodsNamed));
       }
-      String[] groupsDependedUpon = m.getGroupsDependedUpon();
-      if (groupsDependedUpon.length > 0) {
-        for (String group : groupsDependedUpon) {
-          ITestNGMethod[] methodsThatBelongToGroup =
-              MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group);
-          predecessors.addAll(Arrays.asList(methodsThatBelongToGroup));
+      if (XmlTest.isGroupBasedExecution(xmlTest)) {
+        String[] groupsDependedUpon = m.getGroupsDependedUpon();
+        if (groupsDependedUpon.length > 0) {
+          for (String group : groupsDependedUpon) {
+            ITestNGMethod[] methodsThatBelongToGroup =
+                MethodGroupsHelper.findMethodsThatBelongToGroup(m, methods, group);
+            predecessors.addAll(Arrays.asList(methodsThatBelongToGroup));
+          }
         }
       }
 
@@ -409,18 +415,6 @@ public static void fixMethodsWithClass(
     }
   }
 
-  public static List<ITestNGMethod> invokedMethodsToMethods(
-      Collection<IInvokedMethod> invokedMethods) {
-    List<ITestNGMethod> result = Lists.newArrayList();
-    for (IInvokedMethod im : invokedMethods) {
-      ITestNGMethod tm = im.getTestMethod();
-      tm.setDate(im.getDate());
-      result.add(tm);
-    }
-
-    return result;
-  }
-
   public static List<IMethodInstance> methodsToMethodInstances(List<ITestNGMethod> sl) {
     return sl.stream().map(MethodInstance::new).collect(Collectors.toList());
   }

src/main/java/org/testng/internal/MethodInheritance.java

@@ -8,10 +8,10 @@
 
 import java.util.Map.Entry;
 import java.util.Objects;
-import java.util.Optional;
 import org.testng.ITestNGMethod;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
+import org.testng.xml.XmlTest;
 
 public class MethodInheritance {
 
@@ -131,12 +131,13 @@ public static void fixMethodInheritance(ITestNGMethod[] methods, boolean before)
            *  that @BeforeClass methods in derived class depend on all @BeforeClass methods
            *  of base class. Vice versa for @AfterXXX methods
            */
+
           for (int i = 0; i < l.size() - 1; i++) {
             ITestNGMethod m1 = l.get(i);
             for (int j = i + 1; j < l.size(); j++) {
               ITestNGMethod m2 = l.get(j);
-              String[] groups = Optional.ofNullable(m2.getGroups()).orElse(new String[] {});
-              if (groups.length != 0) {
+              boolean groupMode = XmlTest.isGroupBasedExecution(m2.getXmlTest());
+              if (groupMode) {
                 //Do not resort to adding implicit depends-on if there are groups
                 continue;
               }

src/main/java/org/testng/internal/RunInfo.java

@@ -2,11 +2,13 @@
 
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.function.Supplier;
 import org.testng.IMethodSelector;
 import org.testng.IMethodSelectorContext;
 import org.testng.ITestNGMethod;
 
 import java.util.List;
+import org.testng.xml.XmlTest;
 
 /**
  * This class contains all the information needed to determine what methods should be run. It gets
@@ -16,6 +18,15 @@
 public class RunInfo {
 
   private final Set<MethodSelectorDescriptor> m_methodSelectors = new TreeSet<>();
+  private final Supplier<XmlTest> xmlTest;
+
+  public RunInfo(Supplier<XmlTest>  xmlTest) {
+    this.xmlTest = xmlTest;
+  }
+
+  public XmlTest getXmlTest() {
+    return xmlTest.get();
+  }
 
   public void addMethodSelector(IMethodSelector selector, int priority) {
     Utils.log("RunInfo", 3, "Adding method selector: " + selector + " priority: " + priority);

src/main/java/org/testng/internal/TestNGMethodFinder.java

@@ -235,7 +235,7 @@ private void addConfigurationMethod(
       String[] afterGroups,
       Object instance) {
     if (method.getDeclaringClass().isAssignableFrom(clazz)) {
-      ITestNGMethod confMethod =
+      ConfigurationMethod confMethod =
           new ConfigurationMethod(
               new ConstructorOrMethod(method),
               annotationFinder,
@@ -249,7 +249,7 @@ private void addConfigurationMethod(
               isAfterTestMethod,
               beforeGroups,
               afterGroups,
-              instance);
+              instance, this.runInfo.getXmlTest());
       results.add(confMethod);
     }
   }

src/main/java/org/testng/xml/XmlTest.java

@@ -665,4 +665,23 @@ public XmlGroups getXmlGroups() {
   public boolean nameMatchesAny(List<String> names) {
     return names.contains(getName());
   }
+
+  /**
+   * @param xmlTest - The {@link XmlTest} that represents the current <code>test</code>
+   * @return - <code>true</code> if group based selection was employed and false otherwise.
+   */
+  public static boolean isGroupBasedExecution(XmlTest xmlTest) {
+    if (xmlTest == null) {
+      return false;
+    }
+    XmlGroups xmlGroups = xmlTest.getXmlGroups();
+    if (xmlGroups == null) {
+      return false;
+    }
+    XmlRun xmlRun = xmlGroups.getRun();
+    if (xmlRun == null) {
+      return false;
+    }
+    return !xmlRun.getIncludes().isEmpty();
+  }
 }

src/test/java/org/testng/internal/MethodHelperTest.java

@@ -1,6 +1,7 @@
 package org.testng.internal;
 
 import org.testng.ITestNGMethod;
+import org.testng.Reporter;
 import org.testng.TestNGException;
 import org.testng.annotations.Test;
 import org.testng.internal.annotations.IAnnotationFinder;
@@ -30,7 +31,7 @@ public void findDependedUponMethods() throws NoSuchMethodException {
             false,
             new String[0],
             new String[0],
-            testClass);
+            testClass, Reporter.getCurrentTestResult().getTestContext().getCurrentXmlTest());
     method.addMethodDependedUpon("dummyDependsOnMethod");
     ITestNGMethod[] methods = new ITestNGMethod[0];