fix(hooks): resolve TestDiscovery hook context type by attribute kind, not method name

[thomhurst]

Summary

• Source generator was deciding whether a [Before(TestDiscovery)] / [After(TestDiscovery)] hook expected BeforeTestDiscoveryContext vs TestDiscoveryContext by checking whether the method name contained "Before". Hooks named anything else (e.g. PlaywrightDebug) silently failed validation in HookMetadataGenerator.IsValidHookMethod and produced no source — no diagnostic, no error, just a missing hook at runtime. Fix: pass hookKind (from the [Before]/[After] attribute) through and key on it.
• Sister bug in GlobalTestHooksAnalyzer: TestSession and TestDiscovery hook levels were missing from the contextType switch, so the analyzer skipped them entirely — users with a wrong parameter type got neither codegen nor a diagnostic. Now validates all five hook levels and distinguishes Before vs After for TestDiscovery via HookType.

Other hook paths checked (ClassHooksAnalyzer, AssemblyTestHooksAnalyzer, InstanceTestHooksAnalyzer, ReflectionHookDiscoveryService) — only TestDiscovery has the asymmetric Before/After context types, so this was the only affected path.

Test plan

• TUnit.Core.SourceGenerator builds clean
• TUnit.Analyzers builds clean
• Source-gen snapshot test (TestDiscoveryHookTests) regenerated and passes
• Engine regression test with custom-named [Before(TestDiscovery)] hook taking BeforeTestDiscoveryContext passes in source-generated mode
• Same test passes in reflection mode (dual-mode parity)
• TUnit.TestProject builds clean

[claude]

Code Review

Overall: Clean, targeted bug fix with good test coverage. The root cause is correctly identified and fixed in both the source generator and analyzer. A few observations below.

---

What's done well

**Root cause fix in ** is the right approach. Removing in favour of passing through explicitly eliminates a fragile string-matching heuristic that would silently drop hooks whenever the method name didn't happen to contain the right substring. The new check is exhaustive and clear.

** extraction in ** is a genuine improvement. The old code had three separate switch expressions that all had to stay in sync — the new single method is the single source of truth for "which context type belongs to which hook level+kind combination". This also means the and gaps (previously falling through to ) are now closed in one place.

** on is correctly honoured** — both and set all three params to non-null on the path and null on the path, so the annotation is accurate.

---

Minor observation — test assertion gap

only asserts . The hook () was added to cover the regression for the after-hook side, but there's no assertion that . If the after hook is silently dropped (e.g. by a future regression), the test would still pass.

Consider adding:

This is low-risk since the after hook uses (the default path), but having both sides explicitly asserted makes the regression test self-documenting about what it's protecting.

---

Verified: no issues with handling

now maps → regardless of . Unlike , uses the same context type for before and after hooks, so this is correct.

---

Snapshot test

The updated correctly reflects the source-generated output for after the new regression test class was added. Nothing unexpected in the generated code.

---

Summary: The fix is correct and well-scoped. The only actionable item is the missing assertion for in the regression test — worth adding for completeness.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

Overall: Clean, targeted bug fix with good test coverage. The root cause is correctly identified and fixed in both the source generator and analyzer. A few observations below.

---

What's done well

Root cause fix in HookMetadataGenerator.cs is the right approach. Removing the method.Name.Contains("Before") heuristic in favour of passing hookKind through eliminates a fragile string-matching approach that silently dropped hooks whenever the method name did not contain the right substring. The new check hookKind is "Before" or "BeforeEvery" is exhaustive and clear.

GetExpectedContext extraction in GlobalTestHooksAnalyzer.cs is a genuine improvement. The old code had three separate switch expressions that all had to stay in sync; the new single method is the single source of truth for which context type belongs to which hook level + kind combination. This also closes the TestSession and TestDiscovery gaps (previously falling through to null) in one place.

[NotNullWhen(true)] on out HookType? is correctly honoured — both IsStandardHook and IsEveryHook set all three out params to non-null on the true path and null on the false path, so the annotation is accurate.

---

Minor observation — test assertion gap

The test only asserts CustomNamedBeforeDiscoveryCalls > 0. The [After(TestDiscovery)] hook (AnotherCustomNamedDiscoveryHook) was added to cover the regression on the after-hook side, but there is no assertion that CustomNamedAfterDiscoveryCalls > 0. If the after hook is silently dropped by a future regression, the test would still pass.

Consider adding:

await Assert.That(CustomNamedAfterDiscoveryCalls).IsGreaterThan(0);

This is low-risk since the after hook path already worked before this PR, but having both sides explicitly asserted makes the regression test self-documenting about what it protects.

