You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Isolate test registration callback failures - #7556
An exception from an OnTestRegistered receiver or delegate currently escapes discovery and aborts the entire test request. Catch registration failures per test, preserving the original exception as that test's result so unrelated tests still execute. Invalidate cached display names even when a callback fails.
Adds regression fixtures for receiver and asynchronous delegate failures in sequential (3 tests) and parallel (8 tests) registration batches, plus engine assertions for failure messages and unaffected passing tests.
Validation: built TUnit.TestProject for net10.0 and ran both fixtures directly in generated and reflection modes. All four runs reported exactly two intended failures with their original messages; the remaining 1 or 6 tests passed. Engine tests are left to CI per repository instructions; Native AOT execution is also left to CI. No metadata generation or public API changes.
Review follow-up: best-effort track existing argument objects after callback failures and clear earlier skip reasons so the original failure wins. Added a disposal observer with a throwing graph getter and skip-before-throw coverage. Rebuilt net10.0 and ran all three fixtures separately in both generated and reflection modes; results matched their expected intentional failures and all observer/unrelated tests passed.
Review was skipped as selected files did not have any reviewable changes.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 6472b172-fab0-448d-bea5-4670da163c09
Commits
Reviewing files that changed from the base of the PR and between 94fece1 and c10f713.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
Registration callback exceptions are now handled for the affected test instead of escaping registration. The service attempts to track existing arguments for disposal, clears the skip reason, marks the test as failed, and invalidates the display-name cache in a finally block. New tests cover callback failures and disposal.
RegisterTest catches registration failures and marks the affected test as failed. It attempts to track arguments for disposal and logs tracking failures. Cache invalidation now runs in finally. Test cases cover receiver and delegate exceptions, serial and parallel registration, and disposal.
Registration failure result assertions tests/TUnit.Engine.Tests/RegistrationFailure7525Tests.cs
Engine tests assert failure counts and error messages for registration failures. They also check that unrelated tests pass and that the disposal case reports the expected results.
thomhurst/TUnit#7292: Adds another OnTestRegistered receiver pass in TestFilterService; this change handles failures from that receiver path.
thomhurst/TUnit#7417: Adds OnTestRegistered delegate invocation in TestFilterService; this change handles exceptions from those delegates.
thomhurst/TUnit#7328: Transfers cleanup ownership to tests and failed rows; this change tracks existing arguments when registration fails.
Merge Risk: Minimal · previous review d6be2
Registration failures are isolated per test; no merge-blocking risk was identified.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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 change: isolating failures from test registration callbacks.
Linked Issues check
Issue #7525 requires registration callback failures to become failures of the affected test and not abort unrelated tests in generated and reflection modes. TestFilterService.RegisterTest catches fa…
Out of Scope Changes check
The reviewed changes are limited to isolating registration callback failures in TestFilterService and adding focused regression fixtures and engine assertions for issue #7525. The argument-disposal …
Full details: Linked Issues check
Explanation
Issue #7525 requires registration callback failures to become failures of the affected test and not abort unrelated tests in generated and reflection modes. TestFilterService.RegisterTest catches failures per test, clears an earlier skip reason, stores the original exception with TestState.Failed, and invalidates the display-name cache in finally. The same method runs in both the sequential and parallel registration paths. The added fixtures cover receiver and asynchronous delegate failures, and RegistrationFailure7525Tests asserts failure counts, original message text, and unaffected passing tests for 3-test and 8-test batches. The engine test is parameterized by TestMode.
Full details: Out of Scope Changes check
Explanation
The reviewed changes are limited to isolating registration callback failures in TestFilterService and adding focused regression fixtures and engine assertions for issue #7525. The argument-disposal tracking and skip-precedence coverage support the required per-test failure behavior. No unrelated production feature, public API change, or unrelated test scope is evidenced.
Full details: Docstring Coverage
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
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.
A rabbit checks each test in line If callbacks fail, it marks the sign It tracks the arguments for their rest And lets the other tests proceed The cache is cleared before it’s done
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b9fee5d2-3886-4ba4-9ac2-5675d62263d4
Commits
Reviewing files that changed from the base of the PR and between 4891fce and d6be20e.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.
Splitting RegisterTest into a wrapper plus RegisterTestCore puts the catch in one place. Sequential and parallel registration both go through it, so one failing test no longer aborts the request.
InvalidateDisplayNameCache now runs in finally. That also covers the early return inside the delegate path, so removing the two inline calls is safe.
Pre-failed tests are already handled downstream. TestScheduler (~line 253) and TestCoordinator (~line 88) both skip execution for a test that is already Failed and report the stored exception. The test body won't run, and the fixtures assert this.
The tests cover the receiver and async delegate cases, and both the sequential and parallel (8+) batches. The engine assertions check the failure messages and that the unrelated tests still pass.
Minor, not blocking
catch (Exception) will also swallow OperationCanceledException if the request is cancelled during registration. The test would then be reported as Failed instead of the cancellation propagating. If registration takes a cancellation token, consider rethrowing when it is cancelled.
Registration failures are now recorded only on the test result. If a user receiver fails, nothing is logged at discovery time. A debug-level log line would make this easier to diagnose.
I couldn't run the tests here. The engine tests are left to CI, per the repo instructions. Approving in spirit.
[Medium impact] The PR appears safe to merge; no new actionable issues were found.
Summary
Registration callback errors now fail the affected test without stopping registration of unrelated tests.
Registration callback failures stay with the test that triggered them.
Both earlier findings are addressed: the catch tracks existing arguments and clears SkipReason before recording the failure. The supplied threads are unnumbered, so they have no entries in previousFindings.
Addressed the blocking disposal and skip-precedence findings in d6be20e.
Validation: net10.0 TestProject build succeeded. Targeted RegistrationFailureTests, ParallelRegistrationFailureTests, and RegistrationDisposalTests ran separately in generated and reflection modes. Each produced only its intended callback failures (2/3, 2/8, and 1/2 respectively), original error messages, zero skips, and passing unrelated/disposal-observer tests. Engine tests and Native AOT remain for CI per repository guidance.
Claude's nonblocking suggestions: registration has no request cancellation token, so there is no request-token cancellation to distinguish here; callback-thrown cancellation remains a per-test failure. Receiver errors are already logged by InvokeTestRegisteredReceiver, and delegates retain their exception in the reported test result. Keeping that existing diagnostic behavior avoids an extra duplicate error log.
The fix is sound. Splitting RegisterTest into a catching wrapper and RegisterTestCore puts the isolation in one place. Sequential and parallel registration both go through it, and the finally now invalidates the display-name cache on every path, including the early return in the delegate path.
I checked the earlier feedback against the head commit:
Argument disposal (Greptile P1): it is handled. TrackSkippedTestArguments runs best-effort and its own failure is only logged, so a throwing tracking getter can't mask the original error. RegistrationDisposalTests covers this.
Skip precedence (Greptile P1): it is handled. SkipReason = null before SetResult(Failed, ex) means the registration failure wins over an earlier SetSkipped. The receiver fixture exercises this.
Cancellation and logging (Claude's earlier notes): your reply is reasonable. Registration has no request token, and receiver failures are already logged.
Nits, non-blocking:
tests/TUnit.TestProject/Bugs/7525/Tests.cs: the OnTestRegistered body in ThrowingReceiverAttribute has a stray indent on the opening brace.
RegistrationFailure7525Tests.cs: add a blank line between the two [Test] methods.
The TrackSkippedTestArguments name is now slightly misleading, since it is also used for registration-failed tests. If it is reused again, a more neutral name or a short doc comment would help. The inline comment already explains this at the call site, so it is fine as is.
Engine tests and AOT are left to CI, as the repo instructions say. Approving.
Current-head Claude and Codex verdicts are positive, and all blocking threads are dispositioned. The formatting suggestions are nonblocking and left as-is for this review cycle; the existing inline comment explains tracking failed arguments, so a helper rename is unnecessary here. Promoting the reviewed head d6be20e to ready. CI and the promotion-triggered Claude review must complete before the merge gate can arm auto-merge.
Review: Isolate test registration callback failures
The change is sound. Wrapping RegisterTestCore in a per-test try/catch fits the existing failure path: TestScheduler and TestCoordinator already publish a TestState.Failed result set before execution. Moving InvalidateDisplayNameCache into a finally also removes the duplicated call. Clearing SkipReason so the real failure wins is a reasonable call, and the tests cover both the sequential and parallel (8 or more tests) paths.
Points to consider (none blocking):
Cancellation is swallowed.catch (Exception) also catches OperationCanceledException. If the session is cancelled during registration, the test will be recorded as a failure rather than cancellation propagating. Consider catch (Exception ex) when (ex is not OperationCanceledException), or at least check the cancellation token. If registration has no cancellation token, this is moot, but it is worth confirming.
Discovery-only mode. With isForExecution == false the failure is set on the test but nothing publishes it. A broken receiver may show as a normal discovered test and only fail at run time. This is probably acceptable, but a short comment would make the intent clear.
Test-fixture nit. In Bugs/7525/Tests.cs, OnTestRegistered in ThrowingReceiverAttribute has a misindented {. RegistrationDisposable.Created is a static list that is never cleared, so the disposal test depends on only one instance being created. That holds today but is fragile if the fixture is rerun or retried.
Test formatting. There is a missing blank line between the two test methods in RegistrationFailure7525Tests.cs.
Overall this is a good, minimal fix for #7525, and I have no blocking concerns.
The earlier blocker is cleared: Claude run 41778376506 completed successfully with no blocking concerns. Read every review surface; existing threads remain resolved and the current-head Codex verdict was positive before this rebase.
The macOS .NET job subsequently timed out in RunAssertionsAnalyzersTestsModule after 30 minutes. Its hang artifact has only six active-test names and no managed dump; cleanup killed an orphan createdump. Tracked this separate failure as #7574 rather than treating it as a diagnosed infrastructure flake or blindly rerunning it.
Rebased onto main 98b0697 (eight new base commits) and pushed 94fece1 for fresh CI/review. Both patches are unchanged according to git range-diff. No rerun of the failed head was requested.
Validation on the rebased head: SDK 10.0.403; net10.0 TestProject build succeeded (existing warnings, zero errors). Ran RegistrationFailureTests, ParallelRegistrationFailureTests and RegistrationDisposalTests separately in generated and reflection modes. Both modes reported the intended failure/pass counts 2/1, 2/6 and 1/1, original callback messages and zero skips. No local engine suite run; engine/AOT remain for CI.
Promotion review suggestions are nonblocking: registration has no request cancellation token; discovery preserves the catalogue while execution reports the per-test failure. The fixture runs once per child process today; repeated-session isolation and formatting suggestions remain follow-ups rather than expanding this fix. Fresh head reviews and CI are required before auto-merge can be armed.
The fix is sound and small. RegisterTest now wraps the old body (renamed RegisterTestCore) in try/catch/finally. A callback failure is recorded per test with SetResult(TestState.Failed, ex), so one throwing receiver or delegate no longer aborts discovery. The scheduler already handles pre-failed tests (TestScheduler.cs:253, State == Failed && !HasStartedExecution). I did not run the tests.
What I checked
Moving InvalidateDisplayNameCache() into finally is correct. It also removes the early-return duplicate.
Clearing SkipReason so the original failure wins is a sensible precedence rule.
The best-effort TrackSkippedTestArguments call is guarded and logged at debug level, so a bad tracking getter cannot mask the original exception.
The regression fixtures cover receiver and async-delegate failures in both the sequential and parallel (8+ tests) paths. They also cover argument disposal.
Suggestions (non-blocking)
catch (Exception ex) will also swallow OperationCanceledException from a cancelled test request and mark the test Failed. Consider catch (Exception ex) when (ex is not OperationCanceledException), or at least check the cancellation token, so a Ctrl+C during discovery is not reported as test failures.
The cleanup in the catch block (track arguments, clear the skip, set the result) would read better as a small MarkRegistrationFailed(test, ex, isForExecution) helper. RegisterTest would then stay a thin wrapper.
Test nits: ThrowingReceiverAttribute.OnTestRegistered has a mis-indented brace. RegistrationFailure7525Tests is missing a blank line between the two [Test] methods.
Overall this looks good to merge once CI is green. Suggestion 1 is the only one I would consider before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Inactive
An exception from an
OnTestRegisteredreceiver or delegate currently escapes discovery and aborts the entire test request. Catch registration failures per test, preserving the original exception as that test's result so unrelated tests still execute. Invalidate cached display names even when a callback fails.Adds regression fixtures for receiver and asynchronous delegate failures in sequential (3 tests) and parallel (8 tests) registration batches, plus engine assertions for failure messages and unaffected passing tests.
Validation: built
TUnit.TestProjectfor net10.0 and ran both fixtures directly in generated and reflection modes. All four runs reported exactly two intended failures with their original messages; the remaining 1 or 6 tests passed. Engine tests are left to CI per repository instructions; Native AOT execution is also left to CI. No metadata generation or public API changes.Review follow-up: best-effort track existing argument objects after callback failures and clear earlier skip reasons so the original failure wins. Added a disposal observer with a throwing graph getter and skip-before-throw coverage. Rebuilt net10.0 and ran all three fixtures separately in both generated and reflection modes; results matched their expected intentional failures and all observer/unrelated tests passed.
Closes #7525
Summary by CodeRabbit