Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
fix(engine): polish review for #5448
- ReflectionHookDiscoveryService: hoist Materialize() out of the duplicate-scan predicate; comment on why the guard stays despite _registeredMethods short-circuit
- HookDelegateBuilder: comment on _processedHooks reference-identity dedup assumption
  • Loading branch information
thomhurst committed Apr 25, 2026
commit 9b95cbeedeefe981ff837fac72c60035f56a8aaa
24 changes: 18 additions & 6 deletions TUnit.Engine/Discovery/ReflectionHookDiscoveryService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,15 @@ private static void RegisterAfterHook(
break;
case HookType.TestDiscovery:
var discoveryMetadata = CreateMethodMetadata(type, method);
// Check if this hook is already registered (prevent duplicates)
if (!Sources.AfterTestDiscoveryHooks.Any(h => h.Materialize().MethodInfo.Name == discoveryMetadata.Name &&
h.Materialize().MethodInfo.Type == discoveryMetadata.Type))
// Defence in depth: _registeredMethods.TryAdd above would normally short-circuit
// before we reach here, but this guard protects against future refactors that
// might bypass the upstream dedup (e.g. a separate registration path).
if (!Sources.AfterTestDiscoveryHooks.Any(h =>
{
var m = h.Materialize();
return m.MethodInfo.Name == discoveryMetadata.Name &&
m.MethodInfo.Type == discoveryMetadata.Type;
}))
{
var discoveryHook = new AfterTestDiscoveryHookMethod
{
Expand Down Expand Up @@ -673,9 +679,15 @@ private static void RegisterAfterEveryHook(
// The source generator ignores the "Every" suffix for TestDiscovery hooks
// Register it as a regular After hook to match source-gen behavior
var discoveryEveryMetadata = CreateMethodMetadata(type, method);
// Check if this hook is already registered (prevent duplicates)
if (!Sources.AfterTestDiscoveryHooks.Any(h => h.Materialize().MethodInfo.Name == discoveryEveryMetadata.Name &&
h.Materialize().MethodInfo.Type == discoveryEveryMetadata.Type))
// Defence in depth: _registeredMethods.TryAdd above would normally short-circuit
// before we reach here, but this guard protects against future refactors that
// might bypass the upstream dedup (e.g. a separate registration path).
if (!Sources.AfterTestDiscoveryHooks.Any(h =>
{
var m = h.Materialize();
return m.MethodInfo.Name == discoveryEveryMetadata.Name &&
m.MethodInfo.Type == discoveryEveryMetadata.Type;
}))
{
var discoveryHook = new AfterTestDiscoveryHookMethod
{
Expand Down
5 changes: 4 additions & 1 deletion TUnit.Engine/Services/HookDelegateBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@ internal sealed class HookDelegateBuilder : IHookDelegateBuilder
private IReadOnlyList<NamedHookDelegate<AssemblyHookContext>>? _beforeEveryAssemblyHooks;
private IReadOnlyList<NamedHookDelegate<AssemblyHookContext>>? _afterEveryAssemblyHooks;

// Cache for processed hooks to avoid re-processing event receivers
// Cache for processed hooks to avoid re-processing event receivers.
// Dedup relies on reference identity (HookMethod does not override Equals/GetHashCode);
// safe today because Materialize() returns the same cached instance per hook.
// If HookMethod ever overrides Equals/GetHashCode, revisit this assumption.
private readonly ConcurrentDictionary<object, bool> _processedHooks = new();

public HookDelegateBuilder(EventReceiverOrchestrator eventReceiverOrchestrator, TUnitFrameworkLogger logger)
Expand Down
Loading