diff --git a/src/TUnit.Core/ContextProvider.cs b/src/TUnit.Core/ContextProvider.cs index a34aff4e854..f90f0579c0d 100644 --- a/src/TUnit.Core/ContextProvider.cs +++ b/src/TUnit.Core/ContextProvider.cs @@ -12,6 +12,7 @@ internal class ContextProvider(IServiceProvider serviceProvider, string testSess { private readonly ConcurrentDictionary _assemblyContexts = new(); private readonly ConcurrentDictionary _classContexts = new(); + private readonly Lock _creationLock = new(); public GlobalContext GlobalContext { get; } = new() { @@ -51,11 +52,30 @@ internal class ContextProvider(IServiceProvider serviceProvider, string testSess /// public AssemblyHookContext GetOrCreateAssemblyContext(Assembly assembly) { - return _assemblyContexts.GetOrAdd(assembly, static (assembly, context) => - new AssemblyHookContext(context) + if (_assemblyContexts.TryGetValue(assembly, out var existing)) + { + return existing; + } + + // Not ConcurrentDictionary.GetOrAdd: the context constructor registers itself with its + // parent, so a factory that loses the GetOrAdd race would leave an orphaned, test-less + // context visible through TestSessionContext.Assemblies. Creation happens once per + // assembly, so the lock is off the hot path. + lock (_creationLock) + { + if (_assemblyContexts.TryGetValue(assembly, out existing)) + { + return existing; + } + + var created = new AssemblyHookContext(TestSessionContext) { Assembly = assembly - }, TestSessionContext); + }; + + _assemblyContexts[assembly] = created; + return created; + } } /// @@ -67,14 +87,31 @@ public ClassHookContext GetOrCreateClassContext( [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicMethods)] Type classType) { - return _classContexts.GetOrAdd(classType, static ([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicMethods)] - type, state) => + if (_classContexts.TryGetValue(classType, out var existing)) { - return new ClassHookContext(state.GetOrCreateAssemblyContext(type.Assembly)) + return existing; + } + + // See GetOrCreateAssemblyContext: a lost GetOrAdd race would leave an orphaned class + // context in AssemblyHookContext.TestClasses. The assembly context is resolved before + // taking _creationLock on purpose, so the assembly and class paths never nest the lock. + var assemblyContext = GetOrCreateAssemblyContext(classType.Assembly); + + lock (_creationLock) + { + if (_classContexts.TryGetValue(classType, out existing)) { - ClassType = type + return existing; + } + + var created = new ClassHookContext(assemblyContext) + { + ClassType = classType }; - }, this); + + _classContexts[classType] = created; + return created; + } } /// diff --git a/src/TUnit.Core/Models/AssemblyHookContext.cs b/src/TUnit.Core/Models/AssemblyHookContext.cs index ea1f6105cf7..41d44a189bc 100644 --- a/src/TUnit.Core/Models/AssemblyHookContext.cs +++ b/src/TUnit.Core/Models/AssemblyHookContext.cs @@ -27,13 +27,26 @@ internal AssemblyHookContext(TestSessionContext testSessionContext) : base(testS private readonly List _testClasses = []; private TestContext[]? _cachedAllTests; + // Set when the last class was removed and the assembly was detached from the session. Guarded by _lock. + private bool _detached; + public void AddClass(ClassHookContext classHookContext) { lock (_lock) { _testClasses.Add(classHookContext); InvalidateCache(); + + // A class reattached by a dynamic test after filtering emptied this assembly. + if (_detached) + { + _detached = false; + TestSessionContext.AddAssembly(this); + return; + } } + + TestSessionContext.InvalidateTestCaches(); } public IReadOnlyList TestClasses { get { lock (_lock) return [.. _testClasses]; } } @@ -49,20 +62,32 @@ private void InvalidateCache() internal bool FirstTestStarted { get; set; } - internal void RemoveClass(ClassHookContext classContext) + internal void InvalidateTestCaches() { - bool empty; lock (_lock) { - _testClasses.Remove(classContext); InvalidateCache(); - empty = _testClasses.Count == 0; } - if (empty) + TestSessionContext.InvalidateTestCaches(); + } + + internal void RemoveClass(ClassHookContext classContext) + { + lock (_lock) { - TestSessionContext.RemoveAssembly(this); + _testClasses.Remove(classContext); + InvalidateCache(); + + if (_testClasses.Count == 0) + { + _detached = true; + TestSessionContext.RemoveAssembly(this); + return; + } } + + TestSessionContext.InvalidateTestCaches(); } internal override void SetAsyncLocalContext() diff --git a/src/TUnit.Core/Models/ClassHookContext.cs b/src/TUnit.Core/Models/ClassHookContext.cs index 331cca60910..5f214901820 100644 --- a/src/TUnit.Core/Models/ClassHookContext.cs +++ b/src/TUnit.Core/Models/ClassHookContext.cs @@ -29,6 +29,9 @@ internal ClassHookContext(AssemblyHookContext assemblyHookContext) : base(assemb private readonly HashSet _testSet = new(ReferenceEqualityComparer.Instance); private readonly List _tests = []; + // Set when the last test was removed and the class was detached from its assembly. Guarded by _lock. + private bool _detached; + public void AddTest(TestContext testContext) { lock (_lock) @@ -38,6 +41,14 @@ public void AddTest(TestContext testContext) return; // Prevent duplicates } _tests.Add(testContext); + + // A dynamic test added at run time to a class whose built tests were all filtered out. + // Reattach under _lock so a concurrent detach cannot interleave with it. + if (_detached) + { + _detached = false; + AssemblyContext.AddClass(this); + } } } @@ -76,20 +87,57 @@ public override int GetHashCode() return ClassType.GetHashCode(); } + /// + /// Drops every test not in . Used once filtering has decided which + /// built tests will run, so hooks only see participating tests. Removes the class from its assembly + /// when no tests remain. + /// + internal void RetainTests(HashSet testsToKeep) + { + lock (_lock) + { + if (_tests.RemoveAll(t => !testsToKeep.Contains(t)) == 0) + { + return; + } + + _testSet.IntersectWith(testsToKeep); + + if (_tests.Count is 0) + { + Detach(); + return; + } + } + + AssemblyContext.InvalidateTestCaches(); + } + internal void RemoveTest(TestContext test) { - bool empty; lock (_lock) { _testSet.Remove(test); _tests.Remove(test); - empty = _tests.Count is 0; + + if (_tests.Count is 0) + { + Detach(); + } } + } - if (empty) + // Caller holds _lock. Lock order is class -> assembly -> session; the parents never call back + // into a class while holding their own lock. + private void Detach() + { + if (_detached) { - AssemblyContext.RemoveClass(this); + return; } + + _detached = true; + AssemblyContext.RemoveClass(this); } internal override void SetAsyncLocalContext() diff --git a/src/TUnit.Core/Models/TestSessionContext.cs b/src/TUnit.Core/Models/TestSessionContext.cs index 2db99eb5bb1..4a57647b8f7 100644 --- a/src/TUnit.Core/Models/TestSessionContext.cs +++ b/src/TUnit.Core/Models/TestSessionContext.cs @@ -97,6 +97,14 @@ private void InvalidateCaches() internal bool FirstTestStarted { get; set; } + internal void InvalidateTestCaches() + { + lock (_lock) + { + InvalidateCaches(); + } + } + private int _failureCount; /// diff --git a/src/TUnit.Engine/Reporters/Html/ActivityCollector.cs b/src/TUnit.Engine/Reporters/Html/ActivityCollector.cs index 6098468ea44..e4aef1f6f5e 100644 --- a/src/TUnit.Engine/Reporters/Html/ActivityCollector.cs +++ b/src/TUnit.Engine/Reporters/Html/ActivityCollector.cs @@ -420,6 +420,10 @@ private void OnActivityStopped(Activity activity) // Enumerate* struct enumerators instead of LINQ ToArray over the IEnumerable // properties: no boxed enumerator or intermediate array per span. Two passes over // the (short) linked lists — count, then fill — keep the arrays exactly sized. + // Another thread can still add or remove tags/events/links on the activity between + // the passes, so the fill pass is bounded by the count and the array trimmed to what + // it actually saw. An exception here would escape through Activity.Stop() into + // whatever code stopped the activity. var tags = ToReportKeyValues(activity.EnumerateTagObjects()); SpanEvent[]? events = null; @@ -435,6 +439,11 @@ private void OnActivityStopped(Activity activity) var i = 0; foreach (ref readonly var evt in activity.EnumerateEvents()) { + if (i == events.Length) + { + break; + } + events[i++] = new SpanEvent { Name = evt.Name, @@ -442,6 +451,8 @@ private void OnActivityStopped(Activity activity) Tags = ToReportKeyValues(evt.EnumerateTagObjects()) }; } + + events = TrimToFilled(events, i); } var parentSpanId = activity.ParentSpanId != default ? activity.ParentSpanId.ToString() : null; @@ -459,12 +470,19 @@ private void OnActivityStopped(Activity activity) var i = 0; foreach (ref readonly var link in activity.EnumerateLinks()) { + if (i == links.Length) + { + break; + } + links[i++] = new SpanLink { TraceId = link.Context.TraceId.ToString(), SpanId = link.Context.SpanId.ToString() }; } + + links = TrimToFilled(links, i); } var statusStr = activity.Status switch @@ -522,6 +540,11 @@ private void OnActivityStopped(Activity activity) var i = 0; foreach (ref readonly var tag in tagObjects) { + if (i == result.Length) + { + break; + } + result[i++] = new ReportKeyValue { Key = tag.Key, @@ -529,7 +552,23 @@ private void OnActivityStopped(Activity activity) }; } - return result; + return TrimToFilled(result, i); + } + + private static T[]? TrimToFilled(T[] array, int filled) + { + if (filled == array.Length) + { + return array; + } + + if (filled == 0) + { + return null; + } + + Array.Resize(ref array, filled); + return array; } public void Dispose() diff --git a/src/TUnit.Engine/TestDiscoveryService.cs b/src/TUnit.Engine/TestDiscoveryService.cs index c4de2871059..42295790438 100644 --- a/src/TUnit.Engine/TestDiscoveryService.cs +++ b/src/TUnit.Engine/TestDiscoveryService.cs @@ -116,12 +116,42 @@ public async Task DiscoverTests(string testSessionId, ITest var finalTests = filteredTests as List ?? [.. filteredTests]; + if (isForExecution && finalTests.Count != allTests.Count) + { + RemoveExcludedTestsFromHookContexts(allTests, finalTests); + } + await _testFilterService.RegisterTestsAsync(finalTests, isForExecution).ConfigureAwait(false); var finalContext = ExecutionContext.Capture(); return new TestDiscoveryResult(finalTests, finalContext); } + // Every built test was added to its ClassHookContext when its TestContext was created. Filters + // that can only be evaluated after building (e.g. on properties added at discovery time) leave + // tests in those contexts that will never run, so hooks would otherwise see them through + // TestClasses/AllTests/TestCount and fire work for classes that are not part of the run. + private static void RemoveExcludedTestsFromHookContexts( + List allTests, + List finalTests) + { + var testsToKeep = new HashSet(TUnit.Core.Helpers.ReferenceEqualityComparer.Instance); + foreach (var test in finalTests) + { + testsToKeep.Add(test.Context); + } + + var visitedClasses = new HashSet(TUnit.Core.Helpers.ReferenceEqualityComparer.Instance); + foreach (var test in allTests) + { + var classContext = test.Context.ClassContext; + if (!testsToKeep.Contains(test.Context) && visitedClasses.Add(classContext)) + { + classContext.RetainTests(testsToKeep); + } + } + } + // Project the resolved tests onto their contexts into a pre-sized list. // TestDiscoveryContext.AddTests stores its argument directly when it is already an // IReadOnlyList and only calls ToArray() otherwise. A List diff --git a/tests/TUnit.TestProject/Bugs/5700/Repro.cs b/tests/TUnit.TestProject/Bugs/5700/Repro.cs index 28d7366e403..32ff03f7ed7 100644 --- a/tests/TUnit.TestProject/Bugs/5700/Repro.cs +++ b/tests/TUnit.TestProject/Bugs/5700/Repro.cs @@ -13,8 +13,14 @@ namespace TUnit.TestProject.Bugs._5700; /// /// The test uses a rendezvous (TaskCompletionSource) rather than wall-clock /// sampling so it stays deterministic on slow CI runners: Test1 waits for -/// any Test2 invocation to start, and each Test2 invocation waits for Test1 -/// to be live. Either side timing out → fix is broken. +/// any Test2 invocation to start. Under the bug Test2 could only start after +/// Test1 (and every other unconstrained test) finished, so Test1 times out. +/// +/// Only Test1 waits. Test1 sits in the shared parallel queue, so on a busy +/// runner it can be dispatched long after the keyed lane has started Test2. +/// Making Test2 wait for Test1 as well (as an earlier version did) measured +/// queue depth rather than the bug, and flaked when Test1 was dispatched more +/// than a minute after Test2. /// /// Test2 also asserts the keyed constraint still serializes its own invocations. /// @@ -23,25 +29,20 @@ public class Repro5700 { private const string Test2Key = "Tests.Test2"; - private static readonly TaskCompletionSource Test1Live = new(TaskCreationOptions.RunContinuationsAsynchronously); private static readonly TaskCompletionSource Test2Live = new(TaskCreationOptions.RunContinuationsAsynchronously); private static int _test2Active; private static int _test2ConcurrentViolations; // Deadline is generous because the engine-test harness runs Repro5700 inside a // subprocess populated with every `[EngineTest=Pass]` test in the project - // (hundreds). On a busy CI runner the parallel queue can be saturated by other - // unconstrained tests, so allow a full minute for either side to be dispatched. - // The bug being guarded — keyed tests deferred behind the entire parallel - // bucket — would manifest as Test1Live/Test2Live never being set at all, not as - // a 60-second scheduling delay. + // (hundreds). The bug being guarded — keyed tests deferred behind the entire + // parallel bucket — would manifest as Test2Live never being set while Test1 + // is running, not as a 60-second scheduling delay. private static readonly TimeSpan RendezvousTimeout = TimeSpan.FromSeconds(60); [Test] public async Task Test1_RunsAlongsideKeyedTest2() { - Test1Live.TrySetResult(); - using var cts = new CancellationTokenSource(RendezvousTimeout); await Test2Live.Task.WaitAsync(cts.Token); } @@ -62,9 +63,6 @@ public async Task Test2_KeyedSerializes(int param) Test2Live.TrySetResult(); - using var cts = new CancellationTokenSource(RendezvousTimeout); - await Test1Live.Task.WaitAsync(cts.Token); - // Hold the key briefly so the second Test2 invocation queues behind // us, exposing any concurrency-violation in keyed serialization. await Task.Delay(200); diff --git a/tests/TUnit.TestProject/FilterByDynamicAddedPropertyTests.cs b/tests/TUnit.TestProject/FilterByDynamicAddedPropertyTests.cs index ede2552a2a1..5cd9d72aec9 100644 --- a/tests/TUnit.TestProject/FilterByDynamicAddedPropertyTests.cs +++ b/tests/TUnit.TestProject/FilterByDynamicAddedPropertyTests.cs @@ -6,8 +6,17 @@ namespace TUnit.TestProject; public class FilterByDynamicAddedPropertyTests { [Test] - public void Test1() + public async Task Test1() { + // This class is only ever run through a property filter, which cannot be applied until every + // test in the assembly has been built. Tests the filter excluded must not leak into the hook + // contexts, or assembly/session hooks act on classes that are not part of the run. + var sessionClasses = TestSessionContext.Current!.TestClasses.Select(c => c.ClassType).Distinct().ToArray(); + var assemblyClasses = TestContext.Current!.ClassContext.AssemblyContext.TestClasses.Select(c => c.ClassType).Distinct().ToArray(); + + await Assert.That(sessionClasses).IsEquivalentTo([typeof(FilterByDynamicAddedPropertyTests)]); + await Assert.That(assemblyClasses).IsEquivalentTo([typeof(FilterByDynamicAddedPropertyTests)]); + await Assert.That(TestSessionContext.Current.AllTests).Count().IsEqualTo(1); } public class MyDynamicallyAddedPropertyAttribute : Attribute, ITestDiscoveryEventReceiver diff --git a/tests/TUnit.UnitTests/ContextProviderTests.cs b/tests/TUnit.UnitTests/ContextProviderTests.cs index 28d4558f93d..65dca29df65 100644 --- a/tests/TUnit.UnitTests/ContextProviderTests.cs +++ b/tests/TUnit.UnitTests/ContextProviderTests.cs @@ -64,6 +64,106 @@ public async Task CreateTestContext_PublishesContextWithTestDetailsAlreadyAssign await Assert.That(publishedContext.Metadata.TestDetails).IsSameReferenceAs(testDetails); } + [Test] + public async Task GetOrCreateClassContext_ConcurrentCallers_RegisterASingleContext() + { + // Contexts register themselves with their parent when constructed. Creating them through + // a racing ConcurrentDictionary.GetOrAdd factory left the losers registered as orphaned, + // test-less entries in AssemblyHookContext.TestClasses / TestSessionContext.Assemblies. + for (var attempt = 0; attempt < 50; attempt++) + { + var provider = new ContextProvider(new EmptyServiceProvider(), Guid.NewGuid().ToString(), testFilter: null); + // The engine creates the session context before building tests; do the same so only + // the class/assembly creation path races. + var session = provider.TestSessionContext; + using var start = new ManualResetEventSlim(); + + var workers = Enumerable.Range(0, Math.Clamp(Environment.ProcessorCount, 4, 16)) + .Select(_ => Task.Factory.StartNew(() => + { + start.Wait(); + return provider.GetOrCreateClassContext(typeof(DummyTestClass)); + }, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default)) + .ToArray(); + + start.Set(); + var contexts = await Task.WhenAll(workers); + + await Assert.That(contexts.All(c => ReferenceEquals(c, contexts[0]))).IsTrue(); + await Assert.That(session.Assemblies).Count().IsEqualTo(1); + await Assert.That(session.TestClasses).Count().IsEqualTo(1); + } + } + + [Test] + public async Task AddTest_AfterFilteringEmptiedClass_ReattachesClassAndAssembly() + { + // Filtering can remove every built test of a class, which detaches the class (and its + // assembly) from the hook contexts. A dynamic test added to that class at run time must + // make it visible to assembly and session hooks again. + var provider = new ContextProvider(new EmptyServiceProvider(), Guid.NewGuid().ToString(), testFilter: null); + var session = provider.TestSessionContext; + + CreateDummyTestContext(provider, "Test:0"); + var classContext = provider.GetOrCreateClassContext(typeof(DummyTestClass)); + + classContext.RetainTests(new HashSet()); + + await Assert.That(session.Assemblies).IsEmpty(); + await Assert.That(session.TestClasses).IsEmpty(); + + var dynamicTest = CreateDummyTestContext(provider, "Test:1"); + + await Assert.That(session.Assemblies).Count().IsEqualTo(1); + await Assert.That(session.TestClasses).Count().IsEqualTo(1); + await Assert.That(classContext.AssemblyContext.AllTests).Contains(dynamicTest); + await Assert.That(session.AllTests).Contains(dynamicTest); + } + + private static TestContext CreateDummyTestContext(ContextProvider provider, string testId) + { + var classMetadata = new ClassMetadata + { + Type = typeof(DummyTestClass), + TypeInfo = new ConcreteType(typeof(DummyTestClass)), + Name = nameof(DummyTestClass), + Namespace = typeof(DummyTestClass).Namespace ?? string.Empty, + Assembly = new AssemblyMetadata + { + Name = typeof(DummyTestClass).Assembly.GetName().Name ?? string.Empty + }, + Parent = null, + Parameters = [], + Properties = [] + }; + + var methodMetadata = MethodMetadataFactory.Create( + nameof(DummyTestClass.SomeTest), + typeof(DummyTestClass), + typeof(Task), + classMetadata); + + var testDetails = new TestDetails([]) + { + TestId = testId, + TestName = nameof(DummyTestClass.SomeTest), + ClassType = typeof(DummyTestClass), + MethodName = nameof(DummyTestClass.SomeTest), + ClassInstance = PlaceholderInstance.Instance, + TestMethodArguments = [], + TestClassArguments = [], + MethodMetadata = methodMetadata, + ReturnType = typeof(Task), + AttributesByType = new Dictionary>() + }; + + return provider.CreateTestContext( + typeof(DummyTestClass), + new TestBuilderContext { TestMetadata = methodMetadata }, + testDetails, + CancellationToken.None); + } + private sealed class DummyTestClass { public Task SomeTest() => Task.CompletedTask; diff --git a/tests/TUnit.UnitTests/SessionActivityLifecycleTests.cs b/tests/TUnit.UnitTests/SessionActivityLifecycleTests.cs index 5e06b11cca0..cc7c2efd00e 100644 --- a/tests/TUnit.UnitTests/SessionActivityLifecycleTests.cs +++ b/tests/TUnit.UnitTests/SessionActivityLifecycleTests.cs @@ -430,19 +430,37 @@ public ValueTask>> Collect /// Manages an scoped to a test, ensuring /// cleanup even if the test fails. /// + /// + /// The listener sees every activity on the TUnit sources in the process, including the + /// engine's own spans for tests running in parallel with this one. Only activities started + /// from this test's async flow are tracked, so never stops a span + /// the engine is still writing to on another thread. + /// private sealed class ActivityListenerScope : IDisposable { + private static readonly AsyncLocal CurrentScope = new(); + private readonly ActivityListener _listener; private readonly ConcurrentBag _activities = []; + private readonly ActivityListenerScope? _previousScope; public ActivityListenerScope() { + _previousScope = CurrentScope.Value; + CurrentScope.Value = this; + _listener = new ActivityListener { ShouldListenTo = static source => source.Name is TUnitActivitySource.SourceName or TUnitActivitySource.LifecycleSourceName, Sample = static (ref ActivityCreationOptions _) => ActivitySamplingResult.AllDataAndRecorded, - ActivityStarted = activity => _activities.Add(activity), + ActivityStarted = activity => + { + if (ReferenceEquals(CurrentScope.Value, this)) + { + _activities.Add(activity); + } + }, }; ActivitySource.AddActivityListener(_listener); @@ -450,6 +468,8 @@ public ActivityListenerScope() public void Dispose() { + CurrentScope.Value = _previousScope; + // Stop any activities that are still running to prevent leaks foreach (var activity in _activities) {