From bc62322e304358458e02dcbf740f6209ddebe381 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Wed, 8 Apr 2026 22:52:11 +0100 Subject: [PATCH 1/2] feat: TUnit0074 analyzer for redundant hook attributes on overrides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a method declares [Before(Test)] / [After(Test)] and overrides a base method that already declares the same hook attribute, both registrations are invoked via virtual dispatch on the same instance — the override's body runs twice per test. The previous fix (#5428 / #5441) deduplicated this at runtime via MethodInfo.GetBaseDefinition(), which silently hid the duplication instead of surfacing it to the author and didn't address the [InheritsTests] variant in #5450. Replace the runtime dedup with a compile-time analyzer (TUnit0074, Error) plus a code fix that removes the redundant attribute. The analyzer walks the full IMethodSymbol.OverriddenMethod chain so transitive cases (gap in the middle of the override chain) are also caught, and the [InheritsTests] + abstract intermediate shape from #5450 fails to compile at the intermediate's override. Revert the runtime dedup: drop InstanceHookMethod.BaseDefinition, ResolveBaseDefinition / IsOverriddenByMoreDerivedHook from HookDelegateBuilder, and the GetBaseDefinition() init in ReflectionHookDiscoveryService. PublicAPI snapshots updated accordingly. Regression tests (VirtualHookOverrideTests + Bugs/5450) rewritten to the "attribute only on base, override without attribute" shape — the only shape TUnit0074 still allows. Assertions are inlined inside the hook bodies because After-hooks are sorted derived-class-first across the type hierarchy, so a separate verification hook on the derived class would run before the base teardown. --- .../VirtualHookOverrideCodeFixProvider.cs | 74 ++++++ .../VirtualHookOverrideAnalyzerTests.cs | 242 ++++++++++++++++++ TUnit.Analyzers/AnalyzerReleases.Unshipped.md | 1 + TUnit.Analyzers/Resources.resx | 9 + TUnit.Analyzers/Rules.cs | 3 + .../VirtualHookOverrideAnalyzer.cs | 82 ++++++ TUnit.Core/Hooks/InstanceHookMethod.cs | 24 +- .../ReflectionHookDiscoveryService.cs | 6 +- TUnit.Engine/Services/HookDelegateBuilder.cs | 71 ----- ...Has_No_API_Changes.DotNet10_0.verified.txt | 1 - ..._Has_No_API_Changes.DotNet8_0.verified.txt | 1 - ..._Has_No_API_Changes.DotNet9_0.verified.txt | 1 - ...ary_Has_No_API_Changes.Net4_7.verified.txt | 1 - .../InheritsTestsVirtualHookOverrideTests.cs | 64 +++++ TUnit.TestProject/VirtualHookOverrideTests.cs | 35 +-- 15 files changed, 496 insertions(+), 119 deletions(-) create mode 100644 TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs create mode 100644 TUnit.Analyzers.Tests/VirtualHookOverrideAnalyzerTests.cs create mode 100644 TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs create mode 100644 TUnit.TestProject/Bugs/5450/InheritsTestsVirtualHookOverrideTests.cs diff --git a/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs b/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs new file mode 100644 index 0000000000..1795e59d11 --- /dev/null +++ b/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs @@ -0,0 +1,74 @@ +using System.Collections.Immutable; +using System.Composition; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using TUnit.Analyzers; + +namespace TUnit.Analyzers.CodeFixers; + +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(VirtualHookOverrideCodeFixProvider)), Shared] +public class VirtualHookOverrideCodeFixProvider : CodeFixProvider +{ + private const string Title = "Remove redundant hook attribute"; + + public sealed override ImmutableArray FixableDiagnosticIds { get; } = + ImmutableArray.Create(Rules.RedundantHookAttributeOnOverride.Id); + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) + { + return; + } + + foreach (var diagnostic in context.Diagnostics) + { + var attributeSyntax = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true) + .FirstAncestorOrSelf(); + + if (attributeSyntax is null) + { + continue; + } + + context.RegisterCodeFix( + CodeAction.Create( + title: Title, + createChangedDocument: c => RemoveAttributeAsync(context.Document, attributeSyntax, c), + equivalenceKey: Title), + diagnostic); + } + } + + private static async Task RemoveAttributeAsync(Document document, AttributeSyntax attributeSyntax, CancellationToken cancellationToken) + { + var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + if (root is null) + { + return document; + } + + var attributeList = (AttributeListSyntax)attributeSyntax.Parent!; + + SyntaxNode newRoot; + if (attributeList.Attributes.Count == 1) + { + // KeepNoTrivia so we don't leave a stray blank line where the attribute list was. + newRoot = root.RemoveNode(attributeList, SyntaxRemoveOptions.KeepNoTrivia)!; + } + else + { + var newAttributeList = attributeList.WithAttributes( + attributeList.Attributes.Remove(attributeSyntax)); + newRoot = root.ReplaceNode(attributeList, newAttributeList); + } + + return document.WithSyntaxRoot(newRoot); + } +} diff --git a/TUnit.Analyzers.Tests/VirtualHookOverrideAnalyzerTests.cs b/TUnit.Analyzers.Tests/VirtualHookOverrideAnalyzerTests.cs new file mode 100644 index 0000000000..0fc8341fc1 --- /dev/null +++ b/TUnit.Analyzers.Tests/VirtualHookOverrideAnalyzerTests.cs @@ -0,0 +1,242 @@ +using Verifier = TUnit.Analyzers.Tests.Verifiers.CSharpAnalyzerVerifier; + +namespace TUnit.Analyzers.Tests; + +public class VirtualHookOverrideAnalyzerTests +{ + [Test] + public async Task Before_Test_On_Both_Base_And_Override_Reports_Error() + { + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + [Before(Test)] + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public class DerivedClass : BaseClass + { + [{|#0:Before(Test)|}] + public override Task SetupAsync() => base.SetupAsync(); + } + """, + Verifier.Diagnostic(Rules.RedundantHookAttributeOnOverride) + .WithLocation(0) + .WithArguments("Before", "BaseClass", "SetupAsync") + ); + } + + [Test] + public async Task After_Test_On_Both_Base_And_Override_Reports_Error() + { + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + [After(Test)] + public virtual Task TeardownAsync() => Task.CompletedTask; + } + + public class DerivedClass : BaseClass + { + [{|#0:After(Test)|}] + public override Task TeardownAsync() => base.TeardownAsync(); + } + """, + Verifier.Diagnostic(Rules.RedundantHookAttributeOnOverride) + .WithLocation(0) + .WithArguments("After", "BaseClass", "TeardownAsync") + ); + } + + [Test] + public async Task Attribute_Only_On_Base_Is_Fine() + { + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + [Before(Test)] + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public class DerivedClass : BaseClass + { + public override Task SetupAsync() => base.SetupAsync(); + } + """ + ); + } + + [Test] + public async Task Attribute_Only_On_Override_Is_Fine() + { + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public class DerivedClass : BaseClass + { + [Before(Test)] + public override Task SetupAsync() => base.SetupAsync(); + } + """ + ); + } + + [Test] + public async Task Abstract_Intermediate_With_InheritsTests_Reports_Error_On_Intermediate() + { + // Regression shape for https://github.com/thomhurst/TUnit/issues/5450 — an abstract + // intermediate overrides the base hook and then concrete subclasses pick up the tests via + // [InheritsTests]. The duplication happens on the intermediate's override, so that's where + // the diagnostic should fire. + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + [Before(Test)] + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public abstract class IntermediateClass : BaseClass + { + [{|#0:Before(Test)|}] + public override Task SetupAsync() => base.SetupAsync(); + + [Test] + public void DoTest() { } + } + + [InheritsTests] + public class ConcreteA : IntermediateClass; + + [InheritsTests] + public class ConcreteB : IntermediateClass; + """, + Verifier.Diagnostic(Rules.RedundantHookAttributeOnOverride) + .WithLocation(0) + .WithArguments("Before", "BaseClass", "SetupAsync") + ); + } + + [Test] + public async Task Chain_With_Gap_Still_Reports_Error_Via_Transitive_Ancestor() + { + // A has the hook attribute, B overrides without it (fine — virtual dispatch), C overrides + // with the hook attribute again — C is the source of the duplication because A is still + // in the ancestor chain. + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class A + { + [Before(Test)] + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public class B : A + { + public override Task SetupAsync() => base.SetupAsync(); + } + + public class C : B + { + [{|#0:Before(Test)|}] + public override Task SetupAsync() => base.SetupAsync(); + } + """, + Verifier.Diagnostic(Rules.RedundantHookAttributeOnOverride) + .WithLocation(0) + .WithArguments("Before", "A", "SetupAsync") + ); + } + + [Test] + public async Task Mismatched_Hook_Types_Are_Fine() + { + // [Before(Test)] on the base and [After(Test)] on the override are different hooks; both + // run in their own phase, so there is no duplication. + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + [Before(Test)] + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public class DerivedClass : BaseClass + { + [After(Test)] + public override Task SetupAsync() => base.SetupAsync(); + } + """ + ); + } + + [Test] + public async Task New_Method_Is_Fine() + { + // `new` is not `override` — derived's method is a different method, so both hooks are + // registered on different targets and neither double-fires. + await Verifier + .VerifyAnalyzerAsync( + """ + using System.Threading.Tasks; + using TUnit.Core; + using static TUnit.Core.HookType; + + public class BaseClass + { + [Before(Test)] + public virtual Task SetupAsync() => Task.CompletedTask; + } + + public class DerivedClass : BaseClass + { + [Before(Test)] + public new Task SetupAsync() => Task.CompletedTask; + } + """ + ); + } + +} diff --git a/TUnit.Analyzers/AnalyzerReleases.Unshipped.md b/TUnit.Analyzers/AnalyzerReleases.Unshipped.md index 7c65f71a62..1f7d41765a 100644 --- a/TUnit.Analyzers/AnalyzerReleases.Unshipped.md +++ b/TUnit.Analyzers/AnalyzerReleases.Unshipped.md @@ -5,6 +5,7 @@ Rule ID | Category | Severity | Notes TUnit0061 | Usage | Error | ClassDataSource type requires parameterless constructor TUnit0062 | Usage | Warning | CancellationToken must be the last parameter TUnit0073 | Usage | Error | Missing polyfill types required by TUnit +TUnit0074 | Usage | Error | Hook attribute is redundant on an override ### Removed Rules diff --git a/TUnit.Analyzers/Resources.resx b/TUnit.Analyzers/Resources.resx index 3462f5c018..8cf0b5223b 100644 --- a/TUnit.Analyzers/Resources.resx +++ b/TUnit.Analyzers/Resources.resx @@ -534,6 +534,15 @@ Missing polyfill types required by TUnit + + When a virtual instance hook method ([Before(Test)] / [After(Test)]) is already declared on a base class, applying the same hook attribute to an override causes the hook to be registered twice. Because virtual dispatch routes both registrations to the same override, the override's body runs twice per test. Remove the attribute from the override — the base class hook will still call through to the override via virtual dispatch. + + + [{0}(Test)] is already declared on base method '{1}.{2}'. Remove this attribute from the override — virtual dispatch will still invoke this override when the base hook runs. + + + Hook attribute is redundant on an override + Tuple types require reflection for property/field access which is not AOT-compatible. Consider using concrete types or value deconstruction. diff --git a/TUnit.Analyzers/Rules.cs b/TUnit.Analyzers/Rules.cs index 684b61e18d..599087ee4a 100644 --- a/TUnit.Analyzers/Rules.cs +++ b/TUnit.Analyzers/Rules.cs @@ -173,6 +173,9 @@ public static class Rules customTags: [WellKnownDiagnosticTags.CompilationEnd], helpLinkUri: "https://www.nuget.org/packages/Polyfill"); + public static readonly DiagnosticDescriptor RedundantHookAttributeOnOverride = + CreateDescriptor("TUnit0074", UsageCategory, DiagnosticSeverity.Error); + public static readonly DiagnosticDescriptor GenericTypeNotAotCompatible = CreateDescriptor("TUnit0300", UsageCategory, DiagnosticSeverity.Warning); diff --git a/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs b/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs new file mode 100644 index 0000000000..aaea5dd061 --- /dev/null +++ b/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs @@ -0,0 +1,82 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using TUnit.Analyzers.Extensions; + +namespace TUnit.Analyzers; + +/// +/// Reports TUnit0074 when a method marked with [Before(Test)] or [After(Test)] +/// overrides a base method that is also marked with the same hook type. Both registrations are +/// invoked via virtual dispatch, so the override's body runs twice per test. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class VirtualHookOverrideAnalyzer : ConcurrentDiagnosticAnalyzer +{ + public override ImmutableArray SupportedDiagnostics { get; } = + ImmutableArray.Create(Rules.RedundantHookAttributeOnOverride); + + protected override void InitializeInternal(AnalysisContext context) + { + context.RegisterSymbolAction(AnalyzeSymbol, SymbolKind.Method); + } + + private static void AnalyzeSymbol(SymbolAnalysisContext context) + { + if (context.Symbol is not IMethodSymbol { IsOverride: true } methodSymbol) + { + return; + } + + foreach (var attributeData in methodSymbol.GetAttributes()) + { + if (!attributeData.IsStandardHook(context.Compilation, out _, out var hookLevel, out var hookType)) + { + continue; + } + + // Only instance (Test-level) hooks participate in virtual dispatch; all other hook + // levels are enforced static elsewhere. + if (hookLevel != HookLevel.Test) + { + continue; + } + + var conflictingBase = FindBaseWithMatchingHook(methodSymbol, hookType!.Value, context.Compilation); + if (conflictingBase is null) + { + continue; + } + + context.ReportDiagnostic(Diagnostic.Create( + Rules.RedundantHookAttributeOnOverride, + attributeData.GetLocation() ?? methodSymbol.Locations.FirstOrDefault(), + hookType.Value.ToString(), + conflictingBase.ContainingType?.Name ?? "base", + conflictingBase.Name)); + } + } + + // Walks the full override chain (not just the immediate base) — any ancestor with the same + // hook type causes the duplication. See the Chain_With_Gap test for the transitive case. + private static IMethodSymbol? FindBaseWithMatchingHook(IMethodSymbol methodSymbol, HookType hookType, Compilation compilation) + { + var current = methodSymbol.OverriddenMethod; + while (current is not null) + { + foreach (var baseAttribute in current.GetAttributes()) + { + if (baseAttribute.IsStandardHook(compilation, out _, out var baseLevel, out var baseType) + && baseLevel == HookLevel.Test + && baseType == hookType) + { + return current; + } + } + + current = current.OverriddenMethod; + } + + return null; + } +} diff --git a/TUnit.Core/Hooks/InstanceHookMethod.cs b/TUnit.Core/Hooks/InstanceHookMethod.cs index a2176b6a06..2b2a2776f7 100644 --- a/TUnit.Core/Hooks/InstanceHookMethod.cs +++ b/TUnit.Core/Hooks/InstanceHookMethod.cs @@ -1,6 +1,4 @@ -using System.ComponentModel; -using System.Diagnostics.CodeAnalysis; -using System.Reflection; +using System.Diagnostics.CodeAnalysis; namespace TUnit.Core.Hooks; @@ -11,28 +9,18 @@ public record InstanceHookMethod : HookMethod, IExecutableHook { [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] private readonly Type _classType = null!; - + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] public override Type ClassType => _classType; - - public required Type InitClassType - { + + public required Type InitClassType + { [param: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] - init { _classType = value; } + init { _classType = value; } } public Func? Body { get; init; } - /// - /// The base method definition for this hook (i.e. ), - /// used by the engine to deduplicate virtual hook methods that are overridden in a derived - /// class. Optional — when null, the engine resolves it via reflection from - /// and the metadata name/parameters. Set directly by the reflection discovery path which already - /// holds a at registration time. - /// - [EditorBrowsable(EditorBrowsableState.Never)] - public MethodInfo? BaseDefinition { get; init; } - public ValueTask ExecuteAsync(TestContext context, CancellationToken cancellationToken) { // Skip instance hooks if this is a pre-skipped test diff --git a/TUnit.Engine/Discovery/ReflectionHookDiscoveryService.cs b/TUnit.Engine/Discovery/ReflectionHookDiscoveryService.cs index 5b73d487a6..328093ca6d 100644 --- a/TUnit.Engine/Discovery/ReflectionHookDiscoveryService.cs +++ b/TUnit.Engine/Discovery/ReflectionHookDiscoveryService.cs @@ -713,8 +713,7 @@ private static void RegisterInstanceBeforeHook( HookExecutor = GetHookExecutor(method), Order = order, RegistrationIndex = Interlocked.Increment(ref _registrationIndex), - Body = CreateInstanceHookDelegate(type, method), - BaseDefinition = method.GetBaseDefinition() + Body = CreateInstanceHookDelegate(type, method) }; bag.Add(hook); } @@ -739,8 +738,7 @@ private static void RegisterInstanceAfterHook( HookExecutor = GetHookExecutor(method), Order = order, RegistrationIndex = Interlocked.Increment(ref _registrationIndex), - Body = CreateInstanceHookDelegate(type, method), - BaseDefinition = method.GetBaseDefinition() + Body = CreateInstanceHookDelegate(type, method) }; bag.Add(hook); } diff --git a/TUnit.Engine/Services/HookDelegateBuilder.cs b/TUnit.Engine/Services/HookDelegateBuilder.cs index dc58d56358..6f78f1a99a 100644 --- a/TUnit.Engine/Services/HookDelegateBuilder.cs +++ b/TUnit.Engine/Services/HookDelegateBuilder.cs @@ -53,46 +53,6 @@ private static Type GetCachedGenericTypeDefinition(Type type) return _genericTypeDefinitionCache.GetOrAdd(type, static t => t.GetGenericTypeDefinition()); } - /// - /// When walking the class hierarchy from derived to base collecting instance hooks, a virtual - /// hook method overridden in a derived class is registered twice — once on the base type, once - /// on the derived type — but virtual dispatch causes both registrations to invoke the same - /// override. This helper records the base method definition for each hook seen so far and - /// returns true when a hook's underlying method has already been collected via a more-derived - /// override. See issue #5428. - /// - private static bool IsOverriddenByMoreDerivedHook(InstanceHookMethod hook, HashSet seenBaseDefinitions) - { - // Prefer the pre-resolved BaseDefinition (set by the reflection discovery path). - // The source-generated path doesn't currently populate it, so fall back to a one-shot - // reflection lookup keyed by name + parameter types. Result is cached in the parent - // _beforeTestHooksCache / _afterTestHooksCache, so this only runs on first build per type. - var baseDefinition = hook.BaseDefinition ?? ResolveBaseDefinition(hook); - if (baseDefinition is null) - { - return false; - } - - return !seenBaseDefinitions.Add(baseDefinition); - } - - private static MethodInfo? ResolveBaseDefinition(InstanceHookMethod hook) - { - var parameters = hook.MethodInfo.Parameters; - var paramTypes = parameters.Length == 0 - ? Type.EmptyTypes - : parameters.Select(p => p.Type).ToArray(); - - var methodInfo = hook.ClassType.GetMethod( - hook.MethodInfo.Name, - BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly, - binder: null, - types: paramTypes, - modifiers: null); - - return methodInfo?.GetBaseDefinition(); - } - public async ValueTask InitializeAsync() { await _logger.LogDebugAsync("Building global hook delegates...").ConfigureAwait(false); @@ -231,13 +191,6 @@ private async Task>> BuildBeforeTes { var hooksByType = new List<(Type type, List<(int order, int registrationIndex, NamedHookDelegate hook)> hooks)>(); - // Tracks base method definitions (via MethodInfo.GetBaseDefinition()) of hooks we've - // already collected from more-derived types. Used to skip virtual hook methods that - // are overridden in a derived class — without this, both the base method registration - // AND the override registration would run, but virtual dispatch makes them invoke the - // same override, causing it to execute twice. See issue #5428. - var seenBaseDefinitions = new HashSet(); - // Collect hooks for each type in the hierarchy var currentType = type; while (currentType != null) @@ -248,11 +201,6 @@ private async Task>> BuildBeforeTes { foreach (var hook in sourceHooks) { - if (IsOverriddenByMoreDerivedHook(hook, seenBaseDefinitions)) - { - continue; - } - var namedHook = await CreateInstanceHookDelegateAsync(hook); typeHooks.Add((hook.Order, hook.RegistrationIndex, namedHook)); } @@ -266,11 +214,6 @@ private async Task>> BuildBeforeTes { foreach (var hook in openTypeHooks) { - if (IsOverriddenByMoreDerivedHook(hook, seenBaseDefinitions)) - { - continue; - } - var namedHook = await CreateInstanceHookDelegateAsync(hook); typeHooks.Add((hook.Order, hook.RegistrationIndex, namedHook)); } @@ -315,10 +258,6 @@ private async Task>> BuildAfterTest { var hooksByType = new List<(Type type, List<(int order, int registrationIndex, NamedHookDelegate hook)> hooks)>(); - // See note in BuildBeforeTestHooksAsync — same dedup needed for After(Test) hooks - // so virtual overrides don't execute twice (issue #5428). - var seenBaseDefinitions = new HashSet(); - // Collect hooks for each type in the hierarchy var currentType = type; while (currentType != null) @@ -329,11 +268,6 @@ private async Task>> BuildAfterTest { foreach (var hook in sourceHooks) { - if (IsOverriddenByMoreDerivedHook(hook, seenBaseDefinitions)) - { - continue; - } - var namedHook = await CreateInstanceHookDelegateAsync(hook); typeHooks.Add((hook.Order, hook.RegistrationIndex, namedHook)); } @@ -347,11 +281,6 @@ private async Task>> BuildAfterTest { foreach (var hook in openTypeHooks) { - if (IsOverriddenByMoreDerivedHook(hook, seenBaseDefinitions)) - { - continue; - } - var namedHook = await CreateInstanceHookDelegateAsync(hook); typeHooks.Add((hook.Order, hook.RegistrationIndex, namedHook)); } diff --git a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt index ccf949b734..a8ef282b22 100644 --- a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt +++ b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt @@ -2321,7 +2321,6 @@ namespace .Hooks public class InstanceHookMethod : ., <.>, .<.TestContext> { public InstanceHookMethod() { } - public .MethodInfo? BaseDefinition { get; init; } public ? Body { get; init; } [.(..PublicMethods)] public override ClassType { get; } diff --git a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt index 415ded6100..e9ed0a0976 100644 --- a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt +++ b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt @@ -2321,7 +2321,6 @@ namespace .Hooks public class InstanceHookMethod : ., <.>, .<.TestContext> { public InstanceHookMethod() { } - public .MethodInfo? BaseDefinition { get; init; } public ? Body { get; init; } [.(..PublicMethods)] public override ClassType { get; } diff --git a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt index 6110f69efb..f2934aed1c 100644 --- a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt +++ b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt @@ -2321,7 +2321,6 @@ namespace .Hooks public class InstanceHookMethod : ., <.>, .<.TestContext> { public InstanceHookMethod() { } - public .MethodInfo? BaseDefinition { get; init; } public ? Body { get; init; } [.(..PublicMethods)] public override ClassType { get; } diff --git a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt index 035b2b9241..e0b73a707b 100644 --- a/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt +++ b/TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt @@ -2264,7 +2264,6 @@ namespace .Hooks public class InstanceHookMethod : ., <.>, .<.TestContext> { public InstanceHookMethod() { } - public .MethodInfo? BaseDefinition { get; init; } public ? Body { get; init; } public override ClassType { get; } public required InitClassType { init; } diff --git a/TUnit.TestProject/Bugs/5450/InheritsTestsVirtualHookOverrideTests.cs b/TUnit.TestProject/Bugs/5450/InheritsTestsVirtualHookOverrideTests.cs new file mode 100644 index 0000000000..748ac252e5 --- /dev/null +++ b/TUnit.TestProject/Bugs/5450/InheritsTestsVirtualHookOverrideTests.cs @@ -0,0 +1,64 @@ +using TUnit.Assertions; +using TUnit.Assertions.Extensions; +using TUnit.Core; +using TUnit.TestProject.Attributes; + +namespace TUnit.TestProject.Bugs._5450; + +// Regression test for https://github.com/thomhurst/TUnit/issues/5450 (related to #5428). +// +// The original failure: a virtual [Before(Test)] hook on a base class was overridden in an +// abstract intermediate class that *also* declared [Before(Test)] on the override, then concrete +// subclasses inherited the tests via [InheritsTests]. The duplicate attribute caused the override +// to execute twice per test. Fixed at compile time by TUnit0074. This test guards the positive +// shape across every concrete [InheritsTests] subclass and both discovery paths. +public class InheritsTestsVirtualHookOverrideTests +{ + public class BaseTestClass + { + public int SetupCalls; + public int TeardownCalls; + + [Before(Test)] + public virtual async Task SetupAsync() + { + SetupCalls++; + // Inline assertion — see VirtualHookOverrideTests for the rationale. + await Assert.That(SetupCalls).IsEqualTo(1); + } + + [After(Test)] + public virtual async Task TeardownAsync() + { + TeardownCalls++; + await Assert.That(TeardownCalls).IsEqualTo(1); + } + } + + public abstract class IntermediateTestClass : BaseTestClass + { + // No [Before(Test)] / [After(Test)] — see the analogous note in VirtualHookOverrideTests. + public override async Task SetupAsync() + { + await base.SetupAsync(); + } + + public override async Task TeardownAsync() + { + await base.TeardownAsync(); + } + + [Test] + [EngineTest(ExpectedResult.Pass)] + public async Task Override_Runs_Exactly_Once() + { + await Assert.That(SetupCalls).IsEqualTo(1); + } + } + + [InheritsTests] + public class ConcreteTestClassA : IntermediateTestClass; + + [InheritsTests] + public class ConcreteTestClassB : IntermediateTestClass; +} diff --git a/TUnit.TestProject/VirtualHookOverrideTests.cs b/TUnit.TestProject/VirtualHookOverrideTests.cs index b81754f58a..470ef55c9e 100644 --- a/TUnit.TestProject/VirtualHookOverrideTests.cs +++ b/TUnit.TestProject/VirtualHookOverrideTests.cs @@ -6,10 +6,7 @@ namespace TUnit.TestProject; // Regression test for https://github.com/thomhurst/TUnit/issues/5428 -// A virtual [Before(Test)]/[After(Test)] hook in a base class that is overridden -// in a derived class (also marked with the same hook attribute) should only execute -// the override once — not twice — because both registrations would otherwise be -// invoked via virtual dispatch on the same instance. +// See Bugs/5450 for the [InheritsTests] variant. public class VirtualHookOverrideTests { public class BaseTestClass @@ -18,29 +15,34 @@ public class BaseTestClass public int TeardownCalls; [Before(Test)] - public virtual Task SetupAsync() + public virtual async Task SetupAsync() { SetupCalls++; - return Task.CompletedTask; + // Inline assertion: a duplicate registration would invoke this twice and fail on the + // second call. A separate verification hook can't sit after the base hook because + // After-hooks are sorted derived-class-first across the type hierarchy (Order only + // sorts within a single type level), so it would run before TeardownAsync below. + await Assert.That(SetupCalls).IsEqualTo(1); } [After(Test)] - public virtual Task TeardownAsync() + public virtual async Task TeardownAsync() { TeardownCalls++; - return Task.CompletedTask; + await Assert.That(TeardownCalls).IsEqualTo(1); } } public class DerivedTestClass : BaseTestClass { - [Before(Test)] + // No [Before(Test)] / [After(Test)] here — the base's attributes already register these + // methods and virtual dispatch routes to the override. TUnit0074 would flag a redundant + // re-declaration. public override async Task SetupAsync() { await base.SetupAsync(); } - [After(Test)] public override async Task TeardownAsync() { await base.TeardownAsync(); @@ -48,20 +50,9 @@ public override async Task TeardownAsync() [Test] [EngineTest(ExpectedResult.Pass)] - public async Task Override_Should_Run_Once() + public async Task Override_Runs_Exactly_Once() { - // TeardownCalls is verified by AfterTeardownAssertion below — by the time - // this test method runs, TeardownAsync has not yet executed (it's an After - // hook), so we can't assert it here. The dedicated [After(Test)] hook with - // Order = int.MaxValue runs last and performs the assertion. await Assert.That(SetupCalls).IsEqualTo(1); } - - // Runs after TeardownAsync to verify the After(Test) override also deduplicates. - [After(Test, Order = int.MaxValue)] - public async Task AfterTeardownAssertion() - { - await Assert.That(TeardownCalls).IsEqualTo(1); - } } } From ed5c622cfde54bce8336054f5c78a0f88e0655ff Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Wed, 8 Apr 2026 23:04:14 +0100 Subject: [PATCH 2/2] address review: combine null guards in analyzer + KeepLeadingTrivia in code fix - Merge the IsStandardHook/HookLevel/hookType checks into one if so the compiler tracks nullability without needing the null-forgiving operator. - Use KeepLeadingTrivia when removing the attribute list so any leading comment above the attribute is preserved on the method declaration. --- .../VirtualHookOverrideCodeFixProvider.cs | 5 +++-- TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs | 11 ++++------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs b/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs index 1795e59d11..4c58dc9e84 100644 --- a/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs +++ b/TUnit.Analyzers.CodeFixers/VirtualHookOverrideCodeFixProvider.cs @@ -59,8 +59,9 @@ private static async Task RemoveAttributeAsync(Document document, Attr SyntaxNode newRoot; if (attributeList.Attributes.Count == 1) { - // KeepNoTrivia so we don't leave a stray blank line where the attribute list was. - newRoot = root.RemoveNode(attributeList, SyntaxRemoveOptions.KeepNoTrivia)!; + // KeepLeadingTrivia so any comment above the attribute list is preserved by being + // re-attached to the next node (the method declaration). + newRoot = root.RemoveNode(attributeList, SyntaxRemoveOptions.KeepLeadingTrivia)!; } else { diff --git a/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs b/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs index aaea5dd061..f1725a8982 100644 --- a/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs +++ b/TUnit.Analyzers/VirtualHookOverrideAnalyzer.cs @@ -30,19 +30,16 @@ private static void AnalyzeSymbol(SymbolAnalysisContext context) foreach (var attributeData in methodSymbol.GetAttributes()) { - if (!attributeData.IsStandardHook(context.Compilation, out _, out var hookLevel, out var hookType)) - { - continue; - } - // Only instance (Test-level) hooks participate in virtual dispatch; all other hook // levels are enforced static elsewhere. - if (hookLevel != HookLevel.Test) + if (!attributeData.IsStandardHook(context.Compilation, out _, out var hookLevel, out var hookType) + || hookLevel != HookLevel.Test + || hookType is null) { continue; } - var conflictingBase = FindBaseWithMatchingHook(methodSymbol, hookType!.Value, context.Compilation); + var conflictingBase = FindBaseWithMatchingHook(methodSymbol, hookType.Value, context.Compilation); if (conflictingBase is null) { continue;