Skip to content

Flaky Test Fix - 2587 #3268

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

Merged
krmahadevan merged 2 commits into from
Apr 26, 2026
+81 −27
Merged

Flaky Test Fix - 2587 #3268

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
17b45be
Flaky Test Fix - 2587
krmahadevan Apr 25, 2026
e30a8bf
Fixing review comments
krmahadevan Apr 26, 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
73 changes: 58 additions & 15 deletions testng-core/src/test/java/test/factory/issue326/IssueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

import static org.assertj.core.api.Assertions.assertThat;

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.testng.TestNG;
import org.testng.annotations.Test;
import org.testng.xml.XmlSuite.ParallelMode;
Expand All @@ -23,32 +25,73 @@ public void testToCheckParallelExecutionOfInstancesWithOneThreadPerInstance() {
testng.addListener(listener);
testng.run();

// Ensure that the difference in start times for the methods in both instances is <= 1000 ms.
long diff = computeDiffInStartTimeFor(listener.getResults(), SampleTestClass.FREDDY);
assertThat(diff).isLessThanOrEqualTo(1000);
diff = computeDiffInStartTimeFor(listener.getResults(), SampleTestClass.BARNEY);
assertThat(diff).isLessThanOrEqualTo(1000);
Map<String, List<Statistics>> results = listener.getResults();
Comment thread
krmahadevan marked this conversation as resolved.

// Ensure that the thread ids for both the instances are different.
// 1. We should be having only 2 instances (factory gets only 2 iterations via dataprovider)
assertThat(results).containsOnlyKeys(SampleTestClass.FREDDY, SampleTestClass.BARNEY);
// 2. Every instance has 2 method invocations
assertThat(results.get(SampleTestClass.FREDDY)).hasSize(2);
assertThat(results.get(SampleTestClass.BARNEY)).hasSize(2);

// 3. The thread id for the methods that belong to each instance is the same.
assertThat(threadIdsFor(results, SampleTestClass.FREDDY)).hasSize(1);
assertThat(threadIdsFor(results, SampleTestClass.BARNEY)).hasSize(1);

// 4. Check to ensure that the thread id on which methods belonging to first instance ran
// is not the same as that of the methods that belong to second instance.
long threadIdFreddyInstance = listener.getThreadIds().get(SampleTestClass.FREDDY);
long threadIdBarneyInstance = listener.getThreadIds().get(SampleTestClass.BARNEY);

assertThat(threadIdFreddyInstance).isNotEqualTo(threadIdBarneyInstance);

// Illustration:
//
// Time --->
// Freddy: |-------------|
// 100ms 900ms
//
// Barney: |-------------|
// 300ms 1100ms
//
// Check:
//
// Freddy starts before Barney ends: 100 < 1100 true
// Barney starts before Freddy ends: 300 < 900 true
//
// So the windows overlap,
// meaning TestNG did not run Freddy completely first and Barney completely second.
assertThat(executionWindowsOverlap(results, SampleTestClass.FREDDY, SampleTestClass.BARNEY))
.isTrue();
}

private static long computeDiffInStartTimeFor(
private static Collection<Long> threadIdsFor(
Map<String, List<Statistics>> allStats, String instanceName) {
long test1OnFreddyInstance = getStartTimeFrom(allStats, instanceName, "test1");
long test2OnFreddyInstance = getStartTimeFrom(allStats, instanceName, "test2");
return Math.abs(test2OnFreddyInstance - test1OnFreddyInstance);
return allStats.get(instanceName).stream()
.map(statistics -> statistics.threadId)
.distinct()
.collect(Collectors.toList());
}

private static boolean executionWindowsOverlap(
Map<String, List<Statistics>> allStats, String firstInstanceName, String secondInstanceName) {
long firstStartTime = getStartTimeFrom(allStats, firstInstanceName);
long firstEndTime = getEndTimeFrom(allStats, firstInstanceName);
long secondStartTime = getStartTimeFrom(allStats, secondInstanceName);
long secondEndTime = getEndTimeFrom(allStats, secondInstanceName);
return firstStartTime < secondEndTime && secondStartTime < firstEndTime;
}

private static long getStartTimeFrom(
Map<String, List<Statistics>> allStats, String instanceName, String methodName) {
Map<String, List<Statistics>> allStats, String instanceName) {
return allStats.get(instanceName).stream()
.mapToLong(statistics -> statistics.startTimeInMs)
.min()
.orElseThrow(IllegalStateException::new);
}

private static long getEndTimeFrom(Map<String, List<Statistics>> allStats, String instanceName) {
return allStats.get(instanceName).stream()
.filter(statistics -> statistics.methodName.equals(methodName))
.findFirst()
.map(statistics -> statistics.startTimeInMs)
.mapToLong(statistics -> statistics.endTimeInMs)
.max()
.orElseThrow(IllegalStateException::new);
}
}
35 changes: 23 additions & 12 deletions testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,24 @@

