Flaky Test Fix - 2587#3268
Conversation
📝 WalkthroughWalkthroughTest refactor: assertions now verify per-instance execution windows overlap and per-instance single-thread usage. Listener refactor: replace TestNG concurrent helpers with JDK concurrent collections and expand Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java (1)
21-22: Avoid String round-trip when reading the thread id; same code is duplicated inStatistics.
SampleTestClass.printer()stores the attribute as aLong(Thread.currentThread().getId()), so converting toStringand re-parsing is unnecessary work and adds an avoidable failure mode (thetoString()will NPE if the attribute is ever absent, with no useful message). The exact same expression is also repeated at Line 44 in theStatistics(ITestResult)constructor — extracting a tiny helper removes the duplication and keeps both call sites consistent.♻️ Suggested refactor
`@Override` public void afterInvocation(IInvokedMethod method, ITestResult testResult) { String key = testResult.getInstance().toString(); results.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(new Statistics(testResult)); - Long threadId = Long.parseLong(testResult.getAttribute(SampleTestClass.THREAD_ID).toString()); - threadIds.putIfAbsent(key, threadId); + threadIds.putIfAbsent(key, threadIdOf(testResult)); } @@ public Statistics(ITestResult testResult) { this( testResult.getMethod().getMethodName(), testResult.getStartMillis(), testResult.getEndMillis(), - Long.parseLong(testResult.getAttribute(SampleTestClass.THREAD_ID).toString())); + threadIdOf(testResult)); } + + private static long threadIdOf(ITestResult testResult) { + return ((Number) testResult.getAttribute(SampleTestClass.THREAD_ID)).longValue(); + }🤖 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 21 - 22, Both LocalTrackingListener and the Statistics(ITestResult) constructor are converting the THREAD_ID attribute to String and reparsing it; instead read the attribute as a Long directly, null-check it, and avoid the toString()/parseLong() round-trip. Add a small helper (e.g., getThreadId(ITestResult) or a static utility) that returns a Long by fetching testResult.getAttribute(SampleTestClass.THREAD_ID), validating non-null, and returning the Long; replace the duplicated parsing logic in LocalTrackingListener (where threadIds.putIfAbsent(key, threadId) is used) and in the Statistics(ITestResult) constructor with calls to that helper so both sites use the same safe, direct Long extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java`:
- Around line 21-22: Both LocalTrackingListener and the Statistics(ITestResult)
constructor are converting the THREAD_ID attribute to String and reparsing it;
instead read the attribute as a Long directly, null-check it, and avoid the
toString()/parseLong() round-trip. Add a small helper (e.g.,
getThreadId(ITestResult) or a static utility) that returns a Long by fetching
testResult.getAttribute(SampleTestClass.THREAD_ID), validating non-null, and
returning the Long; replace the duplicated parsing logic in
LocalTrackingListener (where threadIds.putIfAbsent(key, threadId) is used) and
in the Statistics(ITestResult) constructor with calls to that helper so both
sites use the same safe, direct Long extraction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dae0ff7-9bbd-45b7-851c-e336667f1888
📒 Files selected for processing (2)
testng-core/src/test/java/test/factory/issue326/IssueTest.javatestng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java
|
The Nitpick comment looks good |
Closes testng-team#2587 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. Non-overlapping sequential execution would look like this: Time ---> Freddy: |-------------| 100ms 900ms Barney: |-------------| 950ms 1600ms Check: Freddy starts before Barney ends: 100 < 1600 true Barney starts before Freddy ends: 950 < 900 false So the assertion would fail.
krmahadevan
force-pushed
the
bugfix/fix_2587
branch
from
9ffa08a to
ebe472f
Compare
Fixed |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java (1)
21-22: Unify thread‑id extraction betweenafterInvocationandStatistics(ITestResult).The same attribute is read two different ways:
((Number) attr).longValue()on line 21 vs.Long.parseLong(attr.toString())on line 44. ThetoString()+parseLonground‑trip is unnecessary, slightly fragile (e.g., would fail forDouble/Floatrepresentations like"1.0E1"), and inconsistent with the cast used a few lines above. Prefer theNumbercast in both places (or pass the already‑extractedthreadIdinto the constructor).♻️ Suggested change
`@Override` public void afterInvocation(IInvokedMethod method, ITestResult testResult) { String key = testResult.getInstance().toString(); results.computeIfAbsent(key, k -> new CopyOnWriteArrayList<>()).add(new Statistics(testResult)); Long threadId = ((Number) testResult.getAttribute(SampleTestClass.THREAD_ID)).longValue(); threadIds.putIfAbsent(key, threadId); } @@ public Statistics(ITestResult testResult) { this( testResult.getMethod().getMethodName(), testResult.getStartMillis(), testResult.getEndMillis(), - Long.parseLong(testResult.getAttribute(SampleTestClass.THREAD_ID).toString())); + ((Number) testResult.getAttribute(SampleTestClass.THREAD_ID)).longValue()); }Also applies to: 39-45
🤖 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 21 - 22, The thread-id extraction is inconsistent: change the code that currently uses Long.parseLong(attr.toString()) (in the Statistics(ITestResult) path) to use the same Number cast pattern as in afterInvocation — i.e., cast the attribute to Number and call longValue() — or alternatively modify the caller to pass the already-extracted threadId into the Statistics constructor; update the Statistics(ITestResult) constructor (or its caller) to use ((Number) attr).longValue() instead of toString()/Long.parseLong to avoid fragile parsing and unify behavior with afterInvocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java`:
- Around line 21-22: The thread-id extraction is inconsistent: change the code
that currently uses Long.parseLong(attr.toString()) (in the
Statistics(ITestResult) path) to use the same Number cast pattern as in
afterInvocation — i.e., cast the attribute to Number and call longValue() — or
alternatively modify the caller to pass the already-extracted threadId into the
Statistics constructor; update the Statistics(ITestResult) constructor (or its
caller) to use ((Number) attr).longValue() instead of toString()/Long.parseLong
to avoid fragile parsing and unify behavior with afterInvocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52a51ba7-543b-44b7-98de-5442a1259d01
📒 Files selected for processing (2)
testng-core/src/test/java/test/factory/issue326/IssueTest.javatestng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java
🚧 Files skipped from review as they are similar to previous changes (1)
- testng-core/src/test/java/test/factory/issue326/IssueTest.java
krmahadevan
force-pushed
the
bugfix/fix_2587
branch
from
ebe472f to
e30a8bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java (1)
14-23: Concurrency primitives look correct.
ConcurrentHashMap.computeIfAbsent+CopyOnWriteArrayList.addis safe under concurrentafterInvocationcalls, andputIfAbsentcorrectly captures the first thread observed per instance. Good replacement for the previous helpers.Minor follow-up (optional): now that
Statistics.threadIdis recorded per invocation, the separatethreadIdsmap duplicates information that can be derived fromresults. Consider droppingthreadIds/getThreadIds()and letting callers compute thread ids fromStatistics(see related comment inIssueTest.java).🤖 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 14 - 23, The threadIds ConcurrentMap and getThreadIds() duplicate data already captured by Statistics.threadId; remove the threadIds field and any getThreadIds() helper, and update callers to derive thread ids from results (use results.computeIfAbsent / the List<Statistics> entries and read Statistics.threadId) so afterInvocation only updates results (method afterInvocation and class-local symbols results, threadIds, getThreadIds, and Statistics.threadId are the places to change).testng-core/src/test/java/test/factory/issue326/IssueTest.java (1)
37-44: Redundant thread-id sources can be consolidated.Lines 37-38 already establish each instance ran on a single distinct thread id (via
Statistics.threadId). The follow-up comparison on lines 42-44 then re-readslistener.getThreadIds()(a separate map populated viaputIfAbsent) for the same information. SinceStatisticsnow carriesthreadId, the second source is redundant — you could derive both checks fromthreadIdsFor(...)and drop thegetThreadIds()map fromLocalTrackingListenerentirely. Not a bug, just simpler and avoids two parallel sources of truth that could drift.♻️ Possible simplification
- // 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); + // 3. Each instance ran on exactly one thread, and the two instances used different threads. + Collection<Long> freddyThreads = threadIdsFor(results, SampleTestClass.FREDDY); + Collection<Long> barneyThreads = threadIdsFor(results, SampleTestClass.BARNEY); + assertThat(freddyThreads).hasSize(1); + assertThat(barneyThreads).hasSize(1); + assertThat(freddyThreads).isNotEqualTo(barneyThreads);🤖 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/IssueTest.java` around lines 37 - 44, The test repeats thread-id checks from two sources; use the existing Statistics.threadId via threadIdsFor(results, ...) exclusively and remove the redundant listener.getThreadIds() usage (and the backing map in LocalTrackingListener) to avoid duplicate state. Update the assertions to compare the single-thread-per-instance values obtained from threadIdsFor(...) for SampleTestClass.FREDDY and SampleTestClass.BARNEY, remove reads of listener.getThreadIds() (and corresponding putIfAbsent logic and map field in LocalTrackingListener), and ensure any tests or code paths that relied on LocalTrackingListener.getThreadIds() are switched to use Statistics.threadId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java`:
- Around line 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.
---
Nitpick comments:
In `@testng-core/src/test/java/test/factory/issue326/IssueTest.java`:
- Around line 37-44: The test repeats thread-id checks from two sources; use the
existing Statistics.threadId via threadIdsFor(results, ...) exclusively and
remove the redundant listener.getThreadIds() usage (and the backing map in
LocalTrackingListener) to avoid duplicate state. Update the assertions to
compare the single-thread-per-instance values obtained from threadIdsFor(...)
for SampleTestClass.FREDDY and SampleTestClass.BARNEY, remove reads of
listener.getThreadIds() (and corresponding putIfAbsent logic and map field in
LocalTrackingListener), and ensure any tests or code paths that relied on
LocalTrackingListener.getThreadIds() are switched to use Statistics.threadId.
In `@testng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java`:
- Around line 14-23: The threadIds ConcurrentMap and getThreadIds() duplicate
data already captured by Statistics.threadId; remove the threadIds field and any
getThreadIds() helper, and update callers to derive thread ids from results (use
results.computeIfAbsent / the List<Statistics> entries and read
Statistics.threadId) so afterInvocation only updates results (method
afterInvocation and class-local symbols results, threadIds, getThreadIds, and
Statistics.threadId are the places to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 757749d7-2f66-4f9a-92f8-3b294b01001a
📒 Files selected for processing (2)
testng-core/src/test/java/test/factory/issue326/IssueTest.javatestng-core/src/test/java/test/factory/issue326/LocalTrackingListener.java
| 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); | ||
| } |
There was a problem hiding this comment.
🧩 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.javaRepository: testng-team/testng
Length of output: 2261
🏁 Script executed:
cat -n testng-core/src/test/java/test/factory/issue326/SampleTestClass.javaRepository: testng-team/testng
Length of output: 1763
🏁 Script executed:
# Check IInvokedMethod to see if isTestMethod() method exists
rg -t java 'isTestMethod' --type-listRepository: 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.javaRepository: 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.
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
juherr
left a comment
juherr
left a comment
There was a problem hiding this comment.
Lgtm but have a look at coderabbit review
Its not relevant to this PR in specific because the sample class in question does not have configuration methods. |
2 participants
Closes #2587
Illustration:
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.
Non-overlapping sequential execution would look like this:
Check:
Freddy starts before Barney ends: 100 < 1600 true
Barney starts before Freddy ends: 950 < 900 false
So the assertion would fail.
Fixes # .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyWe encourage pull requests that:
If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.
Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.
Summary by CodeRabbit