Fix ITestSkippedEventReceiver not firing for [Skip]-attributed tests

[thomhurst]

Summary

• Fixes ITestSkippedEventReceiver.OnTestSkipped never being invoked when a test is skipped via [Skip("...")] attribute
• Moves RegisterReceivers call earlier in the execution flow, before skip checks, so the EventReceiverRegistry global flags are set and the fast-path doesn't short-circuit

Root Cause

EventReceiverOrchestrator.RegisterReceivers() was only called from TestInitializer.PrepareTest(), which runs after all skip checks in TestCoordinator. When a test was skipped via [Skip] attribute, PrepareTest was never reached, so:

1. The attribute implementing ITestSkippedEventReceiver was never registered in the global EventReceiverRegistry
2. _registry.HasTestSkippedReceivers() returned false
3. InvokeTestSkippedEventReceiversAsync short-circuited at the fast-path check
4. OnTestSkipped was never called

This did not affect runtime skips via Skip.Test() (which throw SkipTestException during test body execution), because PrepareTest runs before the test body.

Fix

Added RegisterReceivers call in ExecuteTestInternalAsync before the first skip check. The _initializedObjects deduplication in the orchestrator ensures no double-registration with the existing call in PrepareTest.

Test plan

• Verify ITestSkippedEventReceiver.OnTestSkipped fires for [Skip]-attributed tests
• Verify existing runtime skip via Skip.Test() still works
• Verify non-skipped test lifecycle (start/end events) is unaffected
• CI passes

Closes #5252

[claude]

Code Review

Good fix — the root cause analysis in the PR description is clear and accurate, and the change is surgical.

What the fix does

populates the global flags (e.g. ). Previously this was only called from , which runs after all skip checks. So for -attributed tests, the flag was never set and short-circuited immediately at:

if (!_registry.HasTestSkippedReceivers())
    return ValueTask.CompletedTask; // always hit for [Skip] tests before this fix

Moving the call earlier (before skip checks) and relying on _initializedObjects.Add() deduplication to prevent double-registration in PrepareTest is the correct and minimal solution. ✓

Strengths

• Minimal change (4 lines) with no risk of regression for the non-skip path.
• ConcurrentHashSet-based deduplication is thread-safe and correct.
• A test case was added (EventReceiverTests.Skipped_Test_With_Event_Receiver) that directly exercises the bug.

One limitation worth documenting

The early RegisterReceivers call at line 64 works for attribute-based event receivers (the common case) and pre-existing objects, because BuildEligibleEventObjects includes details.ClassInstance which will be null before PrepareTest creates it.

This means: if a test class instance itself implements ITestSkippedEventReceiver and the test is skipped via [Skip], OnTestSkipped still won't fire on that instance (the instance doesn't exist yet). This is arguably an inherent limitation of attribute-skip semantics — you can't call a method on an object that hasn't been instantiated — but it's a subtle edge case that's worth a code comment or a doc note so users aren't surprised.

Nit

RegisterReceivers(TestContext context, CancellationToken cancellationToken) accepts but never uses cancellationToken — pre-existing, but now there are two call sites making it more visible. Could be cleaned up separately.

Verdict

The fix is correct, well-reasoned, and includes test coverage. The class-instance edge case is an inherent limitation of the skip path, not a bug introduced here. Ship it.

[claude]

Code Review

Good fix — the root cause analysis in the PR description is clear and accurate, and the change is surgical.

What the fix does

RegisterReceivers populates the global EventReceiverRegistry flags (e.g. HasTestSkippedReceivers()). Previously this was only called from TestInitializer.PrepareTest(), which runs after all skip checks. So for [Skip]-attributed tests, the flag was never set and InvokeTestSkippedEventReceiversAsync short-circuited immediately at:

if (!_registry.HasTestSkippedReceivers())
    return ValueTask.CompletedTask; // always hit for [Skip] tests before this fix

Moving the call earlier (before skip checks) and relying on _initializedObjects.Add() deduplication to prevent double-registration in PrepareTest is the correct and minimal solution.

Strengths

