fix: cascade HookExecutorAttribute from class/assembly to hooks (#5462)

[thomhurst]

Summary

Fixes #5462. HookExecutorAttribute was only honored when applied directly to a hook method — placing it on the containing class or the assembly was silently ignored, and the hook ran on DefaultExecutor. This is the inverse gap to #5452/#5463 (which fixed CultureAttribute / STAThreadExecutorAttribute / TestExecutorAttribute not applying to hooks at all).

Approach

Mirror the pattern #5463 introduced. Make the non-generic HookExecutorAttribute base implement IHookRegisteredEventReceiver and IScopedAttribute (with ScopeType = typeof(IHookExecutor)); the generic HookExecutorAttribute<T> inherits the implementation. Cascading is delivered for free by the existing receiver pipeline:

• MethodMetadata.GetCustomAttributes() already aggregates method + class + assembly attributes, in that order.
• EventReceiverOrchestrator.InvokeHookRegistrationEventReceiversAsync runs them through ScopedAttributeFilter (first wins per ScopeType), so method-level beats class-level beats assembly-level automatically.
• HookMethod.SetHookExecutor leaves _hookExecutorIsExplicit = false, so cascaded executors remain overridable by per-test CustomHookExecutor (How to wrap all methods of a test correctly? #2666 path), while a method-level [HookExecutor<T>] still wins via the init-time _hookExecutorIsExplicit = true branch in ResolveEffectiveExecutor.

No changes to discovery services — both source-gen (HookMetadataGenerator) and reflection (ReflectionHookDiscoveryService) modes are fixed via the receiver pipeline alone, exactly as the issue suggested.

The base attribute also gains [AttributeUsage(Assembly | Class | Method)] (it had no AttributeUsage previously and defaulted to All) and [DynamicallyAccessedMembers(PublicConstructors)] on both the constructor parameter and the generic T parameter for AOT/trim correctness.

Public API delta

HookExecutorAttribute (non-generic) now implements IHookRegisteredEventReceiver + IScopedAttribute and exposes Order, ScopeType, OnHookRegistered. Snapshot files updated for net8.0 / net9.0 / net10.0 / net472. Additive only — +semver:minor.

Test plan

• New HookExecutorHookTests.cs fixtures (mirroring CultureHookTests):
  • HookExecutorHookTests_ClassLevel — class-level [HookExecutor<T>] cascades to Before/After(Class) and Before/After(Test)
  • HookExecutorHookTests_MethodLevelOverride — method-level beats class-level (asserts class-level executor was not invoked)
  • HookExecutorHookTests_InheritsClassLevel — class-level applies when no method-level override
  • Each fixture uses its own RecordingHookExecutor subclass so parallel runs stay isolated
• New tests confirmed failing against pre-fix main, then passing after fix
• All four new tests pass in source-gen mode
• All four new tests pass in reflection mode (--reflection)
• Existing HookExecutorTests (5 tests, method-level coverage of every hook lifecycle slot) pass in both modes — no regression in the existing path
• Existing CultureHookTests_* (4 tests, the analogous [Bug]: Test hooks are run in default culture #5452 coverage) pass in both modes
• TUnit.Core.SourceGenerator.Tests (117 tests) pass — source-gen unaffected
• TUnit.PublicAPI.Core_Library_Has_No_API_Changes snapshot test passes on net8.0, net9.0, net10.0, net472 with the accepted deltas
• dotnet build TUnit.slnx — 0 errors

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[claude]

Code Review

Overall this is a clean, well-scoped fix that correctly mirrors the CultureAttribute pattern established in #5463. The changes are minimal, targeted, and additive-only on the public API.

Core Change (HookExecutorAttribute.cs)

Minor style nit — inline ??= vs. named property accessor

CultureAttribute exposes its cached executor via a named property:

private CultureExecutor? _executor;
private CultureExecutor Executor => _executor ??= new CultureExecutor(cultureInfo);

The new HookExecutorAttribute.OnHookRegistered inlines the caching:

context.HookExecutor = _executor ??= (IHookExecutor)Activator.CreateInstance(_hookExecutorType)!;

This works, but extracting to a private property (private IHookExecutor Executor => _executor ??= (IHookExecutor)Activator.CreateInstance(_hookExecutorType)!;) would maintain consistency with the sibling pattern and slightly reduce noise in the hotpath method. Minor, but worth aligning for consistency.

Benign thread-safety note

The _executor ??= compound operation isn't atomically thread-safe — two threads could both observe null and both call Activator.CreateInstance. CultureAttribute has the same gap. Since IHookExecutor implementations are expected to be stateless (see GenericAbstractExecutor), the double-instantiation is harmless and wasted only one object allocation. Still, something to be aware of if a stateful executor is ever introduced.

ScopeType correctness

CultureAttribute uses ScopeType = typeof(ITestExecutor) (because it affects test execution primarily), while the new attribute uses typeof(IHookExecutor). This is semantically correct — HookExecutorAttribute only governs hooks — and ensures the two attributes don't compete for the same deduplication slot in ScopedAttributeFilter, which is the right behaviour.

---

Tests (HookExecutorHookTests.cs)

The bucket-per-executor-type isolation strategy in RecordingHookExecutorState is smart and avoids cross-fixture pollution. A few observations:

IsEqualTo(0) in MethodLevelOverride is load-bearing

var classLevelCount = RecordingHookExecutorState.Count(nameof(RecordingHookExecutor_F2ClassLevel));
await Assert.That(classLevelCount).IsEqualTo(0);

This relies on HookExecutorAttribute not wrapping test method bodies (only hooks). That's currently true, but the assertion would silently start failing if the engine ever extends HookExecutor scope to test methods. A comment explaining this invariant ("HookExecutorAttribute applies to lifecycle hooks only, not test method bodies") would protect future readers.

Static state never reset

RecordingHookExecutorState._invocations is never cleared between runs. If tests are retried (e.g., flake-retry tooling) the counts accumulate, making the >= N assertions still pass but masking a zero count on a re-run. Not a current issue, but a design smell to note.

>= N vs. exact count assertions

The assertions are intentionally loose (IsGreaterThanOrEqualTo) to tolerate parallel execution ordering. That's pragmatic given that CultureHookTests can use exact equality because it checks the current state, not a counter. The looseness is acceptable, but it means a regression where the executor is called too many times (e.g., double-registration) would go undetected. Not a blocking concern.

Assembly-level coverage

The fix claims to cover assembly-level cascading (same pipeline), but no assembly-level test exists. Since the mechanism is shared with class-level, this is a gap in test documentation rather than correctness, but a follow-up test would be a good addition.

---

Snapshot files

All four TFM snapshots (net8.0, net9.0, net10.0, net472) are correctly updated. The .Net4_7 snapshot omitting [DynamicallyAccessedMembers] on the constructor parameter is expected behaviour.

---

Summary

The fix is correct and well-implemented. The issues raised above are minor observations — no blockers. The AOT annotations, AttributeUsage correction, and adherence to the IScopedAttribute/IHookRegisteredEventReceiver pipeline are all done right.