feat: per-hook activity spans with method names

[thomhurst]

Summary

• Each hook method now gets its own dedicated OpenTelemetry activity/span named "hook: ClassName.MethodName(params)" instead of a single generic "hook: BeforeTest" / "hook: AfterTest" wrapping all hooks
• Introduced NamedHookDelegate<TContext> to carry hook names through the delegate pipeline from HookDelegateBuilder to HookExecutor
• Applies to all hook levels: test session, assembly, class, and test hooks

Before

test case
  └─ hook: BeforeTest
  └─ test body
  └─ hook: AfterTest

After

test case
  └─ hook: MySetup.Initialize(context)
  └─ hook: BaseClass.Setup(context)
  └─ test body
  └─ hook: MyClass.Cleanup(context)
  └─ hook: GlobalHooks.AfterEvery(context)

Test plan

• Verify build passes across all target frameworks
• Run tests with an OpenTelemetry collector to confirm individual hook spans appear with correct names
• Verify session/assembly/class-level hooks also produce named spans

[claude]

Code Review: feat/per-hook-activity-spans

Great feature — granular per-hook spans are significantly more useful for debugging slow/failing hooks in distributed traces. The overall design is solid and the centralization of activity lifecycle in HookExecutor.ExecuteHookWithActivityAsync is the right call.

---

Issue 1 (Important): Unsafe cast in SortAndAddHooks — AOT risk

HookDelegateBuilder.cs, ~line 144–157

private static void SortAndAddHooks<TDelegate>(
    List<NamedHookDelegate<TestContext>> target,
    List<(int order, int registrationIndex, TDelegate hook)> hooks)
    where TDelegate : struct
{
    ...
    target.Add((NamedHookDelegate<TestContext>)(object)hook); // ⚠️ boxing struct through object
}

This is always called where TDelegate = NamedHookDelegate<TestContext>, making the generic parameter fake. Boxing a struct through object to cast it is an AOT red flag — under NativeAOT, generic instantiations over value types have strict requirements, and this double-cast bypasses compile-time verification.

Recommended fix: Drop the generic entirely, since it is never actually generic:

private static void SortAndAddHooks(
    List<NamedHookDelegate<TestContext>> target,
    List<(int order, int registrationIndex, NamedHookDelegate<TestContext> hook)> hooks)
{
    hooks.Sort(...);
    foreach (var (_, _, hook) in hooks)
        target.Add(hook);
}

This mirrors SortAndAddClassHooks which is already correctly typed and cast-free. Unifying the two also removes the confusing asymmetry.

---

Issue 2 (Minor): Assembly hook sorting drops registrationIndex — non-deterministic ordering

HookDelegateBuilder.cs, BuildBeforeAssemblyHooksAsync / BuildAfterAssemblyHooksAsync

return allHooks
    .OrderBy(h => h.order)  // ← missing .ThenBy(h => h.registrationIndex)
    .Select(h => h.hook)
    .ToList();

Every other build path (including BuildGlobalHooksAsync) uses a secondary sort on registrationIndex:

.OrderBy(static h => h.order)
.ThenBy(static h => h.registrationIndex)

