[thomhurst]

Summary

Fixes both reporting problems on --fail-fast runs from #7119, and the related timeout noise from the issue comment.

Before (FailFastReporting7119/* --fail-fast --maximum-parallel-tests 1, both modes):

failed Fails
failed Unhandled exception - InvalidOperationException: Intentional failure to trigger fail-fast (#7119)
  total: 2   failed: 2   skipped: 0

After:

failed Fails
skipped Parallel1 ... skipped Global2   (Test was not run because --fail-fast stopped the test run after a failure)
Test execution stopped due to fail-fast.
  total: 7   failed: 1   skipped: 6

Root cause

1. Duplicate "Unhandled exception" node. When the unconstrained and keyed phases both ran, TestScheduler.WaitForTasksWithFailFastHandling caught the cancellation and re-threw the failing test's own exception (GetFirstFailFastException). That exception is not a cancellation, so TUnitTestFramework sent it to its generic handler, which publishes an "Unhandled exception" test node. The failing test already had its own node. With only one phase running, the re-throw was skipped, which is why the issue needed two phases.
2. Cancelled tests not reported. Tests that fail-fast stopped before they started (waiting for a parallelism slot, a constraint key, the global [NotInParallel] lock, or a phase that never ran) never reached TestCoordinator, which is where results are published. Nothing else reported them.
3. Timeout noise (issue comment). On a cancelled run, the global [NotInParallel] loop kept walking every remaining test into a cancelled lock wait, and TestRunner logged each one as "Unhandled exception in test ...", so a plain timeout looked like a lock deadlock.

Fix

• TestScheduler.RunPhasesConcurrentlyAsync is now plain Task.WhenAll. The cancellation propagates, and the framework's existing fail-fast branch logs it and fails the session (exit code 10, as before --fail-fast: cancelled tests are not reported, and a failure can appear twice as an "Unhandled exception" node #7119). A real non-cancellation fault from a phase is still re-thrown unchanged.
• TestScheduler.ScheduleAndExecuteAsync: on a cancelled run, before the After(TestSession) hooks, every test still in NotStarted, WaitingForDependencies or Queued gets a result. That covers scheduled tests, dynamic tests the queue processor scheduled, and dynamic tests left in the queue. After fail-fast these tests are skipped, with a reason that names --fail-fast, so the failure count stays at the real failures. After any other cancellation they are cancelled. Tests that started already report their own result and are not touched. A test that failed before it ran (data source, argument registration or dependency resolution error) and never reached TestCoordinator gets its failure published, so it does not disappear from the results. Reporting is best effort and never replaces the run's outcome.
• The global [NotInParallel] loop stops as soon as the token is cancelled.
• TestRunner treats an OperationCanceledException on a cancelled token as "not started": it logs at debug level and does not trigger fail-fast again.
• Docs: the FailFast settings table says tests that had not started are reported as skipped.

No public API change: GetFirstFailFastException is kept and still populated, and the new TestRunner.IsFailFastTriggered is internal.

Note: when the platform's own token is cancelled (for example --timeout or IDE stop), MTP aborts the session and drops later test-node messages, so those runs still show Aborted with no per-test nodes. The sweep is a no-op there. The [NotInParallel] "Unhandled exception" log spam is gone in that case too.

Tests

• New FailFastReporting7119 scenario in TUnit.TestProject: a Critical-priority failing test, plus unconstrained, keyed and global [NotInParallel] tests, and DataSourceFails, whose data source throws. Both modes now report 8 tests: 2 failed, 6 skipped. Without the pre-failed fix, DataSourceFails was missing (7 tests, 1 failed).
• New engine test FailFastReportingTests.FailFast_ReportsEveryTestOnce_AndCancelledTestsAsSkipped (--fail-fast --maximum-parallel-tests 1). It asserts there is no "Unhandled exception" node, that all 8 tests have exactly one result, that only Fails and DataSourceFails failed, that the others are Passed or NotExecuted (at least 4 NotExecuted), and that both global tests are NotExecuted. It failed before the fix (found the extra node, and total was 2) and passes after it.

Runs (net10.0, Release, Windows):

• FailFastReportingTests and KeyedNotInParallelFailFastTests: each passed 5 times in a row (Reflection mode; AOT engine tests only run in CI).
• Manual scenario runs in both source-gen and reflection modes: FailFastReporting7119 (above), KeyedNotInParallelFailFastDynamic7074 (total 5: 1 failed, 2 passed, 2 skipped, exit 10) and FailFastTest (total 4: 1 failed, 2 passed, 1 skipped, exit 10). None had an "Unhandled exception" node.
• Engine tests TestExecutionCancellationTests, CancellationAfterHooksTests and TestExecutorCancellationTokenTests passed. ExternalCancellationTests, CanCancelTests and SigtermTests are excluded on Windows, so CI on Linux covers them.
• TUnit.UnitTests: 566 passed, 4 skipped, 0 failed.

Fixes #7119

Summary by CodeRabbit

• Bug Fixes

  • Fail-fast runs now report tests that did not start as skipped after a test failure, rather than treating cancellation while waiting to run as another failure.
  • Other cancellations now report remaining tests as cancelled.
  • Sequential tests that cannot run in parallel stop promptly when the run is cancelled.
  • Errors encountered while reporting remaining tests no longer replace the run’s outcome.

• Documentation

  • Clarified how tests that have not started are reported after fail-fast cancels a run.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-19T21:20:31.166241Z	7627555	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: be36e845-dc18-427d-8ae2-fdd8bef079c8

📥 Commits

Reviewing files that changed from the base of the PR and between 7627555 and 136e062.

📒 Files selected for processing: 5

• src/TUnit.Core/AbstractExecutableTest.cs
• src/TUnit.Engine/Scheduling/TestRunner.cs
• src/TUnit.Engine/Scheduling/TestScheduler.cs
• tests/TUnit.Engine.Tests/Scheduling/FailFastReportingTests.cs
• tests/TUnit.TestProject/Bugs/_7119/FailFastReporting7119.cs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.

📝 Walkthrough

The runner handles requested cancellation separately from test failures. The scheduler reports tests that did not start after cancellation and avoids surfacing the fail-fast failure again as an unhandled exception. Regression tests cover fail-fast result reporting.

Changes: Fail-fast reporting

Layer / File(s)	Summary
Cancellation during scheduling and execution src/TUnit.Core/AbstractExecutableTest.cs, src/TUnit.Engine/Scheduling/TestRunner.cs, src/TUnit.Engine/Scheduling/TestScheduler.cs	The runner marks tests as started when execution begins and handles requested cancellation separately from general exceptions. The scheduler stops sequential execution when cancelled and combines concurrent phases without the previous fail-fast exception recovery.
Reporting tests that did not start src/TUnit.Engine/Scheduling/TestScheduler.cs	The scheduler tracks dynamic tests and reports eligible unstarted tests as skipped during fail-fast or cancelled during other cancellations. It publishes pre-execution failures and suppresses reporting errors.
Fail-fast reporting test and documentation tests/TUnit.TestProject/Bugs/_7119/FailFastReporting7119.cs, tests/TUnit.Engine.Tests/Scheduling/FailFastReportingTests.cs, docs/docs/reference/programmatic-configuration.md	The scenario and assertions cover fail-fast reporting for parallel, keyed, and global tests. The documentation states that tests not yet started are reported as skipped.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant TestScheduler
  participant TestRunner
  participant TestResults
  TestScheduler->>TestRunner: Run tests with cancellation tokens
  TestRunner-->>TestScheduler: Handle requested cancellation separately
  TestScheduler->>TestResults: Report eligible unstarted tests as skipped or cancelled

Loading

Merge Risk: ⚪ Minimal · up to 136e0

The change makes fail-fast runs report each test once and reports tests that never started. No concrete merge-blocking risk was identified in the reviewed changes.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 20.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main engine change: reporting each test exactly once during fail-fast runs.
Linked Issues check		The changes satisfy the coding requirements in [#7119]. Phase cancellation no longer re-publishes the failing exception as an "Unhandled exception" result. The scheduler reports tests that remain not …
Out of Scope Changes check		The changes remain within [#7119]. Scheduler and runner changes implement fail-fast result reporting and cancellation handling. The internal execution-deduplication flag supports correct reporting for…

Full details: Linked Issues check

Explanation

The changes satisfy the coding requirements in [#7119]. Phase cancellation no longer re-publishes the failing exception as an "Unhandled exception" result. The scheduler reports tests that remain not started or queued after fail-fast, including tests stopped during dependency, parallel, keyed, and global-lock scheduling. Pre-execution failures are also reported. The added engine test checks one result per selected test, no duplicate unhandled-exception result, and the expected failed and not-executed results.

Full details: Out of Scope Changes check

Explanation

The changes remain within [#7119]. Scheduler and runner changes implement fail-fast result reporting and cancellation handling. The internal execution-deduplication flag supports correct reporting for dependency-related execution paths. The documentation, regression test, and fail-fast scenario directly support the behavior. No unrelated public API change or unrelated implementation is shown.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

_{A rabbit watched the tests take flight,
One failure paused the queue outright.
The waiting tests were marked with care,
No extra failure joined them there.
The logs stayed clear; the results were right,
Then hopped the rabbit into the night.}

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7627555057

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The reviewed changes appear safe to merge.

Summary

Completes fail-fast reporting by including tests that failed before execution.

• Cancelled runs report results for tests that never started.
• Parallel test phases keep the original failure from becoming a second result.

No new actionable issues were found in the changes since the previous review. No previous review threads were supplied. Review checks were source-only; no tests were run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Run cancelled] --> B[Inspect remaining tests]
  B --> C{Failed before execution?}
  C -->|Yes| D[Report original failure]
  C -->|No| E{Still waiting to run?}
  E -->|No| F[Leave existing result unchanged]
  E -->|Yes| G{Fail-fast triggered?}
  G -->|Yes| H[Report skipped]
  G -->|No| I[Report cancelled]

Loading

_{Reviews (2) · Last reviewed commit: "fix(engine): report pre-failed tests tha..." · Reviewed by Greptile}

[github-actions]

Review of #7145

I reviewed the diff by reading it. I didn't build the PR or run its tests. The skill-based review tool failed to run, so this was a manual pass.

Overall this is a good fix. It addresses each root cause directly:

• Replacing WaitForTasksWithFailFastHandling with plain Task.WhenAll removes the re-thrown non-cancellation exception that produced the duplicate "Unhandled exception" node. It also deletes code, which is a simplification.
• The cancelled-run sweep (ReportTestsNotStartedAsync) runs before the After(TestSession) hooks, so every selected test gets exactly one result. Only NotStarted, WaitingForDependencies and Queued tests are touched, and the sweep is best-effort so it can't replace the run's outcome.
• Using skipped for fail-fast and cancelled for other cancellations keeps the failure count accurate.
• The ThrowIfCancellationRequested in the global [NotInParallel] loop and the OperationCanceledException catch in TestRunner remove the timeout noise.
• The engine test and the TUnit.TestProject fixture cover the original repro.

Minor observations, none blocking:

1. State check race: ReportIfNotStartedAsync reads test.State and then writes the result. This is safe only because the sweep runs after the phases have completed. If a phase ever faults early while sibling tasks are still running, a Queued test could be double-reported. A comment in finally stating that invariant would help. An atomic state transition, such as a compare-exchange to skipped, would be sturdier.
2. Unsynchronised _dynamicTests: the comment says no lock is needed because the processor has finished by the time the sweep runs. This holds only if the processor task is always awaited before finally runs, including on the exception path. Please confirm that.
3. Cancelled branch: it calls TestSessionContext.Current?.MarkFailure() but the skipped branch doesn't. That seems intentional, since skipped tests shouldn't count as failures. A short comment would make the intent clear.
4. Test assertion: NotExecuted >= 4 depends on scheduling. The comment explains why it holds, so it looks safe, but it is worth watching for flakiness.

LGTM.

[thomhurst]

How I handled the review feedback:

• Codex P1 (tests that already failed and were stopped by fail-fast): Fixed in 136e062. See the thread.
• Claude review, item 1 (state check race): The invariant holds, so I added a comment instead of an atomic transition. The sweep runs in finally only after ExecuteGroupedTestsAsync returns or throws. By then Task.WhenAll has waited for every phase task, so no test can still be changing state.
• Claude review, item 2 (_dynamicTests without a lock): Confirmed. ExecuteGroupedTestsAsync awaits dynamicTestProcessingTask on both the success path and the catch path before rethrowing. The comment on ReportIfNotStartedAsync now says this.
• Claude review, item 3 (MarkFailure only in the cancelled branch): This is intentional. Skipped tests must not fail the session, and the cancelled branch does what TestCoordinator does for cancelled tests. I added a comment.
• Claude review, item 4 (NotExecuted >= 4): No change. Fails has Critical priority and runs no later than second with --maximum-parallel-tests 1, so the two global [NotInParallel] tests and the tests still waiting for the slot are always stopped. The test passed 5 runs in a row locally after this change.

CI: documentation-routes, Apple smoke tests and the ubuntu and macOS modularpipeline jobs on 7627555 were not failures. They were cancelled by hand ("The run was canceled by @thomhurst"), so they have no failed step logs. The Windows pipeline, which includes the engine tests, passed. The push re-runs all of them.

[github-actions]

Review of #7145

I read the diff but did not build the PR or run its tests. I found no blocking issues.

What looks good

• Making RunPhasesConcurrentlyAsync a plain Task.WhenAll fixes the cause of the duplicate "Unhandled exception" node. Re-throwing the failing test's own exception was the wrong mechanism, because the framework treats it as a generic failure instead of a cancellation. Letting the cancellation propagate lets the existing fail-fast branch handle it, and the old helper's special case can be deleted.
• The sweep in ReportTestsNotStartedAsync runs in finally, before the session hooks. That gives every selected test exactly one result. It covers scheduled tests, dynamic tests the queue processor scheduled, and dynamic tests still sitting in the queue.
• Skipped for fail-fast and cancelled for other cancellations is sensible, and skipped tests don't inflate the failure count.
• HasStartedExecution is a cheap way to tell "failed before reaching the coordinator" from "already published". The DataSourceFails regression test covers that case.
• The sweep is best effort and logs instead of throwing, so it can't replace the run's outcome.
• The ThrowIfCancellationRequested in the global [NotInParallel] loop removes the per-test "Unhandled exception" log noise on timeout. The new OperationCanceledException when (token.IsCancellationRequested) catch in TestRunner does the same for tests waiting on a lock or slot.

Suggestions (non-blocking)

1. ReportIfNotStartedAsync reads State and then writes the result, and its comment says this is safe because every phase and the queue processor have finished. That holds for the phase tasks, since Task.WhenAll waits for all of them even when one faults. I couldn't confirm it for the dynamic queue processor on the exception path. If the processor can still be running when the finally block executes, _dynamicTests (a plain List) could be read while it is being appended to. Awaiting the processor, or snapshotting the list under a lock, would remove the dependency on that ordering.
2. HasStartedExecution is an internal bool on the public AbstractExecutableTest, next to State. A TestState value or a field on the state manager would keep the lifecycle in one place. This is a minor design point, not a problem today.
3. The test asserts NotExecuted >= 4, which depends on the single-slot scheduling order. It looks deliberately loose, but watch it for flakiness on slow CI runners.

Overall this is a clean root-cause fix with good test coverage. Approving in spirit.

[thomhurst]

Dispositions for the latest claude-review comment (on 136e062):

• Item 1 (dynamic queue processor on the exception path): no change; the ordering holds. In TestScheduler.ExecuteGroupedTestsAsync, the catch around ExecuteAllPhasesAsync calls _dynamicTestQueue.Complete() and then awaits dynamicTestProcessingTask (swallowing its exception) before it rethrows. The success path does the same after the try. The sweep in ReportTestsNotStartedAsync runs in the finally of the caller, so it only starts after ExecuteGroupedTestsAsync has returned or thrown, and by then the processor has finished on either path. _dynamicTests is written only by the processor (TrackDynamicTests), so nothing can append to it while the sweep reads it. The comments on ReportIfNotStartedAsync and TrackDynamicTests state this.
• Item 2 (HasStartedExecution next to State): no change. A new TestState value would be visible to every consumer of the public State property and would need handling in each switch over it. The flag is internal and only answers "did TestCoordinator take ownership of this test's result", which is a different question from the lifecycle state.
• Item 3 (NotExecuted >= 4): no change, as in the earlier dispositions. Fails has Critical priority and runs first or second with --maximum-parallel-tests 1, so the bound holds regardless of runner speed.

No code changes, so no new commit.