import java.util.List;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.CopyOnWriteArrayList;
import org.testng.IInvokedMethod;
import org.testng.IInvokedMethodListener;
import org.testng.ITestResult;
import org.testng.collections.Lists;
import org.testng.collections.Maps;

public class LocalTrackingListener implements IInvokedMethodListener {

private final Map<String, List<Statistics>> results = Maps.newConcurrentMap();
private final Map<String, Long> threadIds = Maps.newConcurrentMap();
private final ConcurrentMap<String, List<Statistics>> results = new ConcurrentHashMap<>();
private final ConcurrentMap<String, Long> threadIds = new ConcurrentHashMap<>();

@Override
public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
String key = testResult.getInstance().toString();
results.computeIfAbsent(key, k -> Lists.newArrayList()).add(new Statistics(testResult));
if (!threadIds.containsKey(key)) {
Long threadId = Long.parseLong(testResult.getAttribute(SampleTestClass.THREAD_ID).toString());
threadIds.put(key, threadId);
}
results.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(new Statistics(testResult));
Long threadId = asLong(testResult);
threadIds.putIfAbsent(key, threadId);
}
Comment on lines 18 to 23

@coderabbitai coderabbitai Bot Apr 26, 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 | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm SampleTestClass currently has no configuration methods, so the NPE is latent rather than active.
fd -t f 'SampleTestClass.java' | xargs rg -nP '@(Before|After)(Method|Class|Suite|Test|Groups)\b'

Repository: testng-team/testng

Length of output: 458

🏁 Script executed:

# List files in issue326 directory to understand the test structure
fd -t f . testng-core/src/test/java/test/factory/issue326/

Repository: testng-team/testng

Length of output: 269

🏁 Script executed:

# Search for SampleTestClass specifically in issue326 directory
fd -t f 'SampleTestClass.java' testng-core/src/test/java/test/factory/issue326/

Repository: testng-team/testng

Length of output: 131

🏁 Script executed:

# Check the full LocalTrackingListener.java file to see the asLong implementation
cat -n testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java

Repository: testng-team/testng

Length of output: 2261

🏁 Script executed:

cat -n testng-core/src/test/java/test/factory/issue326/SampleTestClass.java

Repository: testng-team/testng

Length of output: 1763

🏁 Script executed:

# Check IInvokedMethod to see if isTestMethod() method exists
rg -t java 'isTestMethod' --type-list

Repository: testng-team/testng

Length of output: 452

🏁 Script executed:

# More specific search for IInvokedMethod interface
rg -A 10 'interface IInvokedMethod' testng-core/src/main/java/

Repository: testng-team/testng

Length of output: 44

🏁 Script executed:

# Search for IInvokedMethod interface definition
fd -t f 'IInvokedMethod.java'

Repository: testng-team/testng

Length of output: 123

🏁 Script executed:

# Check if isTestMethod exists in the codebase
rg -t java 'isTestMethod'

Repository: testng-team/testng

Length of output: 4353

🏁 Script executed:

# View the IInvokedMethod interface to understand isTestMethod
cat -n testng-core-api/src/main/java/org/testng/IInvokedMethod.java

Repository: testng-team/testng

Length of output: 771

Latent NPE risk in asLong when afterInvocation is called for configuration methods.

