Segregate native & user listeners before ordering

Closes #2771

Author: krmahadevan

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-2771: After upgrading to TestNG 7.5.0, setting ITestResult.status to FAILURE doesn't fail the test anymore (Julien Herr & Krishnan Mahadevan)
 Fixed: GITHUB-2796: Option for onAfterClass to run after @AfterClass
 Fixed: GITHUB-2857: XmlTest index is not set for test suites invoked with YAML
 

testng-collections/src/main/java/org/testng/collections/Lists.java

@@ -76,10 +76,4 @@ public static <T> List<T> merge(List<T> l1, BiPredicate<T, T> condition, List<T>
             });
     return result;
   }
-
-  public static <K> List<K> newReversedArrayList(Collection<K> c) {
-    List<K> list = newArrayList(c);
-    Collections.reverse(list);
-    return list;
-  }
 }

testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java

@@ -1,5 +1,8 @@
 package org.testng.internal;
 
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
 import java.util.TimeZone;
 
 /** This class houses handling all JVM arguments by TestNG */
@@ -16,13 +19,20 @@ public final class RuntimeBehavior {
   public static final String TESTNG_DEFAULT_VERBOSE = "testng.default.verbose";
   public static final String IGNORE_CALLBACK_INVOCATION_SKIPS = "testng.ignore.callback.skip";
   public static final String SYMMETRIC_LISTENER_EXECUTION = "testng.listener.execution.symmetric";
+  public static final String IDE_PACKAGE_NAMES = "testng.ide.listeners.package.name";
 
   private RuntimeBehavior() {}
 
   public static boolean ignoreCallbackInvocationSkips() {
     return Boolean.getBoolean(IGNORE_CALLBACK_INVOCATION_SKIPS);
   }
 
+  public static List<String> getIdePackageNames() {
+    String packages =
+        Optional.ofNullable(System.getProperty(IDE_PACKAGE_NAMES)).orElse("com.intellij.rt.*");
+    return Arrays.asList(packages.split(","));
+  }
+
   public static boolean strictParallelism() {
     return Boolean.getBoolean(STRICTLY_HONOUR_PARALLEL_MODE);
   }

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

@@ -69,6 +69,7 @@ public class SuiteRunner implements ISuite, IInvokedMethodListener {
   private final SuiteRunState suiteState = new SuiteRunState();
   private final IAttributes attributes = new Attributes();
   private final Set<IExecutionVisualiser> visualisers = Sets.newHashSet();
+  private ITestListener exitCodeListener;
 
   public SuiteRunner(
       IConfiguration configuration,
@@ -94,7 +95,7 @@ public SuiteRunner(
         useDefaultListeners,
         new ArrayList<>() /* method interceptor */,
         null /* invoked method listeners */,
-        null /* test listeners */,
+        new TestListenersContainer() /* test listeners */,
         null /* class listeners */,
         new DataProviderHolder(),
         comparator);
@@ -108,7 +109,7 @@ protected SuiteRunner(
       boolean useDefaultListeners,
       List<IMethodInterceptor> methodInterceptors,
       Collection<IInvokedMethodListener> invokedMethodListeners,
-      Collection<ITestListener> testListeners,
+      TestListenersContainer container,
       Collection<IClassListener> classListeners,
       DataProviderHolder holder,
       Comparator<ITestNGMethod> comparator) {
@@ -120,7 +121,7 @@ protected SuiteRunner(
         useDefaultListeners,
         methodInterceptors,
         invokedMethodListeners,
-        testListeners,
+        container,
         classListeners,
         holder,
         comparator);
@@ -134,7 +135,7 @@ private void init(
       boolean useDefaultListeners,
       List<IMethodInterceptor> methodInterceptors,
       Collection<IInvokedMethodListener> invokedMethodListener,
-      Collection<ITestListener> testListeners,
+      TestListenersContainer container,
       Collection<IClassListener> classListeners,
       DataProviderHolder attribs,
       Comparator<ITestNGMethod> comparator) {
@@ -146,6 +147,7 @@ private void init(
     this.xmlSuite = suite;
     this.useDefaultListeners = useDefaultListeners;
     this.tmpRunnerFactory = runnerFactory;
+    this.exitCodeListener = container.exitCodeListener;
     List<IMethodInterceptor> localMethodInterceptors =
         Optional.ofNullable(methodInterceptors).orElse(Lists.newArrayList());
     setOutputDir(outputDir);
@@ -206,9 +208,7 @@ public <T> T newInstance(Constructor<T> constructor, Object... parameters) {
     }
 
     skipFailedInvocationCounts = suite.skipFailedInvocationCounts();
-    if (null != testListeners) {
-      this.testListeners.addAll(testListeners);
-    }
+    this.testListeners.addAll(container.listeners);
     for (IClassListener classListener :
         Optional.ofNullable(classListeners).orElse(Collections.emptyList())) {
       this.classListeners.put(classListener.getClass(), classListener);
@@ -257,13 +257,19 @@ public void setReportResults(boolean reportResults) {
     useDefaultListeners = reportResults;
   }
 
+  ITestListener getExitCodeListener() {
+    return exitCodeListener;
+  }
+
   private void invokeListeners(boolean start) {
     if (start) {
-      for (ISuiteListener sl : Lists.newArrayList(listeners.values())) {
+      for (ISuiteListener sl :
+          ListenerOrderDeterminer.order(Lists.newArrayList(listeners.values()))) {
         sl.onStart(this);
       }
     } else {
-      List<ISuiteListener> suiteListenersReversed = Lists.newReversedArrayList(listeners.values());
+      List<ISuiteListener> suiteListenersReversed =
+          ListenerOrderDeterminer.reversedOrder(listeners.values());
       for (ISuiteListener sl : suiteListenersReversed) {
         sl.onFinish(this);
       }
@@ -813,4 +819,19 @@ public List<ITestNGMethod> getAllMethods() {
         .flatMap(tr -> Arrays.stream(tr.getAllTestMethods()))
         .collect(Collectors.toList());
   }
+
+  static class TestListenersContainer {
+    private final List<ITestListener> listeners = Lists.newArrayList();
+    private final ITestListener exitCodeListener;
+
+    TestListenersContainer() {
+      this(Collections.emptyList(), null);
+    }
+
+    TestListenersContainer(List<ITestListener> listeners, ITestListener exitCodeListener) {
+      this.listeners.addAll(listeners);
+      this.exitCodeListener =
+          Objects.requireNonNullElseGet(exitCodeListener, () -> new ITestListener() {});
+    }
+  }
 }

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

@@ -20,6 +20,7 @@
 import java.util.Set;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.TimeUnit;
+import org.testng.SuiteRunner.TestListenersContainer;
 import org.testng.annotations.ITestAnnotation;
 import org.testng.collections.Lists;
 import org.testng.collections.Maps;
@@ -29,6 +30,7 @@
 import org.testng.internal.DynamicGraph;
 import org.testng.internal.ExitCode;
 import org.testng.internal.IConfiguration;
+import org.testng.internal.ListenerOrderDeterminer;
 import org.testng.internal.OverrideProcessor;
 import org.testng.internal.ReporterConfig;
 import org.testng.internal.RuntimeBehavior;
@@ -924,7 +926,6 @@ private void initializeDefaultListeners() {
     if (m_failIfAllTestsSkipped) {
       this.exitCodeListener.failIfAllTestsSkipped();
     }
-    addListener(this.exitCodeListener);
     if (m_useDefaultListeners) {
       addReporter(SuiteHTMLReporter.class);
       addReporter(Main.class);
@@ -1106,17 +1107,22 @@ private void runSuiteAlterationListeners() {
   }
 
   private void runExecutionListeners(boolean start) {
-    List<IExecutionListener> executionListeners = m_configuration.getExecutionListeners();
+    List<IExecutionListener> executionListeners =
+        ListenerOrderDeterminer.order(m_configuration.getExecutionListeners());
     if (start) {
       for (IExecutionListener l : executionListeners) {
         l.onExecutionStart();
       }
+      // Invoke our exit code listener after all the user's listeners have run.
+      exitCodeListener.onExecutionStart();
     } else {
       List<IExecutionListener> executionListenersReversed =
-          Lists.newReversedArrayList(executionListeners);
+          ListenerOrderDeterminer.reversedOrder(executionListeners);
       for (IExecutionListener l : executionListenersReversed) {
         l.onExecutionFinish();
       }
+      // Invoke our exit code listener after all the user's listeners have run.
+      exitCodeListener.onExecutionFinish();
     }
   }
 
@@ -1128,7 +1134,11 @@ private static void usage() {
   }
 
   private void generateReports(List<ISuite> suiteRunners) {
-    for (IReporter reporter : m_reporters.values()) {
+    List<IReporter> reporters = new ArrayList<>(m_reporters.values());
+    // Add our Exit code listener as the last of the reporter so that we can still accommodate
+    // whatever changes were done by a user's reporting listener
+    reporters.add(exitCodeListener);
+    for (IReporter reporter : reporters) {
       try {
         long start = System.currentTimeMillis();
         reporter.generateReport(m_suites, suiteRunners, m_outputDir);
@@ -1334,6 +1344,8 @@ private SuiteRunner createSuiteRunner(XmlSuite xmlSuite) {
     DataProviderHolder holder = new DataProviderHolder();
     holder.addListeners(m_dataProviderListeners.values());
     holder.addInterceptors(m_dataProviderInterceptors.values());
+    TestListenersContainer container =
+        new TestListenersContainer(getTestListeners(), this.exitCodeListener);
     SuiteRunner result =
         new SuiteRunner(
             getConfiguration(),
@@ -1343,7 +1355,7 @@ private SuiteRunner createSuiteRunner(XmlSuite xmlSuite) {
             m_useDefaultListeners,
             m_methodInterceptors,
             m_invokedMethodListeners.values(),
-            m_testListeners.values(),
+            container,
             m_classListeners.values(),
             holder,
             Systematiser.getComparator());

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

@@ -10,6 +10,7 @@
 import java.util.Date;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.LinkedBlockingQueue;
@@ -36,6 +37,7 @@
 import org.testng.internal.IContainer;
 import org.testng.internal.ITestClassConfigInfo;
 import org.testng.internal.ITestResultNotifier;
+import org.testng.internal.ListenerOrderDeterminer;
 import org.testng.internal.MethodGroupsHelper;
 import org.testng.internal.MethodHelper;
 import org.testng.internal.ResultMap;
@@ -118,6 +120,8 @@ public class TestRunner
   // The XML method selector (groups/methods included/excluded in XML)
   private final XmlMethodSelector m_xmlMethodSelector = new XmlMethodSelector();
 
+  private ITestListener exitCodeListener;
+
   //
   // These next fields contain all the configuration methods found on this class.
   // At initialization time, they just contain all the various @Configuration methods
@@ -252,6 +256,13 @@ private void init(
     m_injectorFactory = m_configuration.getInjectorFactory();
     m_objectFactory = suite.getObjectFactory();
     setVerbose(test.getVerbose());
+    if (suiteRunner == null) {
+      if (suite instanceof SuiteRunner) {
+        setExitCodeListener(((SuiteRunner) suite).getExitCodeListener());
+      }
+    } else {
+      setExitCodeListener(suiteRunner.getExitCodeListener());
+    }
 
     boolean preserveOrder = test.getPreserveOrder();
     IMethodInterceptor builtinInterceptor =
@@ -963,15 +974,18 @@ private void logStart() {
    */
   private void fireEvent(boolean isStart) {
     if (isStart) {
-      for (ITestListener itl : m_testListeners) {
+      for (ITestListener itl : ListenerOrderDeterminer.order(m_testListeners)) {
         itl.onStart(this);
       }
+      this.exitCodeListener.onStart(this);
 
     } else {
-      List<ITestListener> testListenersReversed = Lists.newReversedArrayList(m_testListeners);
+      List<ITestListener> testListenersReversed =
+          ListenerOrderDeterminer.reversedOrder(m_testListeners);
       for (ITestListener itl : testListenersReversed) {
         itl.onFinish(this);
       }
+      this.exitCodeListener.onFinish(this);
     }
     if (!isStart) {
       MethodHelper.clear(methods(this.getPassedConfigurations()));
@@ -1238,6 +1252,15 @@ void addConfigurationListener(IConfigurationListener icl) {
     }
   }
 
+  private void setExitCodeListener(ITestListener exitCodeListener) {
+    this.exitCodeListener = exitCodeListener;
+  }
+
+  @Override
+  public ITestListener getExitCodeListener() {
+    return Objects.requireNonNull(exitCodeListener, "ExitCodeListener cannot be null.");
+  }
+
   private void dumpInvokedMethods() {
     MethodHelper.dumpInvokedMethodInfoToConsole(getAllTestMethods(), getVerbose());
   }

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

@@ -0,0 +1,84 @@
+package org.testng.internal;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import org.testng.collections.Lists;
+import org.testng.internal.collections.Pair;
+
+/**
+ * A Utility that helps us differentiate between a user's listener and an IDE Listener.
+ *
+ * <p>When dealing with TestNG listeners we would need to ensure that the user created listeners are
+ * invoked first followed by IDE listeners. This is required so that we always honour any state
+ * changes that a user's listener may have done to the internal state of objects that can affect the
+ * outcome of the execution (for e.g.,{@link org.testng.ITestResult})
+ *
+ * <p>The ordering must be done such that, when dealing with "beforeXXX|afterXXX" we group all the
+ * IDE listeners at the end. That way, we can always ensure that the IDE listeners always honour the
+ * changes that were done to the TestNG internal states and give a consistent experience for users
+ * in the IDE.
+ */
+public final class ListenerOrderDeterminer {
+
+  private ListenerOrderDeterminer() {
+    // Defeat instantiation
+  }
+
+  private static final List<String> IDE_PACKAGES =
+      RuntimeBehavior.getIdePackageNames().stream()
+          .map(each -> each.replaceAll("\\Q.*\\E", ""))
+          .collect(Collectors.toList());
+
+  private static final Predicate<Class<?>> IDE_LISTENER =
+      clazz -> IDE_PACKAGES.stream().anyMatch(each -> clazz.getPackage().getName().contains(each));
+
+  /**
+   * @param original - The original collection of listeners
+   * @return - A re-ordered collection wherein TestNG's known listeners are added at the end
+   */
+  public static <T> List<T> order(Collection<T> original) {
+    Pair<List<T>, List<T>> ordered = arrange(original);
+    List<T> ideListeners = ordered.first();
+    List<T> regularListeners = ordered.second();
+    return Lists.merge(regularListeners, ideListeners);
+  }
+
+  /**
+   * @param original - The original collection of listeners
+   * @return - A reversed ordered list wherein the user listeners are found in reverse order
+   *     followed by TestNG known listeners also in reverse order.
+   */
+  public static <T> List<T> reversedOrder(Collection<T> original) {
+    Pair<List<T>, List<T>> ordered = arrange(original);
+    List<T> ideListeners = ordered.first();
+    List<T> regularListeners = ordered.second();
+    Collections.reverse(regularListeners);
+    Collections.reverse(ideListeners);
+    return Lists.merge(regularListeners, ideListeners);
+  }
+
+  private static boolean isIDEListener(Class<?> clazz) {
+    return IDE_LISTENER.test(clazz);
+  }
+
+  private static <T> Pair<List<T>, List<T>> arrange(Collection<T> original) {
+    List<T> ideListeners = new ArrayList<>();
+    List<T> regularListeners = new ArrayList<>();
+    original.stream()
+        .filter(Objects::nonNull)
+        .forEach(
+            each -> {
+              if (isIDEListener(each.getClass())) {
+                ideListeners.add(each);
+              } else {
+                regularListeners.add(each);
+              }
+            });
+    return new Pair<>(ideListeners, regularListeners);
+  }
+}