Streamline listener invocation handling exceptions

Closes #3238

Author: krmahadevan

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.13.0)
+Fixed: GITHUB-3238: Tests never finish if helper throws exception while executing parallel tests ( Krishnan Mahadevan )
 Fixed: GITHUB-3120: ITestNGListenerFactory is broken and never invoked (Krishnan Mahadevan)
 
 7.12.0

testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java

@@ -62,6 +62,12 @@ class TestInvoker extends BaseInvoker implements ITestInvoker {
   private final List<IClassListener> m_classListeners;
   private final boolean m_skipFailedInvocationCounts;
 
+  // ThreadLocal to track if we're currently inside runTestResultListener()
+  // Used to prevent duplicate listener invocations when a listener throws an exception.
+  // See GITHUB-3238.
+  private static final ThreadLocal<Boolean> isInvokingListeners =
+      ThreadLocal.withInitial(() -> false);
+
   public TestInvoker(
       ITestResultNotifier m_notifier,
       ITestContext m_testContext,
@@ -293,9 +299,13 @@ public void runTestResultListener(ITestResult tr) {
                 m_notifier.getTestListeners(), m_configuration.getListenerComparator())
             : ListenerOrderDeterminer.order(
                 m_notifier.getTestListeners(), m_configuration.getListenerComparator());
+    isInvokingListeners.set(true);
     TestListenerHelper.runTestListeners(tr, listeners);
     TestListenerHelper.runTestListeners(
         tr, Collections.singletonList(m_notifier.getExitCodeListener()));
+    // Only reset the flag on successful completion. If an exception is thrown,
+    // we want the flag to remain true so the catch block can detect it.
+    isInvokingListeners.set(false);
   }
 
   private Collection<IDataProviderListener> dataProviderListeners() {
@@ -1020,7 +1030,16 @@ public int invoke(int invCount) {
             .ifPresent(it -> TestResult.copyAttributes(it, r));
         r.setStatus(TestResult.FAILURE);
         result.add(r);
-        runTestResultListener(r);
+        // Only invoke listeners if the exception didn't come from a listener invocation.
+        // If we're inside a listener invocation, the exception came from a listener,
+        // so don't invoke listeners again to prevent duplicate invocations.
+        // See GITHUB-3238.
+        if (!isInvokingListeners.get()) {
+          runTestResultListener(r);
+        } else {
+          // Reset the flag since we're handling the listener exception here
+          isInvokingListeners.set(false);
+        }
         m_notifier.addFailedTest(arguments.getTestMethod(), r);
       } // catch
       return invocationCount.get();

testng-core/src/main/java/org/testng/internal/thread/graph/TestNGFutureTask.java

@@ -31,7 +31,7 @@ protected void done() {
     } catch (InterruptedException | ExecutionException e) {
       throwable = e;
     }
-    callback.accept(result, throwable);
+    callback.accept(result == null ? worker : result, throwable);
   }
 
   @Override

testng-core/src/test/java/test/listeners/ListenersTest.java

@@ -10,15 +10,19 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.assertj.core.api.Assertions;
 import org.assertj.core.api.SoftAssertions;
 import org.testng.CommandLineArgs;
+import org.testng.ITestListener;
 import org.testng.ITestNGListener;
+import org.testng.ITestResult;
 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 org.testng.xml.XmlSuite.ParallelMode;
 import test.SimpleBaseTest;
 import test.listeners.issue2381.FactoryTestClassSample;
 import test.listeners.issue2381.SampleGlobalListener;
@@ -56,6 +60,8 @@
 import test.listeners.issue3082.ObjectRepository;
 import test.listeners.issue3082.ObjectTrackingMethodListener;
 import test.listeners.issue3095.ChildClassSample;
+import test.listeners.issue3238.TestClassWithFailingTestMethodSample;
+import test.listeners.issue3238.TestClassWithPassingTestMethodSample;
 
 public class ListenersTest extends SimpleBaseTest {
   public static final String[] github2638ExpectedList =
@@ -685,6 +691,31 @@ public void ensureInheritanceIsHandledWhenDealingWithListeners() {
     assertThat(testng.getStatus()).isZero();
   }
 
+  @Test(description = "GITHUB-3238")
+  public void ensureListenerFailureDoesNotBreakTestNG() {
+    TestNG testng =
+        create(
+            TestClassWithFailingTestMethodSample.class, TestClassWithPassingTestMethodSample.class);
+    testng.setParallel(ParallelMode.CLASSES);
+    AtomicInteger failingCount = new AtomicInteger(0);
+    AtomicInteger passingCount = new AtomicInteger(0);
+    testng.addListener(
+        new ITestListener() {
+          @Override
+          public void onTestFailure(ITestResult result) {
+            failingCount.incrementAndGet();
+          }
+
+          @Override
+          public void onTestSuccess(ITestResult result) {
+            passingCount.incrementAndGet();
+          }
+        });
+    testng.run();
+    assertThat(failingCount.get()).isEqualTo(1);
+    assertThat(passingCount.get()).isEqualTo(1);
+  }
+
   private void setupTest(boolean addExplicitListener) {
     TestNG testng = new TestNG();
     XmlSuite xmlSuite = createXmlSuite("Xml_Suite");

testng-core/src/test/java/test/listeners/issue3238/BadUtility.java

@@ -0,0 +1,12 @@
+package test.listeners.issue3238;
+
+public class BadUtility {
+
+  private static final int counter = evaluate();
+
+  private static int evaluate() {
+    throw new RuntimeException("Failed on purpose");
+  }
+
+  public static void doNothing() {}
+}

testng-core/src/test/java/test/listeners/issue3238/FailureTrackingListener.java

@@ -0,0 +1,12 @@
+package test.listeners.issue3238;
+
+import org.testng.ITestListener;
+import org.testng.ITestResult;
+
+public class FailureTrackingListener implements ITestListener {
+
+  @Override
+  public void onTestFailure(ITestResult result) {
+    BadUtility.doNothing();
+  }
+}

testng-core/src/test/java/test/listeners/issue3238/TestClassWithFailingTestMethodSample.java

@@ -0,0 +1,15 @@
+package test.listeners.issue3238;
+
+import org.testng.Assert;
+import org.testng.annotations.Listeners;
+import org.testng.annotations.Test;
+
+@Listeners(FailureTrackingListener.class)
+public class TestClassWithFailingTestMethodSample {
+
+  @Test
+  public void failingTest() {
+    System.err.println(":::::");
+    Assert.fail();
+  }
+}

testng-core/src/test/java/test/listeners/issue3238/TestClassWithPassingTestMethodSample.java

@@ -0,0 +1,8 @@
+package test.listeners.issue3238;
+
+import org.testng.annotations.Test;
+
+public class TestClassWithPassingTestMethodSample {
+  @Test
+  public void testMethod() {}
+}