Skip to content

Streamline listener invocation handling exceptions #3258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
krmahadevan wants to merge 2 commits into
base: master
Choose a base branch
from
+124 −7
Open

Streamline listener invocation handling exceptions #3258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2466655
Streamline listener invocation handling exceptions
krmahadevan Feb 4, 2026
1b0949b
Fixing review comments
krmahadevan Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
19 changes: 18 additions & 1 deletion testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -1020,7 +1030,14 @@ 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);
}
Comment on lines +1033 to +1039
Copy link

@coderabbitai coderabbitai bot Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Consider logging or propagating the listener exception rather than silently skipping.

Per the reviewer's concern, listeners are expected not to throw exceptions, but when they do, the exception should cause the run to fail rather than being silently handled. The current approach skips the listener re-invocation and resets the flag, but the original exception context may be lost.

Consider logging the exception at minimum, or re-throwing it after cleanup to ensure the failure is visible:

💡 Suggested improvement to log or propagate the exception 
       } catch (Throwable cause) {
         ITestResult r =
             TestResult.newEndTimeAwareTestResult(
                 arguments.getTestMethod(), m_testContext, cause, start);
         Optional.ofNullable(Reporter.getCurrentTestResult())
             .ifPresent(it -> TestResult.copyAttributes(it, r));
         r.setStatus(TestResult.FAILURE);
         result.add(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);
+          // Log warning that a listener threw an exception
+          log(1, "Listener threw an exception: " + cause.getMessage());
         }
         m_notifier.addFailedTest(arguments.getTestMethod(), r);
       } // catch
🤖 Prompt for AI Agents 
In `@testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java`
around lines 1033 - 1042, The current block in TestInvoker that skips
re-invoking listeners when isInvokingListeners.get() is true should not silently
swallow the original listener exception; update the handler around
isInvokingListeners/runTestResultListener to capture the listener Throwable,
reset isInvokingListeners, then either log the Throwable using the TestNG
logging facility (e.g., Reporter or the framework logger) with context (test
result r) and rethrow it as a RuntimeException or TestNGException so the run
fails visibly; ensure this change is applied in the same method in TestInvoker
so the original exception context is preserved and reported.

Copy link
Member Author

@krmahadevan krmahadevan Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are only ensuring that we dont invoke the listener twice during failures (wherein a failure could have happened because of the test method or because of the listener itself). There are unit tests added as part of this PR that validate the usecases and ensures that the expected behavior is ascertained.

Copy link

@coderabbitai coderabbitai bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krmahadevan, thank you for the clarification. You're right—I can see now that the exception is properly captured in the ITestResult object and recorded via m_notifier.addFailedTest(arguments.getTestMethod(), r), so the failure is not being lost. The code is specifically preventing duplicate listener invocations (which could lead to infinite recursion or double reporting), while still preserving the original exception context in the test result.

The test case ensureListenerFailureDoesNotBreakTestNG with FailureTrackingListener and BadUtility validates this scenario effectively. The approach ensures that when a listener throws during parallel execution, the run completes properly without hanging, and the failure is recorded—which aligns with the fix objectives for issue #3238.

My concern about "silently handling" was addressed by your explanation. The failure is indeed being reported, just not re-notified to listeners to avoid the problematic double invocation.

isInvokingListeners.set(false);
m_notifier.addFailedTest(arguments.getTestMethod(), r);
} // catch
return invocationCount.get();
Expand Down
4 changes: 3 additions & 1 deletion testng-core/src/main/java/org/testng/internal/thread/graph/GraphOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.*;
import org.testng.IDynamicGraph;
import org.testng.collections.Maps;
Expand Down Expand Up @@ -73,7 +74,8 @@ private void mapNodeToParent(List<T> freeNodes) {
}
}

private void afterExecute(IWorker<T> r, Throwable t) {
private void afterExecute(IWorker<T> original, IWorker<T> result) {
IWorker<T> r = Optional.ofNullable(result).orElse(original);
Copy link
Member

@juherr juherr Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should be done in case of error? Why do we hide it and use original worker?

Copy link
Member

@juherr juherr Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original could be captured L59

try (AutoCloseableLock ignore = internalLock.lock()) {
setStatus(r, computeStatus(r));
if (graph.getNodeCount() == graph.getNodeCountWithStatus(IDynamicGraph.Status.FINISHED)) {
Expand Down
9 changes: 4 additions & 5 deletions testng-core/src/main/java/org/testng/internal/thread/graph/TestNGFutureTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
public class TestNGFutureTask<T> extends FutureTask<IWorker<T>> implements IWorker<T> {

private final IWorker<T> worker;
private final BiConsumer<IWorker<T>, Throwable> callback;
private final BiConsumer<IWorker<T>, IWorker<T>> callback;
Copy link
Member

@juherr juherr Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callback pattern is result or error, not result or fallback.


public TestNGFutureTask(IWorker<T> worker, BiConsumer<IWorker<T>, Throwable> callback) {
public TestNGFutureTask(IWorker<T> worker, BiConsumer<IWorker<T>, IWorker<T>> callback) {
super(worker, worker);
this.callback = callback;
this.worker = worker;
Expand All @@ -24,14 +24,13 @@ public void run() {

@Override
protected void done() {
Throwable throwable = null;
IWorker<T> result = null;
try {
result = super.get();
} catch (InterruptedException | ExecutionException e) {
throwable = e;
// Gobble exception and do nothing.
}
callback.accept(result, throwable);
callback.accept(worker, result);
}

@Override
Expand Down
40 changes: 40 additions & 0 deletions testng-core/src/test/java/test/listeners/ListenersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -685,6 +691,40 @@ public void ensureInheritanceIsHandledWhenDealingWithListeners() {
assertThat(testng.getStatus()).isZero();
}

@Test(description = "GITHUB-3238", dataProvider = "dp-3238")
public void ensureListenerFailureDoesNotBreakTestNG(ParallelMode parallelMode) {
XmlSuite xmlSuite =
createXmlSuite(
"3238_suite",
"3238_test",
TestClassWithFailingTestMethodSample.class,
TestClassWithPassingTestMethodSample.class);
xmlSuite.setParallel(parallelMode);
TestNG testng = create(xmlSuite);
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(5);
}

@DataProvider(name = "dp-3238")
public Object[][] getTestData() {
return new Object[][] {{ParallelMode.CLASSES}, {ParallelMode.METHODS}};
}

private void setupTest(boolean addExplicitListener) {
TestNG testng = new TestNG();
XmlSuite xmlSuite = createXmlSuite("Xml_Suite");
Expand Down
12 changes: 12 additions & 0 deletions testng-core/src/test/java/test/listeners/issue3238/BadUtility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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() {}
}
12 changes: 12 additions & 0 deletions testng-core/src/test/java/test/listeners/issue3238/FailureTrackingListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
}
}
14 changes: 14 additions & 0 deletions testng-core/src/test/java/test/listeners/issue3238/TestClassWithFailingTestMethodSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
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() {
Assert.fail();
}
}
20 changes: 20 additions & 0 deletions testng-core/src/test/java/test/listeners/issue3238/TestClassWithPassingTestMethodSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package test.listeners.issue3238;

import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

public class TestClassWithPassingTestMethodSample {
@Test
public void testMethod() {}

@Test
public void anotherTestMethod() {}

@Test(dataProvider = "dp")
public void dataDrivenTest(int ignored) {}

@DataProvider(name = "dp", parallel = true)
public Object[][] dp() {
return new Object[][] {{1}, {2}, {3}};
}
}
Loading