fix: deduplicate virtual hook overrides across class hierarchy (#5428)

[thomhurst]

Summary

• Fixes [Bug]: Virtual hook methods are called for base classes and descendants #5428: a virtual [Before(Test)]/[After(Test)] method overridden in a derived class executed twice.
• Both base and derived registrations invoked the same override via virtual dispatch. HookDelegateBuilder now dedupes hooks by MethodInfo.GetBaseDefinition() while walking the hierarchy from derived to base.
• Adds regression test VirtualHookOverrideTests.

Test plan

• New test VirtualHookOverrideTests.DerivedTestClass.Override_Should_Run_Once fails before fix, passes after
• Existing HookExecutionOrderTest (separate-method hierarchy) still passes

[claude]

Code Review

Good fix for a subtle virtual dispatch bug. The core logic is correct — walking the hierarchy from derived to base and using GetBaseDefinition() to detect already-seen overrides is the right approach. Here are a few things worth considering:

Issues

1. Protected virtual hooks are silently not deduplicated

IsOverriddenByMoreDerivedHook uses BindingFlags.Public only:

var methodInfo = declaringType.GetMethod(
    hook.MethodInfo.Name,
    BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly,
    ...);

If a user writes protected virtual / protected override hook methods, GetMethod returns null, the guard returns false, and both registrations still fire. The bug from #5428 reappears silently.

This is partially constrained by the [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] annotation on InstanceHookMethod.ClassType, which would require adding NonPublicMethods to safely extend this to protected methods under AOT. That change to the annotation would be the right companion fix, or at minimum the limitation should be documented.

2. Architectural smell: reflection lookup to rediscover what's already known

The core awkwardness is that InstanceHookMethod.MethodInfo is a MethodMetadata (a framework-level DTO), not a System.Reflection.MethodInfo. So IsOverriddenByMoreDerivedHook has to reverse-lookup the real MethodInfo from the declaring type by name + parameter types — which is exactly the kind of fragile string-based reflection that can break with overloads, explicit interface implementations, or non-public signatures.

A more robust architecture would store the resolved MethodInfo.GetBaseDefinition() token directly on InstanceHookMethod (or on MethodMetadata), either at registration time (source-gen or reflection discovery), or lazily cached. That would make the deduplication O(1) set lookup with no secondary reflection:

// In InstanceHookMethod / HookMethod:
public MethodInfo? BaseDefinition { get; init; }

And deduplication becomes simply:

if (hook.BaseDefinition is not null && !seenBaseDefinitions.Add(hook.BaseDefinition))
    continue;

3. Source-gen mode not covered (CLAUDE.md dual-mode rule)

The fix is in TUnit.Engine/Services/HookDelegateBuilder.cs (reflection path only). Per the project rules, changes to core hook metadata collection must work in both modes. Does the source-generator register virtual hook overrides with separate entries too? If so, the source-gen path has the same double-execution bug and this fix won't help users running in source-gen mode.

Minor observations

• The regression test asserts via throw new Exception(...) rather than using TUnit's assertion APIs — fine for a regression test, but using Assert.That(SetupCallCount).IsEqualTo(1) would be more idiomatic.
• Before(Class)/After(Class) hooks are intentionally excluded from this fix since they use CreateStaticHookDelegateAsync (static methods can't be virtual in this sense). That's correct and doesn't need changing.
• The duplicated seenBaseDefinitions guard in both sourceHooks and openTypeHooks paths within each method is correct — the same HashSet spans both loops within a single type level, which is the right behavior.

Summary

The fix correctly solves the reported issue for the common case (public virtual hooks). The two areas that deserve a follow-up are: (1) extending coverage to protected virtual hooks (with the corresponding DAM annotation update), and (2) verifying or fixing the source-gen path. The longer-term architectural improvement of storing the base definition on InstanceHookMethod would make this whole class of bugs easier to reason about and prevent.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 12 complexity

Metric	Results
Complexity	12

View in Codacy

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

[claude]

Follow-up Review (after second commit 24d769)

The second commit addresses the three main points from the previous review. Here is a full assessment of the current state.

What was addressed

Pre-resolving BaseDefinition in the reflection path — done correctly. ReflectionHookDiscoveryService now calls method.GetBaseDefinition() at registration time and stores it on InstanceHookMethod.BaseDefinition. The engine prefers this stored value and only falls back to the name+parameter reflection lookup for the source-gen path. This is the right architecture — the reflection path now pays essentially zero extra cost at dedup time.

TUnit assertions in the regression test — done. The test now uses await Assert.That(SetupCalls).IsEqualTo(1) and also covers TeardownAsync with an override and a counter. Good.

[EditorBrowsable(EditorBrowsableState.Never)] on BaseDefinition — good defensive choice to keep framework-internal surface out of user-visible IntelliSense.

---

Remaining issues worth noting

1. TeardownCalls is never asserted

The regression test increments TeardownCalls in TeardownAsync and asserts SetupCalls == 1, but there is no corresponding assertion for TeardownCalls. If the After(Test) dedup regressed, the test would not catch it. Adding await Assert.That(TeardownCalls).IsEqualTo(1) in Override_Should_Run_Once (or a dedicated second test method) would complete the coverage.

2. ResolveBaseDefinition is the live path for source-gen mode — and it uses BindingFlags.DeclaredOnly

The comment says the fallback runs for source-gen. But BindingFlags.DeclaredOnly means: if the source-gen registered the hook under BaseTestClass (because the [Before(Test)] attribute lives on the base method), ResolveBaseDefinition is called with hook.ClassType == BaseTestClass. On that type, DeclaredOnly will find the method — that's fine. But if the registered type is the derived class and the attribute was inherited/re-declared, DeclaredOnly correctly finds the override. The logic is sound for a single level of inheritance.

However, consider a three-level hierarchy: A (base, [Before(Test)] virtual void Foo()), B : A ([Before(Test)] override void Foo()), C : B ([Before(Test)] override void Foo()). Walking from C to A:

• C hook: ResolveBaseDefinition finds C.Foo, GetBaseDefinition() → A.Foo. Added to seenBaseDefinitions. ✓
• B hook: ResolveBaseDefinition finds B.Foo, GetBaseDefinition() → A.Foo. Already seen → skipped. ✓
• A hook: ResolveBaseDefinition finds A.Foo, GetBaseDefinition() → A.Foo. Already seen → skipped. ✓

This actually works correctly. The GetBaseDefinition() contract guarantees all overrides in a chain share the same root definition, so the HashSet correctly deduplicates the entire chain.

3. The [EditorBrowsable] attribute is redundant for BaseDefinition

InstanceHookMethod is itself decorated with [EditorBrowsable(EditorBrowsableState.Never)] at the class level (in non-DEBUG builds, via the inherited #if !DEBUG on HookMethod). Adding [EditorBrowsable] to an individual property of an already-hidden type is harmless but redundant. Not a bug — just noise.

4. The MethodInfo? BaseDefinition property is on InstanceHookMethod in TUnit.Core

TUnit.Core is the public API surface for test authors and is supposed to be AOT-compatible. Adding System.Reflection.MethodInfo (which is not AOT-friendly for general use) to a public record in TUnit.Core is a minor API surface concern. It's annotated [EditorBrowsable(Never)] which helps, but it does widen the DAM/AOT exposure. Longer-term, this could move to an internal sibling type or be wrapped with #if REFLECTION_BASED, but it's a minor concern and not a blocker given the [EditorBrowsable] guard.

---

Summary

The second commit genuinely improves the implementation — the pre-resolved BaseDefinition eliminates the main architectural smell from the first version, and the dedup logic is correct for all depths of virtual override chains. The primary actionable gap is the missing TeardownCalls assertion. Everything else is either working correctly or a cosmetic concern. The fix is solid and ready to merge once that assertion gap is addressed.

[claude]

Follow-up Review (third commit: 94a7a28)

The third commit adds [EngineTest(ExpectedResult.Pass)] to the regression test — a correct fix to ensure the test is executed by the engine harness and its result is validated.

Outstanding gap from previous review

TeardownCalls is still never asserted.

TeardownAsync increments TeardownCalls and After(Test) deduplication is separately applied in BuildAfterTestHooksAsync, but there is no assertion in Override_Should_Run_Once (or any other test method) that checks TeardownCalls == 1. If the After(Test) dedup regressed, no test would catch it.

The simplest fix — add one line to the existing test:

[Test]
[EngineTest(ExpectedResult.Pass)]
public async Task Override_Should_Run_Once()
{
    await Assert.That(SetupCalls).IsEqualTo(1);
    await Assert.That(TeardownCalls).IsEqualTo(1); // add this
}

Note: TeardownCalls will be 0 at the time the test body runs (the After hook fires after the test), so to assert it correctly the count should be checked in a subsequent After(Test) hook or via a dedicated counter-assertion hook. The cleanest approach is a separate [After(Test)] assertion method in DerivedTestClass:

[After(Test)]
public async Task AssertTeardownRunsOnce()
{
    await Assert.That(TeardownCalls).IsEqualTo(1);
}

This would run after the TeardownAsync override, validating it fired exactly once.

Everything else is in good shape

All three substantive points from the first review were addressed in commit 24d769:

• Pre-resolved BaseDefinition eliminates the name+params reverse-lookup for the reflection path.
• TUnit assertions are used throughout.
• Three-level virtual chain dedup is correct (verified by reasoning through GetBaseDefinition() contract).

The fix is correct, well-structured, and ready to merge once the TeardownCalls coverage gap is closed.

[claude]

Follow-up Review (commits 68fd235 and 1d680db)

This is a final review covering the two remaining commits since the last round of feedback.

What changed

Commit 68fd235 — adds the AfterTeardownAssertion hook with Order = int.MaxValue to assert TeardownCalls == 1 after TeardownAsync fires. This closes the last substantive gap called out in the previous review: the After(Test) deduplication path now has an assertion that will catch a regression.

Commit 1d680db — updates the public API snapshot files for all target frameworks (net8.0, net9.0, net10.0, and net4.7) to include MethodInfo? BaseDefinition { get; init; } on InstanceHookMethod. This is the correct snapshot hygiene step — the verified files are committed (not .received.txt), satisfying the snapshot testing rule in CLAUDE.md.

Assessment

All three points from the original review and the single outstanding gap from the second/third review rounds are now resolved:

1. Pre-resolved BaseDefinition on the reflection path — done in commit 24d769.
2. TUnit assertion idioms in the regression test — done in commit 24d769.
3. TeardownCalls assertion for the After(Test) dedup path — done in commit 68fd235, using [After(Test, Order = int.MaxValue)] which is an established pattern in this codebase (consistent with how NotInParallelAttribute.Order and other ordering logic work).
4. Public API snapshot files — done in commit 1d680db.

Minor observation (no action needed)

AfterTeardownAssertion is not itself marked [EngineTest(ExpectedResult.Pass)] — but that is correct, because it is an [After(Test)] hook rather than a [Test] method. The assertion inside it will surface as a test failure on Override_Should_Run_Once if it throws, which is the right behaviour.

Conclusion

The implementation is correct, well-tested, and complete. The deduplication logic is sound for arbitrary inheritance depths (all overrides in a chain share the same GetBaseDefinition() root), the reflection path pays zero extra cost at dedup time (pre-resolved at registration), and the source-gen fallback is in place. This is ready to merge.