• Minimal change (4 lines) with no risk of regression for the non-skip path.
• ConcurrentHashSet-based deduplication is thread-safe and correct.
• A test case was added (EventReceiverTests.Skipped_Test_With_Event_Receiver) that directly exercises the bug.

One limitation worth documenting

The early RegisterReceivers call works for attribute-based event receivers (the common case) and other pre-existing objects, because BuildEligibleEventObjects includes details.ClassInstance — which will be null before PrepareTest creates it.

This means: if a test class instance itself implements ITestSkippedEventReceiver and the test is skipped via [Skip], OnTestSkipped still won't fire on that instance (the instance doesn't exist yet). This is an inherent limitation of attribute-skip semantics — you can't call a method on an uninstantiated object — but it's a subtle edge case worth a brief comment or doc note so users aren't surprised.

Nit

RegisterReceivers(TestContext, CancellationToken) accepts but never uses the CancellationToken — pre-existing, but now there are two call sites making it more visible. Worth a separate cleanup.

Verdict

Correct, well-reasoned, and includes test coverage. The class-instance edge case is an inherent limitation of the skip path, not a problem introduced here. Good to merge.

[claude]

Follow-up Review (after second commit: dd67ea5)

The second commit adds exactly-once invocation guards for both static ([Skip]) and runtime (Skip.Test()) skip paths — a good addition that improves confidence in the fix.

Potential test ordering concern

The verification test uses:

[NotInParallel(nameof(SkippedEventReceiverTests))]
public class SkippedEventReceiverTests

[NotInParallel] prevents parallel execution but does not guarantee ordering. Verify_SkipEventReceiver_Fired_Exactly_Once depends on SkippedTestWithCustomReason having populated the static Events list first. If the scheduler happens to run the verification test before the skipped test, it will fail spuriously.

A more explicit and robust approach is [DependsOn], which exists in TUnit:

[Test]
[DependsOn(nameof(SkippedTestWithCustomReason))]
public async Task Verify_SkipEventReceiver_Fired_Exactly_Once()
{
    ...
}

This makes the dependency explicit and guarantees the skipped test is processed first, regardless of scheduler decisions.

Previous feedback status

• Class-instance limitation (if the test class itself implements ITestSkippedEventReceiver and is [Skip]-attributed, OnTestSkipped won't fire on the instance) — acknowledged as inherent; no action needed, but a doc note would be nice.
• CancellationToken nit — still present (pre-existing, separate cleanup).

Overall

The core fix (moving RegisterReceivers before the skip check) is correct and minimal. The second commit's exactly-once guards are a valuable addition. The [DependsOn] suggestion above is the main thing worth addressing before merge to avoid a potential ordering-dependent flake.

[claude]

Follow-up Review (after commit 4bb5329)

The third commit addresses the ordering concern from my previous review — replacing [NotInParallel] with [DependsOn(nameof(SkippedTestWithCustomReason), ProceedOnFailure = true)]. This is the right approach and makes the sequencing explicit.

ProceedOnFailure = true — correct choice

This flag is necessary here: the dependency (SkippedTestWithCustomReason) is intentionally [Skip]-attributed, so its outcome is "skipped", not "passed". Without ProceedOnFailure = true, TUnit might propagate the non-passing status and skip the verification test too, defeating the purpose. Good call.

Remaining open items (from prior reviews — no blockers)

1. Class-instance limitation — If a test class itself implements ITestSkippedEventReceiver and is skipped via [Skip], OnTestSkipped won't fire on the instance (the instance doesn't exist yet when early registration runs). This is an inherent limitation worth a brief doc note or XML comment on RegisterReceivers so users aren't surprised. No code change required.

2. Unused CancellationToken parameter on RegisterReceivers(TestContext, CancellationToken) — pre-existing, now has two call sites. Fine to address separately.

3. Stray space in context. Metadata.DisplayName in RuntimeSkipEventReceiverTests.VerifyRuntimeSkipEventFired (line ~193) — pre-existing nit.

Overall

All substantive feedback has been addressed. The fix is correct, minimal, and now has a robust test covering the exact-once invocation guarantee with proper ordering. Ready to merge.