Without the ThenBy, assembly hooks registered at the same Order value will have non-deterministic execution order across runs (LINQ's OrderBy is stable per-run but the input order could vary). This is likely a latent bug carried in from before this PR, but since you're touching these methods it's worth fixing now.

---

Issue 3 (Minor): CreateXXXHookDelegateAsync should return ValueTask<> not Task<>

Per project rules, ValueTask should be used for operations that are often synchronous. ProcessHookRegistrationAsync returns immediately on the fast path (cached hook — !_processedHooks.TryAdd is false). This means most call sites after warm-up are effectively synchronous, and wrapping them in Task<> allocates a state machine unnecessarily.

// Before
private async Task<NamedHookDelegate<TestContext>> CreateInstanceHookDelegateAsync(...)

// After
private async ValueTask<NamedHookDelegate<TestContext>> CreateInstanceHookDelegateAsync(...)

(Same for CreateStaticHookDelegateAsync, CreateClassHookDelegateAsync, etc.)

The callers (BuildGlobalHooksAsync's createDelegate parameter) would need Func<THookMethod, ValueTask<NamedHookDelegate<TContext>>> — a small but consistent change.

---

Issue 4 (Nitpick): Array.Empty<Exception>() vs [] inconsistency

HookExecutor.cs, ExecuteAfterTestHooksAsync

return exceptions == null ? Array.Empty<Exception>() : exceptions;

The rest of the file uses []. This is a trivial style inconsistency but the modern form is cleaner:

return exceptions ?? [];

---

What's working well

• NamedHookDelegate<TContext> as readonly record struct — value semantics with zero heap allocation for the wrapper itself. Correct choice for a hot path.
• #if NET / #else dual implementation of ExecuteHookWithActivityAsync — clean gating with no runtime overhead on older targets.
• BuildGlobalHooksAsync generic helper — eliminates substantial duplication across 10 global hook variants.
• Removing Activity management from TestExecutor — the 55-line reduction is exactly the right kind of cleanup; TestExecutor had no business knowing about spans.
• Per-hook span naming (hook: ClassName.MethodName(params)) — the before/after semantics are now actually traceable in distributed tracing tools.

---

Overall this is a clean, well-scoped improvement. The unsafe cast in SortAndAddHooks is the most important thing to fix before merging given the AOT compatibility requirement.

[claude]

Code Review: feat/per-hook-activity-spans

This is a well-executed PR that improves OpenTelemetry observability and significantly reduces code complexity at the same time.

What's good

**** is a minimal, well-chosen abstraction. Pre-computing in the constructor avoids a string allocation on every hook invocation, which aligns with the Performance First principle. Using a readonly record struct is idiomatic for this kind of lightweight carrier type.

The consolidation of five near-identical CreateXxxHookDelegateAsync methods into one generic CreateStaticHookDelegateAsync<TContext> is a meaningful reduction. The five methods were byte-for-byte identical except for the type parameter — this was exactly the kind of duplication that warranted a generic.

Removing the unsafe boxing cast in the old SortAndAddHooks:

// Old: unsafe cast through object
target.Add((Func<TestContext, CancellationToken, Task>)(object)hook);

This was a latent type-safety hazard. The new unified SortAndAddHooks<TContext> is clean and type-safe.

Moving GetOrCreateAssemblyContext / GetOrCreateClassContext outside the foreach loops is a nice micro-optimisation — same context object every iteration, no need to look it up repeatedly.

The #if NET split on ExecuteHookWithActivityAsync is consistent with the existing pattern in this codebase and correctly means non-NET targets get zero overhead.

---

One observation worth considering

The Name property on NamedHookDelegate<TContext> is set at construction time but never read by any consumer after that — only ActivityName and Invoke are accessed in HookExecutor. The Name field currently exists solely to build ActivityName in the convenience constructor.

// NamedHookDelegate: Name is used only to build ActivityName
public NamedHookDelegate(string name, Func<TContext, CancellationToken, Task> invoke)
    : this(name, $"hook: {name}", invoke) { }

This isn't a bug — it's harmless and keeping Name gives a clean escape hatch for future logging or error messages. But if you want to be strict about YAGNI, the struct could drop Name entirely and just take activityName directly. Worth a moment's thought, though I wouldn't block on it.

---

Behavioral note (not a problem, just worth documenting)

The old code had an explicit ActivityStatusCode.Error set on the aggregate "hook: AfterTest" span when hookExceptions.Count > 0. That aggregate span is gone. The new per-hook spans correctly record exceptions via the catch block in ExecuteHookWithActivityAsync — but only for hooks that throw. Since ExecuteAfterTestHooksAsync catches exceptions per hook internally, each failing hook's span will still be marked as failed via TUnitActivitySource.RecordException. The net result is actually better observability (you can see which hook failed rather than just that some after-hook failed), but if anyone was relying on the aggregate span for alerting, that span no longer exists.

---

Summary

Net -141 lines of code, better observability, eliminated an unsafe cast, and no meaningful trade-offs. Clean PR.

[claude]

Code Review: feat/per-hook-activity-spans

This is a well-executed PR that improves OpenTelemetry observability and significantly reduces code complexity at the same time.

What's good

NamedHookDelegate<TContext> is a minimal, well-chosen abstraction. Pre-computing ActivityName in the constructor avoids a string allocation on every hook invocation, which aligns with the Performance First principle. Using a readonly record struct is idiomatic for this kind of lightweight carrier type.

The consolidation of five near-identical CreateXxxHookDelegateAsync methods into one generic CreateStaticHookDelegateAsync<TContext> is a meaningful reduction. The five methods were byte-for-byte identical except for the type parameter — this was exactly the kind of duplication that warranted a generic.

Removing the unsafe boxing cast in the old SortAndAddHooks:

// Old: unsafe cast through object
target.Add((Func<TestContext, CancellationToken, Task>)(object)hook);

This was a latent type-safety hazard. The new unified SortAndAddHooks<TContext> is clean and type-safe.

Moving GetOrCreateAssemblyContext / GetOrCreateClassContext outside the foreach loops is a nice micro-optimisation — same context object every iteration, no need to look it up repeatedly.

The #if NET split on ExecuteHookWithActivityAsync is consistent with the existing pattern in this codebase and correctly means non-NET targets get zero overhead.

---

One observation worth considering

The Name property on NamedHookDelegate<TContext> is set at construction time but never read by any consumer after that — only ActivityName and Invoke are accessed in HookExecutor. The Name field currently exists solely to build ActivityName in the convenience constructor.

This isn't a bug — it's harmless and keeping Name gives a clean escape hatch for future logging or error messages. But if you want to be strict about YAGNI, the struct could drop Name entirely and just take activityName directly. Worth a moment's thought, though I wouldn't block on it.

---

Behavioral note (not a problem, just worth documenting)

The old code had an explicit ActivityStatusCode.Error set on the aggregate "hook: AfterTest" span when hookExceptions.Count > 0. That aggregate span is gone. The new per-hook spans correctly record exceptions via the catch block in ExecuteHookWithActivityAsync — but only for hooks that throw. Since ExecuteAfterTestHooksAsync catches exceptions per hook internally, each failing hook's span will still be marked as failed via TUnitActivitySource.RecordException. The net result is actually better observability (you can see which hook failed rather than just that some after-hook failed), but if anyone was relying on the aggregate span for alerting, that span no longer exists.

---

Summary

Net -141 lines of code, better observability, eliminated an unsafe cast, and no meaningful trade-offs. Clean PR.

[claude]

Code Review: feat/per-hook-activity-spans

This is a well-executed refactor. The feature goal (per-hook named spans) is achieved cleanly, and the implementation brings some meaningful secondary improvements along the way. A few things worth calling out:

---

Positive changes

Context acquisition moved outside the loop (HookExecutor.cs)
Previously GetOrCreateAssemblyContext(assembly) and GetOrCreateClassContext(testClass) were called on every loop iteration inside ExecuteAfterAssemblyHooksAsync / ExecuteAfterClassHooksAsync. The new code calls them once before the loop. This is a subtle correctness improvement — previously if the context factory had any side-effects or returned different instances, loop iterations could diverge. Now it's consistent.

Delegate factory unification
Removing five separate CreateXxxHookDelegateAsync methods in favour of a single generic CreateStaticHookDelegateAsync<TContext> is a great simplification. The old specializations were pure boilerplate — all structurally identical.

NamedHookDelegate pre-computes ActivityName
Storing "hook: {Name}" at construction time rather than formatting on every hook invocation is the right trade-off for a hot path. Good call.

exceptions ?? [] cleanup — clean modern C# idiom over Array.Empty<Exception>().

---

Issues to investigate

1. Dead #else branch in ExecuteHookWithActivityAsync

#if NET
    private static async ValueTask ExecuteHookWithActivityAsync<TContext>(...) { ... }
#else
    private static async ValueTask ExecuteHookWithActivityAsync<TContext>(...)
    {
        await hook.Invoke(context, cancellationToken).ConfigureAwait(false);
    }
#endif

TUnit targets net8.0;net9.0;net10.0 — all of which define NET. The #else branch will never be compiled under the current build matrix. The original TestExecutor.cs used #if NET guards too, but in this case since you're wrapping the entire method, consider just removing the #else branch entirely and leaving only the #if NET path, or removing the conditional entirely if there's no plan to ever support non-NET targets.

2. NamedHookDelegate — consider whether ActivityName is worth its own property

The struct currently stores both Name and ActivityName (always $"hook: {Name}"). If Name is only ever used for logging/display and ActivityName for span creation, having two separate string properties on the struct is fine. But if there are future cases where the activity name might differ from "hook: " + name, the dual-property design makes that easy. The secondary constructor that defaults ActivityName is a nice safety valve. No change needed — just confirming the design intent is sound.

3. Error status on after-test hook failures (confirmed non-regression ✅)

The old TestExecutor.cs code explicitly called afterTestActivity.SetStatus(ActivityStatusCode.Error) when hookExceptions.Count > 0. Checking TUnitActivitySource.RecordException: it calls activity.SetStatus(ActivityStatusCode.Error, exception.Message) internally, so the new per-hook catch block in ExecuteHookWithActivityAsync correctly propagates error status. No regression here — if anything the new per-hook spans give more precise error attribution.

---

Minor

• The BuildGlobalBeforeTestSessionHooksAsync and similar methods previously called CreateTestSessionHookDelegateAsync (a distinct method) but now call CreateStaticHookDelegateAsync. Worth confirming the removed specializations didn't have any unique logic (e.g. extra processing steps) — from the diff they look identical, but a quick grep to confirm no behavioural difference is worthwhile before merging.

---

Summary

The refactor cleanly threads hook names through the pipeline without exposing them to public API surfaces, and NamedHookDelegate<TContext> is a minimal, appropriately-scoped abstraction. The per-hook span granularity is a genuine observability improvement. The dead #else branch is the only thing I'd clean up before merging.

[thomhurst]

Thanks for the reviews! Here's what was addressed across the last two commits:

Already fixed before the reviews (simplification commit):

• Unsafe boxing cast in SortAndAddHooks — unified into single generic SortAndAddHooks<TContext>
• Five duplicate CreateXxxHookDelegateAsync → single generic CreateStaticHookDelegateAsync<TContext>
• Redundant GetOrCreateAssemblyContext/GetOrCreateClassContext calls moved outside loops
• Pre-computed ActivityName in NamedHookDelegate to avoid per-invocation string allocation

Fixed in response to review feedback:

• Added missing .ThenBy(registrationIndex) to assembly hook sorting (latent bug fix)
• Changed Task<> → ValueTask<> for create delegate methods
• Array.Empty<Exception>() → [] for consistency

Kept as-is (with reasoning):

• Name property on NamedHookDelegate — kept as escape hatch for future logging/error messages
• #else branch in ExecuteHookWithActivityAsync — not dead code; Library.props targets netstandard2.0;net8.0;net9.0;net10.0, so the #else branch compiles for netstandard2.0 where NET is not defined
• Removed specializations (CreateTestSessionHookDelegateAsync, etc.) had no unique logic — confirmed identical to the generic version