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
Open

Streamline listener invocation handling exceptions #3258

Show file tree
Hide file tree
Changes from 1 commit
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
21 changes: 20 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,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);

@juherr juherr Feb 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not always set to false after calling listener?

@krmahadevan krmahadevan Feb 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

}
Comment on lines +1033 to +1039

@coderabbitai coderabbitai Bot Feb 4, 2026 •
edited
Loading

Copy link
Copy Markdown

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.

@krmahadevan krmahadevan Feb 5, 2026

Copy link
Copy Markdown
Member Author

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.

@coderabbitai coderabbitai Bot Feb 5, 2026

Copy link
Copy Markdown

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.

m_notifier.addFailedTest(arguments.getTestMethod(), r);
} // catch
return invocationCount.get();
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -31,7 +31,7 @@ protected void done() {
} catch (InterruptedException | ExecutionException e) {
throwable = e;
}
callback.accept(result, throwable);
callback.accept(result == null ? worker : result, throwable);

@juherr juherr Feb 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

result is null when throwable is not null.
The null value should be managed by the callback.

@krmahadevan krmahadevan Feb 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed

}

@Override
Expand Down
31 changes: 31 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,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");
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();
}
}
15 changes: 15 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,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();
}
}
8 changes: 8 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,8 @@
package test.listeners.issue3238;

import org.testng.annotations.Test;

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