Fix flaky test:Move method invocation counter increment to call() method

[krmahadevan]

Closes #2586

This commit fixes the flaky test in test.hook.HookableTest where tests with timeouts (related to issue #599) would inconsistently report pass counts.

Changes:

• Move incrementCurrentInvocationCount() from runOne() to call() method's finally block to ensure it's called exactly once per method execution
• Add try/finally block in call() to guarantee counter increment even when exceptions occur
• Improve exception handling in runOne() to properly propagate errors
• Remove multiple invocation counting when tests are run with timeouts

The root cause was that the invocation counter could be incremented multiple times or inconsistently during test execution with timeouts, leading to flaky behavior where verifyTests() would fail with "expected [1] but found [0]".

This change ensures reliable test counting regardless of timeout settings or execution conditions, fixing issue #2586.

Fixes #2586 .

Did you remember to?

• Add test case(s)
• Update CHANGES.txt
• Auto applied styling via ./gradlew autostyleApply

We encourage pull requests that:

• Add new features to TestNG (or)
• Fix bugs in TestNG

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

• Bug Fixes
  • Improved exception handling so failures during test method execution are consistently wrapped and reported.
  • Ensured the invocation counter is reliably incremented on all exit paths, preserving accurate counts across repeated runs, timeouts, and error scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

InvokeMethodRunnable: runOne() centralizes Throwable handling and wraps failures as TestNGRuntimeException; call() moves m_method.incrementCurrentInvocationCount() into a finally so the invocation count increments on all exit paths.

Changes

Exception Handling and Counter Management

Layer / File(s)	Summary
Exception Wrapping testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java	runOne() removes the prior finally-based counter increment and consolidates reflective invocation with Throwable handling that interrupts the thread and rethrows a wrapped TestNGRuntimeException.
Invocation Counter Guarantee testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java	call() now encloses the invocation loop/single-invocation path in a try with m_method.incrementCurrentInvocationCount() in finally, ensuring the counter increments regardless of exceptions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• juherr

Poem

🐰 I hopped where methods leap and fall,

Wrapped each tumble, answered every call,
Finally I nudged the counter's beat,
So every invocation leaves its seat.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: moving method invocation counter increment from runOne() to call() method to fix flaky tests.
Linked Issues check	✅ Passed	The code changes address the root cause identified in issue #2586 by ensuring the invocation counter is incremented exactly once per method execution, fixing the flaky test failures.
Out of Scope Changes check	✅ Passed	All changes are focused on fixing the invocation counter increment issue in InvokeMethodRunnable, which is directly scoped to the linked issue #2586.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java (1)

70-85: ⚡ Quick win

Add a regression test and a CHANGES.txt entry for this flaky-test fix.

The PR description acknowledges that tests and CHANGES.txt were not updated. For a fix targeted at a flaky test (issue #2586, related to #599), a deterministic regression test that exercises the timeout/invocationCount path and asserts exactly one increment of getCurrentInvocationCount() (and one passed result in verifyTests()) would prevent future regressions of the same class of bug — particularly given that the counter-increment semantics under timeouts are now subtly different from the previous behavior. A short note in CHANGES.txt referencing #2586 is also worth adding for downstream consumers.

Want me to draft a regression test (e.g. a small HookableTest-style class with invocationCount + invocationTimeOut plus a verifier on getCurrentInvocationCount()), and/or a CHANGES.txt line referencing #2586?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`
around lines 70 - 85, Add a deterministic regression test and a CHANGES.txt
entry to prevent regressions around the timeout+invocationCount behavior fixed
in InvokeMethodRunnable.call(): create a small HookableTest-like test that sets
a method with invocationCount > 1 and invocationTimeOut > 0, exercise the
timeout path so only one run completes, assert that
m_method.getCurrentInvocationCount() == 1 (and that verifyTests() reports one
passed result), and ensure the test reliably reproduces the previous flaky
behavior; also add a short CHANGES.txt line referencing issue `#2586` describing
the flaky-test fix so downstream consumers are aware.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`:
- Around line 63-66: The outer catch in InvokeMethodRunnable.runOne that catches
Exception and rethrows new TestNGRuntimeException(e) is redundant and
double-wraps an existing TestNGRuntimeException produced by the inner
catch(Throwable) block; remove that outer try/catch so the inner logic (which
computes the underlying cause and throws the original TestNGRuntimeException
stored/rethrown as t) is the single propagation path, preserving the original
exception identity and cause.

---

Nitpick comments:
In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`:
- Around line 70-85: Add a deterministic regression test and a CHANGES.txt entry
to prevent regressions around the timeout+invocationCount behavior fixed in
InvokeMethodRunnable.call(): create a small HookableTest-like test that sets a
method with invocationCount > 1 and invocationTimeOut > 0, exercise the timeout
path so only one run completes, assert that m_method.getCurrentInvocationCount()
== 1 (and that verifyTests() reports one passed result), and ensure the test
reliably reproduces the previous flaky behavior; also add a short CHANGES.txt
line referencing issue `#2586` describing the flaky-test fix so downstream
consumers are aware.

🪄 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: 3b339a41-2474-4b2f-9dd8-03e0d31d2255

📥 Commits

Reviewing files that changed from the base of the PR and between e371374 and 1bb4eff.

📒 Files selected for processing (1)

• testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java

[coderabbitai]

♻️ Duplicate comments (1)

testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java (1)

52-59: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid re-wrapping TestNGRuntimeException in run()

runOne() now throws TestNGRuntimeException, but run() catches Exception and wraps it again, producing nested TestNGRuntimeException and obscuring the root cause chain.

Proposed fix

 public boolean run() throws TestNGRuntimeException {
   try {
     return call();
+  } catch (TestNGRuntimeException e) {
+    throw e;
   } catch (Exception e) {
     throw new TestNGRuntimeException(e);
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`
around lines 52 - 59, The catch in InvokeMethodRunnable.run() re-wraps
exceptions from runOne() into another TestNGRuntimeException; update the catch
logic so that if the caught Throwable (or its cause) is already a
TestNGRuntimeException you rethrow it as-is instead of creating a new wrapper:
inspect e (and e.getCause()), if instanceof TestNGRuntimeException assign that
instance to t (or throw it directly), otherwise wrap the root cause in a new
TestNGRuntimeException as currently done; ensure this change is applied in the
catch block that sets invoked and t so run() does not produce nested
TestNGRuntimeException instances.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In
`@testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java`:
- Around line 52-59: The catch in InvokeMethodRunnable.run() re-wraps exceptions
from runOne() into another TestNGRuntimeException; update the catch logic so
that if the caught Throwable (or its cause) is already a TestNGRuntimeException
you rethrow it as-is instead of creating a new wrapper: inspect e (and
e.getCause()), if instanceof TestNGRuntimeException assign that instance to t
(or throw it directly), otherwise wrap the root cause in a new
TestNGRuntimeException as currently done; ensure this change is applied in the
catch block that sets invoked and t so run() does not produce nested
TestNGRuntimeException instances.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ce5416e9-11a4-42e3-ad1a-d91016e9279e

📥 Commits

Reviewing files that changed from the base of the PR and between 1bb4eff and 35d8711.

📒 Files selected for processing (1)

• testng-core/src/main/java/org/testng/internal/invokers/InvokeMethodRunnable.java