[thomhurst]

Fixes #7290

Problem

TestFilterService.RegisterTest invokes ITestRegisteredEventReceivers once, before argument registration. That order is deliberate: a SkipAttribute can skip a test before expensive data sources are created. Injected property values are only resolved later, in RegisterTestArgumentsAsync, so a shared fixture received OnTestRegistered as a constructor or method argument but never as an injected property. A fixture that called SetParallelLimiter limited constructor consumers but not property consumers.

Fix

After argument registration succeeds, and when the test has injected properties, RegisterTest runs a second receiver pass. Argument registration already drops the receiver caches (#6554), so the rebuilt list includes the property values. The second pass shares the first pass's reference-identity set, so receivers that already ran (attributes, constructor and method arguments, installed executors) are not invoked twice. The receiver and executor-drain loop moved into a helper used by both passes.

Behavior notes, documented in event-subscribing.md and on ITestRegisteredEventReceiver:

• Property receivers run after every first-pass receiver, whatever their Order.
• A property receiver can call SetSkipped. The shared objects already exist at that point, because the receiver is one of them. TestCoordinator handles the skip as it does for any registration-time skip.
• The second pass is skipped when argument registration fails, and in discovery-only requests (property values are not resolved there).

The change is in the shared registration path, so it covers source-gen and reflection modes with one implementation. ParallelLimiter is read by TestRunner at execution time, so a limiter set in the second pass takes effect.

Tests

tests/TUnit.TestProject/Bugs/_7290/Tests.cs:

• Property consumer with [Repeat(3)]: asserts Parallelism.Limiter is OneAtATime, exactly one OnTestRegistered per test, and a maximum of one test running at once.
• Constructor plus property consumer of the same shared instance: asserts one callback per test (identity dedupe).
• Property fixture that calls SetSkipped: the test body throws, so the test passes only if it is skipped.

tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs asserts 6 total, 5 passed, 1 skipped.

Local results:

• Without the fix (source-gen): 5 of 6 failed, with Value is of type null, not OneAtATime and the skip test executing.
• With the fix (source-gen, direct run): 5 passed, 1 skipped.
• Engine.Tests reflection mode: RegisteredReceiverInjectedProperty7290Tests, PartialInjectionFailure6554Tests and ExpectedStateTests.Pass all passed. AOT mode is skipped locally and runs in CI.
• Related source-gen filters: Bugs._6557, DynamicSkipReasonTests and *ParallelLimit* (35 tests) passed. Bugs._6554 has its one intentional registration failure.

Summary by CodeRabbit

• New Features
  • Receivers provided through injected properties are now notified before the test runs, following receivers registered during the initial pass. They can apply registration actions, including skipping tests.
• Documentation
  • Clarified when injected-property receivers are notified and that shared data sources may already be created when they skip a test.

[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-30T15:04:03.764263Z	8385ded	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

• 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: b2d62d71-d716-429c-a077-288dbe0fb044

Commits

Reviewing files that changed from the base of the PR and between 776f1e8 and 8385ded.

Files selected for processing: 5

• docs/docs/writing-tests/event-subscribing.md
• src/TUnit.Core/Interfaces/ITestRegisteredEventReceiver.cs
• src/TUnit.Engine/Services/TestFilterService.cs
• tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs
• tests/TUnit.TestProject/Bugs/_7290/Tests.cs

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

Walkthrough

The registration flow now invokes receivers from injected property values after successful argument registration, using the existing context and reference-identity tracking to avoid duplicate callbacks. Added tests cover parallel limits, shared instances, skipping, and callback ordering and timing documentation.

Changes: Property Receiver Registration

Layer / File(s)	Summary
Registration receiver phases src/TUnit.Core/Interfaces/ITestRegisteredEventReceiver.cs, docs/docs/writing-tests/event-subscribing.md, src/TUnit.Engine/Services/TestFilterService.cs	The receiver context and invocation logic are shared across registration phases. After argument registration succeeds, the service invokes newly available property receivers without invoking previously processed receivers again. The interface remarks and documentation describe callback ordering and data-source timing.
Property receiver behavior tests tests/TUnit.TestProject/Bugs/_7290/Tests.cs, tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs	Added tests check parallel limiting, registration counts, shared-instance identity, concurrency, and skipping. The engine test asserts the completed run has 6 tests: 5 passed, 0 failed, and 1 not executed.

Priority: Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: Minimal · up to 8385d

Property-injected receivers gain registration callbacks; no merge-blocking issue was established.

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: invoking OnTestRegistered for property-injected data sources in the engine.
Linked Issues check		Issue #7290 requires property-injected data-source values that implement ITestRegisteredEventReceiver to receive OnTestRegistered before scheduling. TestFilterService.RegisterTest performs a sec…
Out of Scope Changes check		The changed files remain connected to issue #7290. The engine changes implement the second receiver pass. The interface and event-subscribing documentation describe the resulting timing and behavior. …

Full details: Linked Issues check

Explanation

Issue #7290 requires property-injected data-source values that implement ITestRegisteredEventReceiver to receive OnTestRegistered before scheduling. TestFilterService.RegisterTest performs a second receiver pass after successful execution-time argument registration, rebuilds the receiver list, and shares a reference-identity set with the first pass. This invokes property receivers, prevents duplicate callbacks for shared constructor and property instances, and applies actions such as SetParallelLimiter and SetSkipped. The added tests cover property injection, limiter behavior, identity deduplication, and registration-time skipping.

Full details: Out of Scope Changes check

Explanation

The changed files remain connected to issue #7290. The engine changes implement the second receiver pass. The interface and event-subscribing documentation describe the resulting timing and behavior. The added engine and test-project tests verify the issue behavior. No unrelated change is shown in the reviewed diff.

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 4 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

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 checks the callback queue.
Property receivers join the view.
One shared context follows through.
A limiter keeps the tests in line.
A skipped test stops before its time.}

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

[github-actions]

Review of #7292

I read the diff only; I did not build it or run any tests. The skill runner errored, so this is a manual review.

The approach is sound. It fixes the cause: property values only exist after RegisterTestArgumentsAsync, so a second receiver pass is needed. It reuses the first pass's reference-identity set, so receivers that already ran aren't invoked twice. It sits in the shared registration path, so source-gen and reflection modes both get it. Extracting InvokeTestRegisteredReceivers and CreateTestRegisteredContext removes duplication. The tests cover the limiter, constructor-plus-property dedupe and skip cases, and the docs and the interface remarks describe the ordering caveat.

Minor points, none blocking:

1. Gate on injected properties. The second pass runs only when TestClassInjectedPropertyArguments.Count > 0. Property values from other sources, such as method-level or nested data-source properties, would not trigger it. If those can also yield receivers, consider gating on whether the rebuilt receiver list has any entry not already in invokedReceivers. That would be more robust than the property-count check, and a cheap reference-set check would cover it.
2. Exception handling. Argument registration failures are caught and recorded with test.SetResult(Failed, ex). If a second-pass receiver throws, I haven't confirmed it gets the same handling as a first-pass throw. Please confirm that InvokeTestRegisteredReceiver handles exceptions the same way for both passes, and ideally add a test for a property receiver that throws.
3. Ordering doc. "Property receivers run after every first-pass receiver, whatever their Order" is documented, which is good. It is a behavioural wart for users who rely on Order, so keep it prominent in the docs.

Overall this looks good to merge once point 2 is confirmed.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

The PR calls OnTestRegistered on injected property values after their arguments are registered.

• Injected property receivers now get a registration callback.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Run first-pass receivers] --> B{Test skipped?}
  B -->|Yes| C[Track existing arguments and return]
  B -->|No| D{Execution request?}
  D -->|No| E[Return without resolving properties]
  D -->|Yes| F[Register arguments and resolve properties]
  F --> G{Succeeded and properties exist?}
  G -->|Yes| H[Rebuild receivers and invoke unseen instances]
  G -->|No| I[Finish registration]
  H --> I

Loading

_{Reviews (1) · Last reviewed commit: "fix(engine): invoke OnTestRegistered on ..." · Reviewed by Greptile}