Segregate native & user listeners before ordering

krmahadevan · krmahadevan · commit 755cfd4c69c4 · 2023-01-08T09:40:20.000+05:30
Closes #2771
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,5 +1,6 @@
 Current
 Fixed: GITHUB-2796: Option for onAfterClass to run after @AfterClass
+Fixed: GITHUB-2771: After upgrading to TestNG 7.5.0, setting ITestResult.status to FAILURE doesn't fail the test anymore (Krishnan Mahadevan)
 Fixed: GITHUB-2857: XmlTest index is not set for test suites invoked with YAML
 
 7.7.1
diff --git a/testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java b/testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java
@@ -16,9 +16,14 @@ 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 INTELLIJ_PACKAGE_NAME = "testng.intellij.package.name";
 
   private RuntimeBehavior() {}
 
+  public static String getIntellijPackageName() {
+    return System.getProperty(INTELLIJ_PACKAGE_NAME, "com.intellij.rt.testng");
+  }
+
   public static boolean ignoreCallbackInvocationSkips() {
     return Boolean.getBoolean(IGNORE_CALLBACK_INVOCATION_SKIPS);
   }
diff --git a/testng-core/src/main/java/org/testng/SuiteRunner.java b/testng-core/src/main/java/org/testng/SuiteRunner.java
@@ -260,11 +260,12 @@ public void setReportResults(boolean reportResults) {
 
   private void invokeListeners(boolean start) {
     if (start) {
-      for (ISuiteListener sl : Lists.newArrayList(listeners.values())) {
+      for (ISuiteListener sl : ListenerOrderDeterminer.order(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);
       }
diff --git a/testng-core/src/main/java/org/testng/TestNG.java b/testng-core/src/main/java/org/testng/TestNG.java
@@ -29,6 +29,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;
@@ -1106,14 +1107,15 @@ 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();
       }
     } else {
       List<IExecutionListener> executionListenersReversed =
-          Lists.newReversedArrayList(executionListeners);
+          ListenerOrderDeterminer.reversedOrder(executionListeners);
       for (IExecutionListener l : executionListenersReversed) {
         l.onExecutionFinish();
       }
diff --git a/testng-core/src/main/java/org/testng/TestRunner.java b/testng-core/src/main/java/org/testng/TestRunner.java
@@ -36,6 +36,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;
@@ -955,12 +956,13 @@ 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);
       }
 
     } else {
-      List<ITestListener> testListenersReversed = Lists.newReversedArrayList(m_testListeners);
+      List<ITestListener> testListenersReversed =
+          ListenerOrderDeterminer.reversedOrder(m_testListeners);
       for (ITestListener itl : testListenersReversed) {
         itl.onFinish(this);
       }
diff --git a/testng-core/src/main/java/org/testng/internal/ListenerOrderDeterminer.java b/testng-core/src/main/java/org/testng/internal/ListenerOrderDeterminer.java
@@ -0,0 +1,94 @@
+package org.testng.internal;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Predicate;
+import org.testng.collections.Lists;
+
+/**
+ * A Utility that helps us differentiate between a user's listener and TestNG's native listeners(
+ * which also includes IntelliJ IDEA Listener)
+ */
+public final class ListenerOrderDeterminer {
+
+  private ListenerOrderDeterminer() {
+    // Defeat instantiation
+  }
+
+  private static final List<String> KNOWN =
+      Arrays.asList(
+          "org.testng.internal.ExitCodeListener",
+          "org.testng.SuiteRunner",
+          "org.testng.TestRunner.ConfigurationListener",
+          "org.testng.reporters.DotTestListener",
+          "org.testng.reporters.EmailableReporter",
+          "org.testng.reporters.EmailableReporter2",
+          "org.testng.reporters.ExitCodeListener",
+          "org.testng.reporters.FailedReporter",
+          "org.testng.reporters.JUnitReportReporter",
+          "org.testng.reporters.JUnitXMLReporter",
+          "org.testng.reporters.SuiteHTMLReporter",
+          "org.testng.reporters.TestHTMLReporter",
+          "org.testng.reporters.TextReporter",
+          "org.testng.reporters.VerboseReporter",
+          "org.testng.reporters.XMLReporter",
+          "org.testng.reporters.jq.Main");
+
+  private static final Predicate<Class<?>> INTELLIJ_IDE_LISTENER =
+      clazz -> clazz.getName().contains(RuntimeBehavior.getIntellijPackageName());
+
+  /**
+   * @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) {
+    List<T> nativeListeners = new ArrayList<>();
+    List<T> foreignListeners = new ArrayList<>();
+    original.stream()
+        .filter(Objects::nonNull)
+        .forEach(
+            each -> {
+              if (isNativeListener(each.getClass())) {
+                nativeListeners.add(each);
+              } else {
+                foreignListeners.add(each);
+              }
+            });
+    return Lists.merge(foreignListeners, nativeListeners);
+  }
+
+  /**
+   * @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) {
+    List<T> nativeListeners = new ArrayList<>();
+    List<T> foreignListeners = new ArrayList<>();
+    original.stream()
+        .filter(Objects::nonNull)
+        .forEach(
+            each -> {
+              if (isNativeListener(each.getClass())) {
+                nativeListeners.add(each);
+              } else {
+                foreignListeners.add(each);
+              }
+            });
+    Collections.reverse(foreignListeners);
+    Collections.reverse(nativeListeners);
+    return Lists.merge(foreignListeners, nativeListeners);
+  }
+
+  private static boolean isNativeListener(Class<?> clazz) {
+    return KNOWN.stream()
+        .anyMatch(
+            each ->
+                each.equalsIgnoreCase(clazz.getCanonicalName())
+                    || INTELLIJ_IDE_LISTENER.test(clazz));
+  }
+}
diff --git a/testng-core/src/main/java/org/testng/internal/TestListenerHelper.java b/testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
@@ -23,7 +23,7 @@ private TestListenerHelper() {
 
   public static void runPreConfigurationListeners(
       ITestResult tr, ITestNGMethod tm, List<IConfigurationListener> listeners) {
-    for (IConfigurationListener icl : listeners) {
+    for (IConfigurationListener icl : ListenerOrderDeterminer.order(listeners)) {
       icl.beforeConfiguration(tr);
       try {
         icl.beforeConfiguration(tr, tm);
@@ -35,7 +35,8 @@ public static void runPreConfigurationListeners(
 
   public static void runPostConfigurationListeners(
       ITestResult tr, ITestNGMethod tm, List<IConfigurationListener> listeners) {
-    List<IConfigurationListener> listenersreversed = Lists.newReversedArrayList(listeners);
+    List<IConfigurationListener> listenersreversed =
+        ListenerOrderDeterminer.reversedOrder(listeners);
     for (IConfigurationListener icl : listenersreversed) {
       switch (tr.getStatus()) {
         case ITestResult.SKIP:
diff --git a/testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java b/testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java
@@ -10,10 +10,10 @@
 import org.testng.ITestResult;
 import org.testng.SkipException;
 import org.testng.SuiteRunState;
-import org.testng.collections.Lists;
 import org.testng.collections.Maps;
 import org.testng.internal.IConfiguration;
 import org.testng.internal.ITestResultNotifier;
+import org.testng.internal.ListenerOrderDeterminer;
 import org.testng.internal.Utils;
 import org.testng.internal.annotations.IAnnotationFinder;
 
@@ -60,8 +60,8 @@ protected void runInvokedMethodListeners(
     boolean isAfterInvocation = InvokedMethodListenerMethod.AFTER_INVOCATION == listenerMethod;
     Collection<IInvokedMethodListener> listeners =
         isAfterInvocation
-            ? Lists.newReversedArrayList(m_invokedMethodListeners)
-            : m_invokedMethodListeners;
+            ? ListenerOrderDeterminer.reversedOrder(m_invokedMethodListeners)
+            : ListenerOrderDeterminer.order(m_invokedMethodListeners);
     for (IInvokedMethodListener currentListener : listeners) {
       try {
         invoker.invokeListener(currentListener, invokedMethod);
diff --git a/testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java b/testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
@@ -272,8 +272,8 @@ public void runTestResultListener(ITestResult tr) {
     boolean isFinished = tr.getStatus() != ITestResult.STARTED;
     List<ITestListener> listeners =
         isFinished
-            ? Lists.newReversedArrayList(m_notifier.getTestListeners())
-            : m_notifier.getTestListeners();
+            ? ListenerOrderDeterminer.reversedOrder(m_notifier.getTestListeners())
+            : ListenerOrderDeterminer.order(m_notifier.getTestListeners());
     TestListenerHelper.runTestListeners(tr, listeners);
   }
 
diff --git a/testng-core/src/main/java/org/testng/junit/JUnitTestRunner.java b/testng-core/src/main/java/org/testng/junit/JUnitTestRunner.java
@@ -17,6 +17,7 @@
 import org.testng.*;
 import org.testng.collections.Lists;
 import org.testng.internal.ITestResultNotifier;
+import org.testng.internal.ListenerOrderDeterminer;
 import org.testng.internal.TestListenerHelper;
 import org.testng.internal.invokers.InvokedMethod;
 
@@ -94,8 +95,8 @@ public void endTest(Test test) {
     boolean isFinished = tr.getStatus() != ITestResult.STARTED;
     List<ITestListener> listeners =
         isFinished
-            ? Lists.newReversedArrayList(m_parentRunner.getTestListeners())
-            : m_parentRunner.getTestListeners();
+            ? ListenerOrderDeterminer.reversedOrder(m_parentRunner.getTestListeners())
+            : ListenerOrderDeterminer.order(m_parentRunner.getTestListeners());
     TestListenerHelper.runTestListeners(tr, listeners);
   }
 
diff --git a/testng-core/src/test/java/test/listeners/ListenersTest.java b/testng-core/src/test/java/test/listeners/ListenersTest.java
@@ -14,6 +14,7 @@
 import org.testng.TestNG;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
+import org.testng.internal.ExitCode;
 import org.testng.xml.XmlSuite;
 import test.SimpleBaseTest;
 import test.listeners.issue2638.DummyInvokedMethodListener;
@@ -23,6 +24,7 @@
 import test.listeners.issue2685.SampleTestFailureListener;
 import test.listeners.issue2752.ListenerSample;
 import test.listeners.issue2752.TestClassSample;
+import test.listeners.issue2771.TestCaseSample;
 
 public class ListenersTest extends SimpleBaseTest {
 
@@ -92,6 +94,13 @@ public void testWiringInOfListenersInMultipleTestTagsWithListenerInSuite() {
     assertThat(logs.get("Xml_Test_2")).containsAll(expected);
   }
 
+  @Test(description = "GITHUB-2771")
+  public void testEnsureNativeListenersAreRunAlwaysAtEnd() {
+    TestNG testng = create(TestCaseSample.class);
+    testng.run();
+    assertThat(testng.getStatus()).isEqualTo(ExitCode.FAILED);
+  }
+
   private void setupTest(boolean addExplicitListener) {
     TestNG testng = new TestNG();
     XmlSuite xmlSuite = createXmlSuite("Xml_Suite");
diff --git a/testng-core/src/test/java/test/listeners/issue2771/CustomSoftAssert.java b/testng-core/src/test/java/test/listeners/issue2771/CustomSoftAssert.java
@@ -0,0 +1,12 @@
+package test.listeners.issue2771;
+
+import org.testng.ITestResult;
+import org.testng.reporters.ExitCodeListener;
+
+public class CustomSoftAssert extends ExitCodeListener {
+  @Override
+  public void onTestSuccess(ITestResult result) {
+    result.setStatus(ITestResult.FAILURE);
+    result.setThrowable(new AssertionError("There have been some failed soft asserts"));
+  }
+}
diff --git a/testng-core/src/test/java/test/listeners/issue2771/TestCaseSample.java b/testng-core/src/test/java/test/listeners/issue2771/TestCaseSample.java
@@ -0,0 +1,13 @@
+package test.listeners.issue2771;
+
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+@Listeners(CustomSoftAssert.class)
+public class TestCaseSample {
+  @Test
+  public void someCustomSoftAsserts() {
+    // doing custom soft asserts
+    // which will trigger test failure in listener
+  }
+}

Original file line number	Diff line number	Diff line change
`@@ -260,11 +260,12 @@ public void setReportResults(boolean reportResults) {`
`260`	`260`
`261`	`261`	`private void invokeListeners(boolean start) {`
`262`	`262`	`if (start) {`
`263`		`- for (ISuiteListener sl : Lists.newArrayList(listeners.values())) {`
	`263`	`+ for (ISuiteListener sl : ListenerOrderDeterminer.order(listeners.values())) {`
`264`	`264`	`sl.onStart(this);`
`265`	`265`	`}`
`266`	`266`	`} else {`
`267`		`- List<ISuiteListener> suiteListenersReversed = Lists.newReversedArrayList(listeners.values());`
	`267`	`+ List<ISuiteListener> suiteListenersReversed =`
	`268`	`+ ListenerOrderDeterminer.reversedOrder(listeners.values());`
`268`	`269`	`for (ISuiteListener sl : suiteListenersReversed) {`
`269`	`270`	`sl.onFinish(this);`
`270`	`271`	`}`