fix: apply CultureAttribute and STAThreadExecutorAttribute to hooks (#5452)

[thomhurst]

Summary

Fixes #5452. CultureAttribute, STAThreadExecutorAttribute, and TestExecutorAttribute were only wired into the test-executor pipeline, so lifecycle hooks (Before/After for Test, Class, Assembly, Session) ignored the attribute entirely — a class-level [Culture("de-AT")] never influenced its own hooks.

Changes

Core infrastructure

• HookRegisteredContext gains a settable HookExecutor property — the hook-side equivalent of the existing Timeout pattern.
• HookMethod.HookExecutor keeps required init (so generated code can still assign via object initializer) plus an internal SetHookExecutor for engine mutation after construction.
• EventReceiverOrchestrator.InvokeHookRegistrationEventReceiversAsync applies the override back to the hook method after the receiver loop.

Attribute fix

• CultureAttribute, STAThreadExecutorAttribute, TestExecutorAttribute<T>, TestExecutorAttribute(Type) all now implement IHookRegisteredEventReceiver. Class/assembly/session hooks pick up the executor through OnHookRegistered; per-test hooks pick it up via a SetHookExecutor call in OnTestRegistered.
• Each attribute caches one executor instance and reuses it for both the test-executor and hook-executor roles.

Precedence bug uncovered by the change
Before this PR, BeforeTestHookMethod, AfterTestHookMethod, and HookTimeoutHelper all checked TestContext.CustomHookExecutor before the hook's own HookExecutor. Once CultureAttribute started calling SetHookExecutor, a class-level [Culture] would silently override an unrelated global [BeforeEvery(Test)] hook's own [HookExecutor<T>] attribute. Fixed so the hook's explicit executor wins; CustomHookExecutor is only used as a fallback when the hook is still on DefaultExecutor. This preserves the #2666 SetHookExecutor scenario for hooks that didn't declare their own executor.

Out-of-scope follow-up

#5462 tracks the inverse gap: HookExecutorAttribute placed at class or assembly level is still ignored by both discovery paths. Left for a separate change.

Test plan

• New CultureHookTests_ClassLevel, CultureHookTests_MethodLevelOverride, CultureHookTests_MethodLevelInheritsClass — cover class-level culture flowing to Before(Test) / After(Test) / Before(Class) and method-level override winning.
• Source-generated mode: full regression matrix (CultureTests, HookExecutorTests, SetHookExecutorTests, SetHookExecutorWithStaticHooksTests, new culture/hook tests) — 25/25 pass.
• Reflection mode: CultureHookTests_ClassLevel passes.
• CI runs full reflection regression matrix.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 1 complexity

Metric	Results
Complexity	1

View in Codacy

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

[claude]

Review

Good work on fixing the core issue — CultureAttribute, STAThreadExecutorAttribute, and TestExecutorAttribute now correctly propagate their executor to lifecycle hooks, and the precedence fix (hook's own explicit executor wins over CustomHookExecutor) is logically sound. The PR description is thorough and the commit is well-structured.

Critical bug: ReferenceEquals vs DefaultHookExecutor in reflection mode

This is the main concern. The precedence logic in BeforeTestHookMethod, AfterTestHookMethod, and HookTimeoutHelper checks:

if (ReferenceEquals(executor, DefaultExecutor.Instance) && context.CustomHookExecutor != null)

In source-gen mode this works, because generated code initialises HookExecutor = DefaultExecutor.Instance. But in reflection mode, ReflectionHookDiscoveryService.GetHookExecutor() returns new DefaultHookExecutor() — a completely separate class (internal class DefaultHookExecutor : IHookExecutor at line 1006 of ReflectionHookDiscoveryService.cs), not DefaultExecutor.Instance. The ReferenceEquals check therefore always returns false for reflection-discovered hooks, which means CustomHookExecutor (set via SetHookExecutor() — the #2666 scenario) is now completely ignored in reflection mode.

This affects the dual-mode requirement from the project rules. The fix could be to either:

1. Have ReflectionHookDiscoveryService.GetHookExecutor() return DefaultExecutor.Instance when there is no explicit [HookExecutor<T>] attribute on the hook method (instead of new DefaultHookExecutor()), or
2. Change the precedence check to be type-based (executor is DefaultExecutor or an IDefaultExecutor marker interface) rather than a singleton reference comparison.

Option 1 seems simpler and more consistent — DefaultHookExecutor in the reflection discovery service duplicates DefaultExecutor anyway (both just call action() for everything).

Minor: _afterClassCulture is captured but never asserted

CultureHookTests_ClassLevel._afterClassCulture is set in AfterClass() but there is no test method that asserts it equals "de-AT". The After(Class) path is therefore untested by this PR. Worth adding a Test_AfterClassHook_RanInClassCulture test symmetrical to the Before(Class) one.

Minor: InstanceHookMethod path in HookDelegateBuilder is inconsistent

CreateInstanceHookDelegateAsync still does if (context.CustomHookExecutor != null) without the ReferenceEquals guard (lines 565–586 of HookDelegateBuilder.cs). This means an explicit [HookExecutor<T>] on an instance Before/After test hook can be overridden by CustomHookExecutor — the opposite precedence from what BeforeTestHookMethod/AfterTestHookMethod now implement. Aligning this path with the same guard would make the behaviour consistent across both static and instance hooks.

Minor: lazy Executor property is not thread-safe

The private CultureExecutor? _executor; private CultureExecutor Executor => _executor ??= new CultureExecutor(cultureInfo); pattern (and the equivalent in STAThreadExecutorAttribute / TestExecutorAttribute) has a benign data race: two threads could both see _executor == null and create two instances. For the executors mentioned here (stateless, creating a fresh thread per call) this is harmless in practice. A comment acknowledging this (or a LazyInitializer.EnsureInitialized if you ever have a stateful executor) would prevent future confusion. The existing comments go some way to explaining the safety guarantee, so this is low priority.

Overall the design is clean and the approach of routing everything through IHookRegisteredEventReceiver is the right abstraction. The reflection-mode DefaultHookExecutor inconsistency is the only thing that needs addressing before merge.

[claude]

Follow-up review

Both issues flagged as critical in the previous review have been addressed.

Previously raised issues — status

Critical: ReferenceEquals vs DefaultHookExecutor in reflection mode — FIXED

ReflectionHookDiscoveryService.GetHookExecutor() now returns DefaultExecutor.Instance instead of new DefaultHookExecutor(), and the duplicate DefaultHookExecutor class has been removed. The ReferenceEquals(_hookExecutor, DefaultExecutor.Instance) guard in ResolveEffectiveExecutor now works correctly in both source-gen and reflection modes. Good cleanup.

Minor: InstanceHookMethod / HookDelegateBuilder inconsistency — FIXED

HookDelegateBuilder.cs no longer references CustomHookExecutor at all; the executor routing is now entirely owned by ResolveEffectiveExecutor.

Minor: _afterClassCulture is captured but never asserted — STILL MISSING

_afterClassCulture is set in AfterClass() but there is still no Test_AfterClassHook_RanInClassCulture test to assert it. The After(Class) code path therefore remains untested by this PR. Worth adding for symmetry with the Before(Class) coverage.

Minor: lazy Executor property thread-safety — acknowledged, not addressed (low priority)

The ??= pattern is still present; harmless for the current stateless executors, but a comment noting the benign-race assumption would future-proof the code.

New observations

HookMethod.ResolveEffectiveExecutor — clean design

Consolidating the "which executor wins" logic into a single method with an explicit comment about the precedence rule (DefaultExecutor → fallback to CustomHookExecutor) makes the semantics clear and the rule easy to change. The required init + internal SetHookExecutor duality on HookMethod.HookExecutor is a reasonable way to keep the source-gen interface stable while still allowing post-construction mutation from the engine.

TestExecutorAttribute refactoring — AOT safe

The refactoring from a primary constructor to a regular constructor correctly propagates [DynamicallyAccessedMembers(PublicConstructors)] to both the field and the constructor parameter, and Activator.CreateInstance(_type) is still covered by the annotation. No AOT regression.

CultureHookTests_MethodLevelOverride.BeforeTest comment

The test comment accurately describes the current semantics (method-level [Culture] overrides class-level for Before(Test) too), but this is an implicit coupling: BeforeTest will only ever be validated against fr-FR because the class has exactly one test. If a second test without [Culture("fr-FR")] were added, BeforeTest would fail. This is fine for a test fixture but worth a quick note in the comment to prevent confusion.

Summary

The critical issue from the previous round is fixed, the HookDelegateBuilder inconsistency is also resolved, and the overall design is sound. The only remaining item worth acting on before merge is adding the Test_AfterClassHook_RanInClassCulture assertion to verify the After(Class) hook also picks up the class-level culture.

[claude]

Third-round review (commit c182a7e)

All previously flagged issues have been addressed. Here's a status summary plus fresh eyes on the final state.

Issues from prior rounds — resolved

Issue	Status
ReferenceEquals broken in reflection mode (DefaultHookExecutor instead of DefaultExecutor.Instance)	Fixed — GetHookExecutor() now returns the singleton and the duplicate class is gone
InstanceHookMethod / HookDelegateBuilder inconsistent executor precedence	Fixed — routed through ResolveEffectiveExecutor, both static and instance hooks share one rule
After(Class) culture path untested	Fixed — AfterClass() now asserts CurrentCulture.Name == "de-AT" directly instead of capturing to a never-asserted field
Single-test invariant undocumented in CultureHookTests_MethodLevelOverride	Fixed — comment added
Lazy Executor property thread-safety (??= benign race)	Acknowledged; still present but harmless for stateless executors. No action needed

Final-state observations

HookMethod.ResolveEffectiveExecutor is the right design. A single method with an explicit comment about precedence (DefaultExecutor.Instance → fallback to CustomHookExecutor) is easy to reason about and easy to change. The required init + internal SetHookExecutor duality is consistent with the pre-existing Timeout mutation pattern in the same record.

TestExecutorAttribute (non-generic) AOT annotation. Moving the ctor parameter's [DynamicallyAccessedMembers] to a backing field and preserving it on the ctor parameter keeps the trimmer annotation chain intact. No regression.

_isHookExecutor caches IsAssignableFrom. Caching the interface check on construction rather than calling it per invocation is a small but correct optimisation.

Test coverage is now complete for the stated scenarios. Before(Class), Before(Test), After(Test), After(Class), method-level override, and class-level inheritance are all exercised. The After(Class) path correctly asserts inline since no subsequent test can read a captured value.

Verdict

Code is clean, the bug is correctly fixed, precedence semantics are well-defined, and the dual-mode requirement is satisfied. Ready to merge.

[claude]

Fourth-round review (commit 4e35d71 → current)

All issues flagged across the three prior rounds are fully resolved. This is a final-state review.

Issues from all prior rounds — status

Issue	Round raised	Status
ReferenceEquals vs new DefaultHookExecutor() in reflection mode	Round 1 (critical)	Fixed — GetHookExecutor() now returns DefaultExecutor.Instance
_afterClassCulture captured but never asserted	Round 1	Fixed — AfterClass() asserts inline now
HookDelegateBuilder still had its own CustomHookExecutor branch	Round 2	Fixed — deleted, all paths go through ResolveEffectiveExecutor
??= lazy executor field not thread-safe	Round 1	Acknowledged as benign (stateless executors) — no action needed

---

Architecture quality

HookMethod.ResolveEffectiveExecutor is exactly the right place for precedence logic. Moving it into the hook model rather than scattering the check across BeforeTestHookMethod, AfterTestHookMethod, InstanceHookMethod, and HookTimeoutHelper eliminates four independent (and previously inconsistent) enforcement points. The ReferenceEquals(DefaultExecutor.Instance) sentinel pattern is clean and correct for both source-gen and reflection modes since the reflection path was fixed to return the same singleton.

The deletion of DefaultHookExecutor (a duplicate of DefaultExecutor) and HookTimeoutHelper.ExecuteHookWithPotentialCustomExecutor is good hygiene — the removed code was already superseded by the centralized approach.

Test coverage

The three-class test structure in CultureHookTests.cs is well-designed:

• CultureHookTests_ClassLevel: covers Before(Class), After(Class), Before(Test), After(Test), and the test body all running under the class-level culture.
• CultureHookTests_MethodLevelOverride: a single test with [Culture("fr-FR")] inside a [Culture("de-AT")] class verifies that the most-specific executor wins and that Before(Test) sees the method-level culture. The comment explaining the single-test invariant is good — it prevents future contributors from naively adding tests to the class and breaking the assertion.
• CultureHookTests_MethodLevelInheritsClass: the no-override case, keeping inheritance clean.

No new concerns

The public API snapshots are updated consistently across all four target frameworks. The HookRegisteredContext.HookExecutor { get; set; } addition follows the existing Timeout property pattern exactly — no API design concern there.

---

Ready to merge. The bug is correctly fixed, the precedence semantics are well-defined and centralized, dual-mode correctness is verified (source-gen and reflection both use DefaultExecutor.Instance as the sentinel), and the regression tests cover the failure modes. No blocking issues.

[claude]

Fifth-round review (final state: commit 0b6ad1be)

The latest commit is a pure public API snapshot update for all four target frameworks (net8.0, net9.0, net10.0, net4.7), correctly reflecting the additions from this PR:

• HookRegisteredContext.HookExecutor { get; set; }
• OnHookRegistered(HookRegisteredContext) on CultureAttribute, STAThreadExecutorAttribute, TestExecutorAttribute, and TestExecutorAttribute<T>
• The additional IHookRegisteredEventReceiver interface in the attribute class signatures

All four snapshot files are updated consistently. No concerns.

Status of all prior issues

Issue	Status
ReferenceEquals vs new DefaultHookExecutor() in reflection mode	Fixed
_afterClassCulture captured but never asserted	Fixed
HookDelegateBuilder inconsistent executor precedence	Fixed
Single-test invariant undocumented in CultureHookTests_MethodLevelOverride	Fixed
Lazy Executor ??= benign thread race	Acknowledged as harmless, no action needed

No new concerns introduced by this commit. The PR is clean and ready to merge.

[claude]

Sixth-round review (no new commits since round 5)

The last commit on this PR is still 0b6ad1be (public API snapshot update), which was fully reviewed in round 5. There are no new changes to evaluate.

Status of all tracked issues

Issue	Round raised	Status
ReferenceEquals vs new DefaultHookExecutor() in reflection mode	Round 1 (critical)	Fixed
_afterClassCulture captured but never asserted	Round 1	Fixed
HookDelegateBuilder inconsistent executor precedence	Round 2	Fixed
Single-test invariant undocumented in CultureHookTests_MethodLevelOverride	Round 2	Fixed
Lazy Executor ??= benign thread race	Round 1	Acknowledged as harmless — no action needed

No new commits, no new concerns. The PR is clean and all previously raised issues are resolved. Ready to merge.

[claude]

Sixth-round review (no new commits since round 5)

The last commit on this PR is still 0b6ad1be (public API snapshot update), which was fully reviewed in round 5. There are no new changes to evaluate.

Status of all tracked issues

Issue	Round raised	Status
ReferenceEquals vs new DefaultHookExecutor() in reflection mode	Round 1 (critical)	Fixed
_afterClassCulture captured but never asserted	Round 1	Fixed
HookDelegateBuilder inconsistent executor precedence	Round 2	Fixed
Single-test invariant undocumented in CultureHookTests_MethodLevelOverride	Round 2	Fixed
Lazy Executor ??= benign thread race	Round 1	Acknowledged as harmless, no action needed

No new commits, no new concerns. The PR is clean and all previously raised issues are resolved. Ready to merge.

[claude]

Sixth-round review (no new commits since round 5): The last commit on this PR is still 0b6ad1b (public API snapshot update), fully reviewed in round 5. No new changes to evaluate. All previously raised issues are resolved. Ready to merge.

[claude]

Review

This is a well-executed bug fix for a real user pain point (class-level [Culture] / [STAThread] being ignored by hooks). I've done an independent review of the full diff. Previous review rounds have already caught and resolved the significant issues, so I'll focus on confirming correctness and flagging anything remaining.

What the PR does well

Precedence logic is correct. ResolveEffectiveExecutor in HookMethod establishes a clean three-tier priority:

1. Explicit [HookExecutor<T>] on the hook method (_hookExecutorIsExplicit) — always wins
2. TestContext.CustomHookExecutor (set per-test via OnTestRegistered, scope-filtered) — most-specific for the test
3. _hookExecutor set via OnHookRegistered — class/assembly fallback

This correctly handles the method-level override case documented in CultureHookTests_MethodLevelOverride.

Executor caching is correct. The ??= lazy initialization in CultureAttribute, STAThreadExecutorAttribute, and TestExecutorAttribute<T> ensures at most one executor instance is created per attribute instance. The benign double-initialization race under parallel registration was acknowledged in prior reviews and is genuinely harmless since all races produce functionally identical executor instances.

DefaultExecutor.Instance sentinel comparison is correct. The reflection path now returns DefaultExecutor.Instance instead of new DefaultHookExecutor(), making the ReferenceEquals check in _hookExecutorIsExplicit reliable across both source-gen and reflection modes. This is a critical correctness fix.

HookMethod is a record — private fields copy correctly. The _hookExecutor and _hookExecutorIsExplicit private fields will be included in the compiler-generated copy constructor, so with expressions on hook records correctly propagate the explicit flag.

Class/assembly/session hooks use HookExecutor (not ResolveEffectiveExecutor). This is correct: those hooks have no TestContext to pass, and their executor is set during hook registration by OnHookRegistered via SetHookExecutor. BeforeClassHookMethod, AfterClassHookMethod, BeforeAssemblyHookMethod, AfterAssemblyHookMethod, BeforeTestSessionHookMethod, and AfterTestSessionHookMethod all call HookExecutor.Execute*, which reads _hookExecutor — the value written by SetHookExecutor. This path is correct.

HookTimeoutHelper simplification is clean. Removing ExecuteHookWithPotentialCustomExecutor and the type-switch on BeforeTestHookMethod/AfterTestHookMethod/InstanceHookMethod eliminates duplicated executor-resolution logic. The hook's own ExecuteAsync now owns the resolution.

DefaultHookExecutor removal is clean. It was a local-only no-op implementation. DefaultExecutor.Instance (via GenericAbstractExecutor) serves the same role as a singleton, eliminating an allocation per hook registration.

One minor observation

TestExecutorAttribute (the non-generic variant) calls Activator.CreateInstance(_type) in Executor, which is a reflection-based instantiation. The [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] annotation on _type correctly propagates the AOT hint from the constructor parameter, so trimming will preserve the required constructors. No issue here.

Test coverage

The three new CultureHookTests_* classes cover the main scenarios:

• Class-level [Culture] flowing to Before(Class), After(Class), Before(Test), After(Test), and the test body
• Method-level [Culture] overriding the class-level for Before(Test) (with the single-test constraint documented inline)
• Inheritance of class-level culture when no method override is present

SetHookExecutorTests and SetHookExecutorWithStaticHooksTests remain intact and verify the #2666 fallback path continues to work.

Summary

The implementation is correct, AOT-safe, dual-mode compliant, and the test coverage is adequate. All issues from prior review rounds have been addressed. This is ready to merge.