GITHUB-426: run firstTimeOnly/lastTimeOnly configs as a barrier around the invocation pool

[juherr]

Problem

FirstAndLastTimeTest.verifyFirstTimeOnly was flaky, occasionally failing with:

expected: ["beforeMethod", "testMethod", "testMethod"]
but was:  ["testMethod", "beforeMethod", "testMethod"]

Investigation showed this is not a test-only issue — it surfaces a real contract violation in the engine.

Root cause

When a @Test has invocationCount > 1 and threadPoolSize > 1, each invocation runs in its own pool thread (TestInvoker#invokePooledTestMethods). The firstTimeOnly @BeforeMethod / lastTimeOnly @AfterMethod were invoked inside those parallel workers:

• firstTimeOnly ran exactly once (deduped via ConfigInvoker#m_executedConfigMethods) but with no barrier — a losing worker proceeded straight to its test, so a test body could run before the @BeforeMethod completed. The Javadoc says it runs "before the first test invocation", so this is a contract violation, not just a listener-ordering artifact.
• lastTimeOnly had no dedup at all: it ran once per invocation (e.g. 4× for invocationCount=4) instead of once, and could run while invocations were still in flight.

Measured on real execution (method bodies, not listeners), invocationCount=4, threadPoolSize=4:

Config	Times run	Barrier honored?
firstTimeOnly @BeforeMethod	1 ✅	No ❌ — a test ran before it finished: 30/30
lastTimeOnly @AfterMethod	4 ❌	No ❌ — ran while tests pending: 30/30

After the fix, both run exactly once and as a barrier (0/30 violations).

Fix

Hoist both configs out of the pool, mirroring the existing @BeforeGroups/@AfterGroups barrier already present in runWorkers:

• invokePooledTestMethods runs the firstTimeOnly setup once before the workers start, and the lastTimeOnly teardown once after every worker completes.
• The per-invocation clones are flagged (BaseTestMethod#skipFirstAndLastTimeOnlyConfigs) so the in-pool config filtering (TestNgMethodUtils) skips those two configs.

A failing firstTimeOnly in the pooled path still skips all invocations (verified).

Also

• Fix the two issue426 sample class names, which were swapped relative to their content (...WithNoThreadPoolSizeDefined actually had threadPoolSize=5, and vice versa).
• Add coverage for the previously-untested lastTimeOnly + thread pool case, and a regression test asserting that a failing shared firstTimeOnly config skips every parallel invocation (none executed).
• CHANGES.txt entry.

Validation

• Full :testng-core:test suite: 12480 passed, 0 failed.
• FirstAndLastTimeTest deterministic across repeated --rerun-tasks runs.
• autostyleCheck passes.

Summary by CodeRabbit

• Bug Fixes
  • Fixed @BeforeMethod(firstTimeOnly=true) and @AfterMethod(lastTimeOnly=true) to execute correctly as a single barrier around parallel test invocations when using threadPoolSize, ensuring proper synchronization and preventing concurrency issues.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e8a31462-2563-4ee4-952d-e3e5f4c16147

📥 Commits

Reviewing files that changed from the base of the PR and between 9a738f7 and c770aea.

📒 Files selected for processing (9)

• CHANGES.txt
• testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
• testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
• testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java
• testng-core/src/test/java/test/invocationcount/FirstAndLastTimeTest.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithNoThreadPoolSizeDefined.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolAndFailingFirstTimeConfig.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolAndFirstLastTimeConfigs.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolSizeDefined.java

✅ Files skipped from review due to trivial changes (2)

• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolSizeDefined.java
• CHANGES.txt

🚧 Files skipped from review as they are similar to previous changes (7)

• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithNoThreadPoolSizeDefined.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolAndFailingFirstTimeConfig.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolAndFirstLastTimeConfigs.java
• testng-core/src/main/java/org/testng/internal/BaseTestMethod.java
• testng-core/src/test/java/test/invocationcount/FirstAndLastTimeTest.java
• testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
• testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java

---

📝 Walkthrough

Walkthrough

TestNG now enforces firstTimeOnly and lastTimeOnly configuration methods as a barrier around parallel invocationCount thread pool execution. A new skip flag in BaseTestMethod marks per-invocation clones; filtering and skip-detection logic in TestNgMethodUtils disables time-scoped configurations inside the pool; TestInvoker executes these configurations once before and once after worker completion via try/finally. Two existing issue426 sample classes are corrected to match their names, a new sample with thread pool and time-only hooks demonstrates the expected invocation order, a failure scenario sample is added, and test methods verify both successful barrier execution and failure handling.

Changes

First/Last time-only barrier around parallel invocation pools

Layer / File(s)	Summary
Skip flag and API in BaseTestMethod testng-core/src/main/java/org/testng/internal/BaseTestMethod.java	Adds m_skipFirstAndLastTimeOnlyConfigs boolean field to mark clones that should skip first/last time-only configurations, with public getter and setter.
Configuration filter and skip-detection in TestNgMethodUtils testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java	Adds necessary imports; updates doesSetupMethodPassFirstTimeFilter and doesTeardownMethodPassLastTimeFilter to short-circuit when skip flag is active; adds skipsTimeScopedConfigs detector and new filtering helpers filterFirstTimeOnlySetupMethods and filterLastTimeOnlyTeardownMethods.
Barrier execution in TestInvoker.invokePooledTestMethods testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java	Marks per-invocation clones to skip time-scoped configs, runs firstTimeOnly setup methods once before spawning workers, and runs lastTimeOnly teardown methods once after completion via try/finally. Adds invokeTimeOnlyConfigurations helper to select and invoke applicable configurations per instance.
Test coverage and sample test classes CHANGES.txt, testng-core/src/test/java/test/invocationcount/FirstAndLastTimeTest.java, testng-core/src/test/java/test/invocationcount/issue426/*	Changelog entry for GITHUB-426 fix, new sample class SampleTestClassWithThreadPoolAndFirstLastTimeConfigs with barrier-scoped hooks, SampleTestClassWithThreadPoolAndFailingFirstTimeConfig to verify failure handling, two existing issue426 samples corrected to match their names, and new test methods verifyFirstAndLastTimeOnlyWithThreadPool and verifyFailingFirstTimeOnlySkipsAllParallelInvocations with refactored test helpers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A barrier stands 'round parallel pools,
First-time and last-time now follow the rules!
No racing configs inside the thread spawn—
They run once before and once when tasks are gone.
When failures arrive, the pool respects the wall,
Order assured, coherence through it all! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.24% 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 accurately summarizes the main change: fixing firstTimeOnly/lastTimeOnly configs to run as a barrier around the invocation pool, which is the core fix in this pull request.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

testng-core/src/test/java/test/invocationcount/FirstAndLastTimeTest.java (1)

150-162: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Preserve strict ordering for the non-thread-pooled fixture

Using containsExactlyInAnyOrder for both classes removes validation of the sequential ordering contract for SampleTestClassWithNoThreadPoolSizeDefined. Keep order-insensitive matching only for the pooled fixture.

Suggested adjustment

   `@Test`(dataProvider = "classNames", description = "GITHUB-426")
   public void verifyFirstTimeOnly(Class<?> clazz) {
     List<String> invokedMethodNames = run(clazz);
     String[] expected = new String[] {"beforeMethod", "testMethod", "testMethod"};
-    // One of the sample classes uses a thread pool, so its test method invocations
-    // run in parallel. firstTimeOnly guarantees only that the `@BeforeMethod` runs
-    // *exactly once* - it does NOT add a barrier that makes it complete before the
-    // parallel test invocations start. The single worker that runs the
-    // configuration does not block the others (see ConfigInvoker, which gates on
-    // m_executedConfigMethods only), so a testMethod can be recorded before
-    // beforeMethod. We therefore assert on occurrences, not on ordering.
-    assertThat(invokedMethodNames).containsExactlyInAnyOrder(expected);
+    if (clazz == SampleTestClassWithThreadPoolSizeDefined.class) {
+      // Pooled invocations are parallel; ordering is non-deterministic.
+      assertThat(invokedMethodNames).containsExactlyInAnyOrder(expected);
+    } else {
+      // Non-pooled invocations should remain sequential and deterministic.
+      assertThat(invokedMethodNames).containsExactly(expected);
+    }
   }

🤖 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/test/java/test/invocationcount/FirstAndLastTimeTest.java`
around lines 150 - 162, The test method verifyFirstTimeOnly currently uses
containsExactlyInAnyOrder for all test fixtures, but this removes validation of
sequential ordering for the non-thread-pooled fixture
SampleTestClassWithNoThreadPoolSizeDefined which should maintain strict
ordering. Refactor the test to apply conditional assertions based on whether the
class uses a thread pool: use containsExactly for the non-thread-pooled fixture
to preserve strict ordering validation, and use containsExactlyInAnyOrder only
for the thread-pooled fixtures. You can either split this into two separate test
methods with different dataProviders or add conditional logic within
verifyFirstTimeOnly to check the clazz parameter and apply the appropriate
assertion.

🤖 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.

Outside diff comments:
In `@testng-core/src/test/java/test/invocationcount/FirstAndLastTimeTest.java`:
- Around line 150-162: The test method verifyFirstTimeOnly currently uses
containsExactlyInAnyOrder for all test fixtures, but this removes validation of
sequential ordering for the non-thread-pooled fixture
SampleTestClassWithNoThreadPoolSizeDefined which should maintain strict
ordering. Refactor the test to apply conditional assertions based on whether the
class uses a thread pool: use containsExactly for the non-thread-pooled fixture
to preserve strict ordering validation, and use containsExactlyInAnyOrder only
for the thread-pooled fixtures. You can either split this into two separate test
methods with different dataProviders or add conditional logic within
verifyFirstTimeOnly to check the clazz parameter and apply the appropriate
assertion.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de07ef7f-2bc0-43ec-8e3c-c2565b7bfd36

📥 Commits

Reviewing files that changed from the base of the PR and between 2191790 and f501fc7.

📒 Files selected for processing (3)

• testng-core/src/test/java/test/invocationcount/FirstAndLastTimeTest.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithNoThreadPoolSizeDefined.java
• testng-core/src/test/java/test/invocationcount/issue426/SampleTestClassWithThreadPoolSizeDefined.java