---

Verified: TestSession handling is correct

GetExpectedContext maps HookLevel.TestSession to TestSessionContext regardless of hookType. Unlike TestDiscovery, TestSession uses the same context type for before and after hooks, so the absence of a Before/After branch there is intentional and correct.

---

Snapshot test

The updated .verified.txt correctly reflects the source-generated output for TestDiscoveryHookTests after the new regression test class was added. Nothing unexpected in the generated code.

---

Summary: The fix is correct and well-scoped. The only actionable item is the missing assertion for CustomNamedAfterDiscoveryCalls — worth adding for completeness, but not a blocker.

[claude]

Code Review (follow-up)

Previous feedback not yet addressed

The prior review noted that CustomNamedDiscoveryHooksWereRegisteredAndRan only asserts CustomNamedBeforeDiscoveryCalls > 0 and does not assert CustomNamedAfterDiscoveryCalls > 0. This is still unaddressed in the current diff.

[Test]
public async Task CustomNamedDiscoveryHooksWereRegisteredAndRan()
{
    await Assert.That(CustomNamedBeforeDiscoveryCalls).IsGreaterThan(0);
    // Missing: await Assert.That(CustomNamedAfterDiscoveryCalls).IsGreaterThan(0);
}

AnotherCustomNamedDiscoveryHook is the regression test for the [After(TestDiscovery)] side — if that hook were silently dropped by a future regression, the test would still pass. Adding the second assertion closes that gap and makes the test self-documenting about both sides it is protecting.

---

Aspire changes look correct

AspireTelemetryHooks.RegisterAspireExporter was itself a victim of the original bug: its method name didn't contain "Before", so the old heuristic would have silently dropped it from source-gen output. After the fix the source generator correctly identifies it as a Before hook and expects BeforeTestDiscoveryContext — dropping the parameter entirely is the right call (parameters are optional on hook methods).

The GetServiceName refactoring in TestTraceExporter.cs is an improvement:

• Respects the standard OTEL_SERVICE_NAME env var (correct OTel convention)
• Falls back to Assembly.GetEntryAssembly()?.GetName().Name (reasonable single-assembly default)

One minor note: in the old implementation, running with multiple assemblies fell back to "TUnit.Tests". Now it returns the entry assembly name regardless of assembly count. This is a better default, but worth documenting if this is a public-facing change.

---

Everything else remains sound

• IsValidHookMethod fix (hookKind is "Before" or "BeforeEvery") is correct and exhaustive.
• GetExpectedContext extraction is clean and covers all five hook levels.
• [NotNullWhen(true)] annotation on out HookType? is correctly honoured.
• Snapshot is consistent with the new test additions.

Blocking item: add await Assert.That(CustomNamedAfterDiscoveryCalls).IsGreaterThan(0); to the regression test before merging.

[claude]

Code Review (third pass)

Previous feedback still unaddressed (third occurrence)

Both prior reviews requested adding an assertion for CustomNamedAfterDiscoveryCalls to the regression test. It is still absent in the current diff.

[Test]
public async Task CustomNamedDiscoveryHooksWereRegisteredAndRan()
{
    await Assert.That(CustomNamedBeforeDiscoveryCalls).IsGreaterThan(0);
    await Assert.That(CustomNamedAfterDiscoveryCalls).IsGreaterThan(0); // still missing
}

AnotherCustomNamedDiscoveryHook (the [After(TestDiscovery)] hook) is the regression guard for the after-side of the original bug. Without the assertion, a future regression that silently drops that hook would leave the test green. Adding one line closes the gap and makes the test self-documenting about which specific path it is protecting.

---

Everything else remains correct

• IsValidHookMethod fix (hookKind is "Before" or "BeforeEvery") is correct and exhaustive.
• GetExpectedContext consolidation is clean — single source of truth for context-type lookup across all five hook levels.
• [NotNullWhen(true)] annotation on the new out HookType? parameter is accurately honoured on both the standard and every-hook paths.
• AspireTelemetryHooks.RegisterAspireExporter: dropping the unused BeforeTestDiscoveryContext parameter is the right call — the hook itself was a victim of the original bug (method name didn't contain "Before"), so the fix incidentally corrects its source-gen registration too.
• GetServiceName refactoring respects the standard OTEL_SERVICE_NAME env var and falls back to the entry assembly name, which is an improvement over the old multi-assembly heuristic.
• Snapshot is consistent with the added regression test class.

One remaining blocker: please add the CustomNamedAfterDiscoveryCalls assertion before merging.