From 8385ded20f96d7899a1e25175ebd9adb99994228 Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:02:03 +0000 Subject: [PATCH] fix(engine): invoke OnTestRegistered on property-injected data sources Registered receivers ran once, before argument registration resolved injected property values, so a shared fixture that implements ITestRegisteredEventReceiver was skipped when injected as a property. Run a second receiver pass after argument registration, sharing the identity set so receivers that already ran are not invoked twice. Fixes #7290 --- docs/docs/writing-tests/event-subscribing.md | 6 + .../ITestRegisteredEventReceiver.cs | 6 + .../Services/TestFilterService.cs | 75 ++++++++---- ...steredReceiverInjectedProperty7290Tests.cs | 27 +++++ tests/TUnit.TestProject/Bugs/_7290/Tests.cs | 110 ++++++++++++++++++ 5 files changed, 201 insertions(+), 23 deletions(-) create mode 100644 tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs create mode 100644 tests/TUnit.TestProject/Bugs/_7290/Tests.cs diff --git a/docs/docs/writing-tests/event-subscribing.md b/docs/docs/writing-tests/event-subscribing.md index d622884acbb..45866a2c594 100644 --- a/docs/docs/writing-tests/event-subscribing.md +++ b/docs/docs/writing-tests/event-subscribing.md @@ -21,6 +21,12 @@ The interfaces they can implement are: This can be useful especially when generating data that you need to track and maybe dispose later. By hooking into these events, we can do things like track and dispose our objects when we need. +## Registration callbacks for injected properties + +TUnit creates injected property values after the first `OnTestRegistered` pass, so that a receiver such as a `SkipAttribute` can skip the test before expensive data sources are created. An injected property value that implements `ITestRegisteredEventReceiver` receives its callback in a second pass, after the property values are resolved and before the test runs. The second pass uses the same `Order` sorting, but it always runs after every receiver from the first pass. + +A property value can still call `SetParallelLimiter`, `SetSkipped` and the other `TestRegisteredContext` methods. If it skips the test, its shared data source objects have already been created. + ## Registration callbacks for executors An executor that implements `ITestRegisteredEventReceiver` receives `OnTestRegistered` when a registration receiver installs it through `SetTestExecutor` or `SetHookExecutor`, unless it has already received the callback for that test. Its callback runs after the installing receiver returns. Dynamically installed executors are not globally sorted by their own `Order`. diff --git a/src/TUnit.Core/Interfaces/ITestRegisteredEventReceiver.cs b/src/TUnit.Core/Interfaces/ITestRegisteredEventReceiver.cs index b1af3c5fb6a..f7470b50a0b 100644 --- a/src/TUnit.Core/Interfaces/ITestRegisteredEventReceiver.cs +++ b/src/TUnit.Core/Interfaces/ITestRegisteredEventReceiver.cs @@ -28,6 +28,12 @@ namespace TUnit.Core.Interfaces; /// Each receiver instance is invoked at most once per test, even when it is both an eligible /// event object and an installed executor. Distinct instances are compared by reference identity. /// +/// +/// Injected property values are resolved after the first registration pass, so that receivers +/// such as can skip a test before expensive data sources are created. +/// An injected property value that implements this interface is invoked in a second pass, after +/// every receiver from the first pass and before the test executes. +/// /// public interface ITestRegisteredEventReceiver : IEventReceiver { diff --git a/src/TUnit.Engine/Services/TestFilterService.cs b/src/TUnit.Engine/Services/TestFilterService.cs index 1ba1d8630a4..4bdab17af62 100644 --- a/src/TUnit.Engine/Services/TestFilterService.cs +++ b/src/TUnit.Engine/Services/TestFilterService.cs @@ -81,9 +81,42 @@ private async Task InvokeTestRegisteredReceiver(ITestRegisteredEventReceiver rec } } + private async Task InvokeTestRegisteredReceivers(ITestRegisteredEventReceiver[] receivers, TestRegisteredContext context, + HashSet invokedReceivers) + { + foreach (var receiver in receivers) + { + await InvokeTestRegisteredReceiver(receiver, context, invokedReceivers).ConfigureAwait(false); + + // Dynamically installed executor callbacks run after their installer, regardless of their + // own Order. The shared identity set also covers executors in the original receiver list. + while (context.TryDequeueExecutorEventReceiver(out var executorReceiver)) + { + await InvokeTestRegisteredReceiver(executorReceiver, context, invokedReceivers).ConfigureAwait(false); + } + } + } + + private static TestRegisteredContext CreateTestRegisteredContext(TestContext testContext) + { + var discoveredTest = new DiscoveredTest + { + TestContext = testContext + }; + + testContext.InternalDiscoveredTest = discoveredTest; + + return new TestRegisteredContext(testContext) + { + DiscoveredTest = discoveredTest + }; + } + private async Task RegisterTest(AbstractExecutableTest test, bool isForExecution) { var registeredReceivers = test.Context.GetTestRegisteredReceivers(); + TestRegisteredContext? registeredContext = null; + HashSet? invokedReceivers = null; // Invoke event receivers BEFORE argument registration so that SkipAttribute // and other ITestRegisteredEventReceiver implementations can set SkipReason @@ -92,30 +125,10 @@ private async Task RegisterTest(AbstractExecutableTest test, bool isForExecution // that need to prevent ClassDataSource initialization when the test should be skipped. if (registeredReceivers.Length > 0) { - var discoveredTest = new DiscoveredTest - { - TestContext = test.Context - }; - - var registeredContext = new TestRegisteredContext(test.Context) - { - DiscoveredTest = discoveredTest - }; - - test.Context.InternalDiscoveredTest = discoveredTest; - var invokedReceivers = new HashSet(Core.Helpers.ReferenceEqualityComparer.Instance); - - foreach (var receiver in registeredReceivers) - { - await InvokeTestRegisteredReceiver(receiver, registeredContext, invokedReceivers).ConfigureAwait(false); + registeredContext = CreateTestRegisteredContext(test.Context); + invokedReceivers = new HashSet(Core.Helpers.ReferenceEqualityComparer.Instance); - // Dynamically installed executor callbacks run after their installer, regardless of their - // own Order. The shared identity set also covers executors in the original receiver list. - while (registeredContext.TryDequeueExecutorEventReceiver(out var executorReceiver)) - { - await InvokeTestRegisteredReceiver(executorReceiver, registeredContext, invokedReceivers).ConfigureAwait(false); - } - } + await InvokeTestRegisteredReceivers(registeredReceivers, registeredContext, invokedReceivers).ConfigureAwait(false); } // If the test was marked as skipped by an event receiver, skip argument registration entirely. @@ -153,15 +166,31 @@ private async Task RegisterTest(AbstractExecutableTest test, bool isForExecution // requests would create fixtures as a side effect and leak them forever (#6151). if (isForExecution) { + var argumentsRegistered = false; + try { await testArgumentRegistrationService.RegisterTestArgumentsAsync(test.Context).ConfigureAwait(false); + argumentsRegistered = true; } catch (Exception ex) { // Mark the test as failed - event receivers have already run above test.SetResult(TestState.Failed, ex); } + + // Injected property values only exist now, after argument registration resolved them, + // and those values can themselves be registered receivers (e.g. a shared fixture that + // calls SetParallelLimiter). Argument registration dropped the receiver caches, so the + // rebuilt list includes them; the shared identity set skips receivers that already ran (#7290). + if (argumentsRegistered && + test.Context.Metadata.TestDetails.TestClassInjectedPropertyArguments.Count > 0) + { + registeredContext ??= CreateTestRegisteredContext(test.Context); + invokedReceivers ??= new HashSet(Core.Helpers.ReferenceEqualityComparer.Instance); + + await InvokeTestRegisteredReceivers(test.Context.GetTestRegisteredReceivers(), registeredContext, invokedReceivers).ConfigureAwait(false); + } } // Clear the cached display name after registration events diff --git a/tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs b/tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs new file mode 100644 index 00000000000..4eb08137f07 --- /dev/null +++ b/tests/TUnit.Engine.Tests/RegisteredReceiverInjectedProperty7290Tests.cs @@ -0,0 +1,27 @@ +using Shouldly; +using TUnit.Engine.Tests.Enums; + +namespace TUnit.Engine.Tests; + +/// +/// Verifies issue #7290: a property-injected data source that implements +/// ITestRegisteredEventReceiver receives OnTestRegistered, so it can set a parallel limiter or +/// skip the test. Runs every class in Bugs/_7290/Tests.cs: four repeated property consumers, one +/// constructor-plus-property consumer and one test skipped by its injected fixture. +/// +public class RegisteredReceiverInjectedProperty7290Tests(TestMode testMode) : InvokableTestBase(testMode) +{ + [Test] + public async Task Test() + { + await RunTestsWithFilter( + "/*/TUnit.TestProject.Bugs._7290/*/*", + [ + result => result.ResultSummary.Outcome.ShouldBe("Completed"), + result => result.ResultSummary.Counters.Total.ShouldBe(6), + result => result.ResultSummary.Counters.Passed.ShouldBe(5), + result => result.ResultSummary.Counters.Failed.ShouldBe(0), + result => result.ResultSummary.Counters.NotExecuted.ShouldBe(1) + ]); + } +} diff --git a/tests/TUnit.TestProject/Bugs/_7290/Tests.cs b/tests/TUnit.TestProject/Bugs/_7290/Tests.cs new file mode 100644 index 00000000000..a9c9deb19c4 --- /dev/null +++ b/tests/TUnit.TestProject/Bugs/_7290/Tests.cs @@ -0,0 +1,110 @@ +using System.Collections.Concurrent; +using TUnit.Core.Interfaces; + +namespace TUnit.TestProject.Bugs._7290; + +/// +/// Reproduction for issue #7290: "ITestRegisteredEventReceiver is not invoked on property-injected +/// data source objects." +/// https://github.com/thomhurst/TUnit/issues/7290 +/// +/// Registered receivers ran once, before argument registration resolved injected property values, +/// so a shared fixture received OnTestRegistered as a constructor or method argument but never as +/// an injected property. The fixture below limits its consumers to one test at a time. +/// +public sealed class OneAtATime : IParallelLimit +{ + public int Limit => 1; +} + +public sealed class LimitedFixture : ITestRegisteredEventReceiver +{ + private readonly ConcurrentDictionary _registrations = new(); + private int _running; + private int _maxRunning; + + public int Order => 0; + + public int MaxRunning => Volatile.Read(ref _maxRunning); + + public ValueTask OnTestRegistered(TestRegisteredContext context) + { + _registrations.AddOrUpdate(context.TestContext.Metadata.TestDetails.TestId, 1, static (_, count) => count + 1); + context.SetParallelLimiter(new OneAtATime()); + return default; + } + + public int RegistrationsFor(TestContext context) => + _registrations.TryGetValue(context.Metadata.TestDetails.TestId, out var count) ? count : 0; + + public async Task RunAsync() + { + var running = Interlocked.Increment(ref _running); + int max; + while (running > (max = Volatile.Read(ref _maxRunning)) && + Interlocked.CompareExchange(ref _maxRunning, running, max) != max) + { + } + + await Task.Delay(50); + Interlocked.Decrement(ref _running); + } +} + +public sealed class SkippingFixture : ITestRegisteredEventReceiver +{ + public int Order => 0; + + public ValueTask OnTestRegistered(TestRegisteredContext context) + { + context.SetSkipped("Skipped by property-injected fixture"); + return default; + } +} + +public sealed class PropertyConsumerTests +{ + [ClassDataSource(Shared = SharedType.PerClass)] + public required LimitedFixture Fixture { get; init; } + + [Test, Repeat(3)] + public async Task Property_Injected_Fixture_Receives_OnTestRegistered() + { + var context = TestContext.Current!; + + await Fixture.RunAsync(); + + await Assert.That(context.Parallelism.Limiter).IsTypeOf(); + await Assert.That(Fixture.RegistrationsFor(context)).IsEqualTo(1); + await Assert.That(Fixture.MaxRunning).IsEqualTo(1); + } +} + +[ClassDataSource(Shared = SharedType.PerClass)] +public sealed class ConstructorAndPropertyConsumerTests(LimitedFixture constructorFixture) +{ + [ClassDataSource(Shared = SharedType.PerClass)] + public required LimitedFixture Fixture { get; init; } + + [Test] + public async Task Shared_Instance_Receives_OnTestRegistered_Once() + { + var context = TestContext.Current!; + + await Assert.That(Fixture).IsSameReferenceAs(constructorFixture); + await Assert.That(context.Parallelism.Limiter).IsTypeOf(); + await Assert.That(Fixture.RegistrationsFor(context)).IsEqualTo(1); + } +} + +public sealed class PropertySkipTests +{ + [ClassDataSource(Shared = SharedType.PerClass)] + public required SkippingFixture Fixture { get; init; } + + [Test] + public void Property_Injected_Fixture_Can_Skip_The_Test() + { + throw new InvalidOperationException("This test should have been skipped by its injected fixture."); + } +}