afterInvocation fires for both test methods and configuration methods. Currently, SampleTestClass.printer() sets THREAD_ID only in test methods (test1, test2). If a configuration method (@BeforeMethod, @AfterMethod, etc.) is added later, afterInvocation will invoke asLong, which casts getAttribute(THREAD_ID) without a null check. Since configuration methods never call printer(), the attribute will be absent, causing NullPointerException or ClassCastException.

Guard against this by returning early for non-test methods:

Proposed fix 
  `@Override`
  public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
+   if (!method.isTestMethod()) {
+     return;
+   }
    String key = testResult.getInstance().toString();
    results.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(new Statistics(testResult));
    Long threadId = asLong(testResult);
    threadIds.putIfAbsent(key, threadId);
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
String key = testResult.getInstance().toString();
results.computeIfAbsent(key, k -> Lists.newArrayList()).add(new Statistics(testResult));
if (!threadIds.containsKey(key)) {
Long threadId = Long.parseLong(testResult.getAttribute(SampleTestClass.THREAD_ID).toString());
threadIds.put(key, threadId);
}
results.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(new Statistics(testResult));
Long threadId = asLong(testResult);
threadIds.putIfAbsent(key, threadId);
}
public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
if (!method.isTestMethod()) {
return;
}
String key = testResult.getInstance().toString();
results.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(new Statistics(testResult));
Long threadId = asLong(testResult);
threadIds.putIfAbsent(key, threadId);
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java`
around lines 18 - 23, afterInvocation currently calls asLong for every
invocation (including configuration methods) which can NPE because THREAD_ID is
only set by SampleTestClass.printer(); update afterInvocation to skip non-test
invocations by checking the invoked method type (use
IInvokedMethod.isTestMethod() or equivalent) and return early for configuration
methods so you only call results.computeIfAbsent(...).add(...) and asLong when
method.isTestMethod() is true.

@krmahadevan krmahadevan Apr 26, 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.

While this is a valid concern in the general context, its not relevant here because test.factory.issue326.SampleTestClass#SampleTestClass does not have any configuration methods.

@coderabbitai coderabbitai Bot Apr 26, 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, that's a fair point — since test.factory.issue326.SampleTestClass is a tightly scoped test fixture with no configuration methods, the risk is indeed not applicable here. I'll note this for future reviews.

(╯°□°）╯

✏️ Learnings added 
Learnt from: krmahadevan
Repo: testng-team/testng PR: 3268
File: testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java:18-23
Timestamp: 2026-04-26T11:56:08.426Z
Learning: In `testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java`, the `afterInvocation` method does not need a guard for configuration methods (e.g., `method.isTestMethod()` check) because the associated test fixture `test.factory.issue326.SampleTestClass` is a tightly scoped test class with no `BeforeMethod`/`AfterMethod`/`BeforeClass`/`AfterClass` configuration methods. The `THREAD_ID` attribute is always set by `SampleTestClass.printer()` for all invocations in this context.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: krmahadevan
Repo: testng-team/testng PR: 3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-09-30T05:21:21.304Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.

Learnt from: krmahadevan
Repo: testng-team/testng PR: 3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-10-09T09:04:04.750Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.


public Map<String, List<Statistics>> getResults() {
Expand All @@ -31,17 +30,29 @@ public Map<String, Long> getThreadIds() {
return threadIds;
}

static class Statistics {
public static class Statistics {
String methodName;
long startTimeInMs;
long endTimeInMs;
long threadId;

public Statistics(ITestResult testResult) {
this(testResult.getMethod().getMethodName(), testResult.getStartMillis());
this(
testResult.getMethod().getMethodName(),
testResult.getStartMillis(),
testResult.getEndMillis(),
asLong(testResult));
}

public Statistics(String methodName, long startTimeInMs) {
public Statistics(String methodName, long startTimeInMs, long endTimeInMs, long threadId) {
this.methodName = methodName;
this.startTimeInMs = startTimeInMs;
this.endTimeInMs = endTimeInMs;
this.threadId = threadId;
}
}

private static long asLong(ITestResult itr) {
return ((Number) itr.getAttribute(SampleTestClass.THREAD_ID)).longValue();
}
}
Loading