[Flaky test fix] - Issue 2579#3269
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces flaky timestamp-based concurrency detection in the issue188 tests with atomic counters that track started test methods and peak concurrent active methods; tests are reset before execution and assert expected invocation counts and peak concurrency. 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/thread/issue188/Issue188TestSample.java (1)
9-9: Optional: document the coupling ofEXPECTED_METHOD_INVOCATIONSto the suite layout.The constant
6is implicitly derived fromIssueTest's setup (3createXmlTestcalls × 2@Testmethods). If a future change adds/removes anXmlTestor@Testmethod on either side without updating the other, the assertions silently drift. Consider either adding a brief comment here describing the derivation, or moving the constant toIssueTestand computing it inline (e.g.,xmlTestCount * methodsPerClass) so the relationship is explicit at the call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java` at line 9, The constant EXPECTED_METHOD_INVOCATIONS in Issue188TestSample is tightly coupled to IssueTest's suite layout (3 XmlTest × 2 `@Test` methods); update this by either adding a brief explanatory comment next to EXPECTED_METHOD_INVOCATIONS describing its derivation ("3 XmlTest * 2 tests = 6") or, preferably, move the constant into IssueTest and compute it dynamically (e.g., derive xmlTestCount and methodsPerClass and set EXPECTED_METHOD_INVOCATIONS = xmlTestCount * methodsPerClass) so the relationship is explicit and maintained at the source of truth.
🤖 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/thread/issue188/Issue188TestSample.java`:
- Line 9: The constant EXPECTED_METHOD_INVOCATIONS in Issue188TestSample is
tightly coupled to IssueTest's suite layout (3 XmlTest × 2 `@Test` methods);
update this by either adding a brief explanatory comment next to
EXPECTED_METHOD_INVOCATIONS describing its derivation ("3 XmlTest * 2 tests =
6") or, preferably, move the constant into IssueTest and compute it dynamically
(e.g., derive xmlTestCount and methodsPerClass and set
EXPECTED_METHOD_INVOCATIONS = xmlTestCount * methodsPerClass) so the
relationship is explicit and maintained at the source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 447b2ef6-5317-41bb-9d7d-6d9209eb54bb
📒 Files selected for processing (2)
testng-core/src/test/java/test/thread/issue188/Issue188TestSample.javatestng-core/src/test/java/test/thread/issue188/IssueTest.java
| } | ||
|
|
||
| private void sleepSilently() { | ||
| private static void trackParallelExecution() { |
There was a problem hiding this comment.
We are trying to ascertain that at some point all the concurrently spawned test methods did overlap each other since its never going to be possible to truly ascertain if TestNG did start all the tests in parallel. Timestamped based guess work is also still only guestimate because we would need to define a tolerance value. And that tolerance value is what has been causing flakiness in CI.
krmahadevan
force-pushed
the
bugfix/fix_2579
branch
from
9d78c8b to
d8d108c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java (2)
43-53: Sleep duration is now the implicit timing knob — worth a brief comment.Reaching
maxActiveTestMethods == 6still relies on all six methods entering the sleep window before the first one exits. 1 s is far more forgiving than 40 ms and should be robust on CI, but a one-line comment explaining why the sleep exists (to keep methods concurrently active long enough to observe peak parallelism) would help future maintainers avoid “optimizing” it away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java` around lines 43 - 53, Add a one-line comment in the trackParallelExecution method explaining that the TimeUnit.SECONDS.sleep(1) call is intentionally long to keep multiple test methods concurrently active so maxActiveTestMethods can reliably observe peak parallelism (e.g., to ensure all six methods enter the sleep window before any exit), and reference the tracking variables activeTestMethods and maxActiveTestMethods so future maintainers don’t remove or shorten the sleep.
9-9: Consider documenting the coupling ofEXPECTED_METHOD_INVOCATIONSto the suite layout.The value
6is implicitly derived fromIssueTest.testSuiteLevelParallelModeregistering 3XmlTests × 2@Testmethods on this class. If the suite layout or the number of@Testmethods here changes, the assertion semantics inIssueTestwill silently shift (e.g., still pass with a smaller-than-expected concurrency). A short Javadoc comment pinning the derivation, or computing it asnumXmlTests * <test-method-count>inIssueTest, would prevent that drift.📝 Suggested doc-only tweak
- public static final int EXPECTED_METHOD_INVOCATIONS = 6; + // 3 XmlTests in IssueTest#testSuiteLevelParallelMode x 2 `@Test` methods in this class. + public static final int EXPECTED_METHOD_INVOCATIONS = 6;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testng-core/src/test/java/test/thread/issue188/Issue188TestSample.java` at line 9, EXPECTED_METHOD_INVOCATIONS is tightly coupled to the suite layout (derived from IssueTest.testSuiteLevelParallelMode registering 3 XmlTests × 2 `@Test` methods) and can silently drift; either add a short Javadoc on the EXPECTED_METHOD_INVOCATIONS constant in Issue188TestSample explaining the derivation (e.g., "3 XmlTests × 2 test methods in this class = 6"), or update IssueTest.testSuiteLevelParallelMode to compute the expected count dynamically (e.g., multiply the number of registered XmlTest instances by the number of `@Test` methods on Issue188TestSample) so the assertion remains correct when tests/layout change.
🤖 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/thread/issue188/Issue188TestSample.java`:
- Around line 43-53: Add a one-line comment in the trackParallelExecution method
explaining that the TimeUnit.SECONDS.sleep(1) call is intentionally long to keep
multiple test methods concurrently active so maxActiveTestMethods can reliably
observe peak parallelism (e.g., to ensure all six methods enter the sleep window
before any exit), and reference the tracking variables activeTestMethods and
maxActiveTestMethods so future maintainers don’t remove or shorten the sleep.
- Line 9: EXPECTED_METHOD_INVOCATIONS is tightly coupled to the suite layout
(derived from IssueTest.testSuiteLevelParallelMode registering 3 XmlTests × 2
`@Test` methods) and can silently drift; either add a short Javadoc on the
EXPECTED_METHOD_INVOCATIONS constant in Issue188TestSample explaining the
derivation (e.g., "3 XmlTests × 2 test methods in this class = 6"), or update
IssueTest.testSuiteLevelParallelMode to compute the expected count dynamically
(e.g., multiply the number of registered XmlTest instances by the number of
`@Test` methods on Issue188TestSample) so the assertion remains correct when
tests/layout change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79bdd882-03c7-4558-a433-8bccc3cfea1f
📒 Files selected for processing (2)
testng-core/src/test/java/test/thread/issue188/Issue188TestSample.javatestng-core/src/test/java/test/thread/issue188/IssueTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- testng-core/src/test/java/test/thread/issue188/IssueTest.java
Closes testng-team#2579 What the new test measures Instead of checking an arbitrary timestamp gap, each test method records when it is actively executing. ``` time ----> active count 6 | *************** 5 | *** *** 4 | *** *** 3 | *** *** 2 | *** *** 1 | *** *** 0 +------------------------------------------------ m1 m2 m3 m4 m5 m6 start running ``` New assertion: startedTestMethods == 6 maxActiveTestMethods == 6 Meaning: all 6 methods started, and at one point all 6 were observed executing concurrently. Behavioral difference Old: "Did every method start within 40 ms of the previous one?" New: "Did all six methods actually run at the same time?" The new check matches the feature under test: suite-level parallel="methods" should execute methods concurrently. It removes the host-specific 40 ms timing assumption while still failing if execution becomes sequential.
krmahadevan
force-pushed
the
bugfix/fix_2579
branch
from
d8d108c to
3ef61f3
Compare
|
@juherr - That was due to formatting problems. Now its fixed. |
2 participants
Closes #2579
What the new test measures
Instead of checking an arbitrary timestamp gap, each test method records when it is actively executing.
New assertion:
startedTestMethods == 6
maxActiveTestMethods == 6
Meaning:
all 6 methods started, and at one point all 6 were observed executing concurrently.
Behavioral difference
Old:
"Did every method start within 40 ms of the previous one?"
New:
"Did all six methods actually run at the same time?"
The new check matches the feature under test: suite-level parallel="methods" should execute methods concurrently. It removes the host-specific 40 ms timing assumption while still failing if execution becomes sequential.
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