diff --git a/TUnit.AspNetCore.Tests/AutoConfigureOpenTelemetryTests.cs b/TUnit.AspNetCore.Tests/AutoConfigureOpenTelemetryTests.cs index 7dbe558dae..d83310d5ed 100644 --- a/TUnit.AspNetCore.Tests/AutoConfigureOpenTelemetryTests.cs +++ b/TUnit.AspNetCore.Tests/AutoConfigureOpenTelemetryTests.cs @@ -1,3 +1,4 @@ +using System.Collections; using System.Diagnostics; using Microsoft.Extensions.DependencyInjection; using OpenTelemetry.Trace; @@ -12,15 +13,16 @@ namespace TUnit.AspNetCore.Tests; /// correlation processor + ASP.NET Core instrumentation. /// /// -/// Serialized against sibling auto-wire tests because -/// attaches a process-global per TracerProvider, -/// so a parallel factory's correlation processor can tag activities created by another -/// factory's SUT. Serializing keeps assertions observing only their own factory's wiring. +/// AutoWires asserts presence of its own TestId tag, so foreign spans observed +/// via the global ASP.NET Core do not +/// affect it — no key needed. Parallel +/// instances stamp activities (subscribed via AddAspNetCoreInstrumentation) on +/// background continuations, so the exporter sink must be thread-safe — polling +/// snapshots its items under a lock. /// -[NotInParallel(nameof(AutoConfigureOpenTelemetryTests))] public class AutoConfigureOpenTelemetryTests : WebApplicationTest { - private readonly List _exported = []; + private readonly LockedActivityList _exported = []; protected override void ConfigureTestServices(IServiceCollection services) { @@ -42,7 +44,14 @@ public async Task AutoWires_TagsAspNetCoreSpans_WithTestId() Activity? taggedSpan = null; while (Environment.TickCount64 < deadline) { - taggedSpan = _exported.FirstOrDefault(a => (a.GetTagItem(TUnitActivitySource.TagTestId) as string) == testId); + foreach (var activity in _exported.Snapshot()) + { + if (activity.GetTagItem(TUnitActivitySource.TagTestId) as string == testId) + { + taggedSpan = activity; + break; + } + } if (taggedSpan is not null) { break; @@ -54,10 +63,19 @@ public async Task AutoWires_TagsAspNetCoreSpans_WithTestId() } } -[NotInParallel(nameof(AutoConfigureOpenTelemetryTests))] +/// +/// Global (no key): asserts absence of any +/// tag, but every +/// with AddAspNetCoreInstrumentation() subscribes to the process-global +/// Microsoft.AspNetCore and a parallel +/// factory's correlation processor stamps spans with its own TestId before they reach +/// this exporter. A keyed constraint cannot enumerate every parallel ; +/// draining alone is the only reliable isolation until per-factory filtering lands. +/// +[NotInParallel] public class AutoConfigureOpenTelemetryOptOutTests : WebApplicationTest { - private readonly List _exported = []; + private readonly LockedActivityList _exported = []; protected override void ConfigureTestOptions(WebApplicationTestOptions options) { @@ -78,9 +96,44 @@ public async Task OptOut_DoesNotTag_AspNetCoreSpans() var response = await client.GetAsync("/ping"); response.EnsureSuccessStatusCode(); - foreach (var activity in _exported) + foreach (var activity in _exported.Snapshot()) { await Assert.That(activity.GetTagItem(TUnitActivitySource.TagTestId)).IsNull(); } } } + +/// +/// OpenTelemetry's AddInMemoryExporter calls ICollection<T>.Add on a +/// background batch thread without locking, so a plain races with +/// any reader (the test's poll loop) and intermittently throws "Collection was modified". +/// This wrapper synchronises every mutation and exposes a for +/// safe iteration. +/// +internal sealed class LockedActivityList : ICollection +{ + private readonly List _items = []; + + public int Count + { + get { lock (_items) return _items.Count; } + } + + public bool IsReadOnly => false; + + public void Add(Activity item) { lock (_items) _items.Add(item); } + + public void Clear() { lock (_items) _items.Clear(); } + + public bool Contains(Activity item) { lock (_items) return _items.Contains(item); } + + public void CopyTo(Activity[] array, int arrayIndex) { lock (_items) _items.CopyTo(array, arrayIndex); } + + public bool Remove(Activity item) { lock (_items) return _items.Remove(item); } + + public Activity[] Snapshot() { lock (_items) return _items.ToArray(); } + + public IEnumerator GetEnumerator() => ((IEnumerable)Snapshot()).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); +} diff --git a/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelCrossPhaseTests.cs b/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelCrossPhaseTests.cs new file mode 100644 index 0000000000..4b8bc1c4e0 --- /dev/null +++ b/TUnit.Engine.Tests/Scheduling/KeyedNotInParallelCrossPhaseTests.cs @@ -0,0 +1,43 @@ +using Shouldly; +using TUnit.Engine.Tests.Enums; + +namespace TUnit.Engine.Tests.Scheduling; + +/// +/// Regression for https://github.com/thomhurst/TUnit/discussions/5700. +/// An unconstrained [Test] must run concurrently with [Test, NotInParallel("key")] tests +/// in the same class — keyed NotInParallel only blocks tests sharing a key, it must not +/// serialize against the rest of the suite. +/// +public class KeyedNotInParallelCrossPhaseTests(TestMode testMode) : InvokableTestBase(testMode) +{ + [Test] + public async Task UnconstrainedTest_RunsAlongsideKeyedNotInParallelTest() + { + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + + await RunTestsWithFilter("/*/*/Repro5700/*", + [ + result => result.ResultSummary.Outcome.ShouldBe("Completed"), + result => result.ResultSummary.Counters.Total.ShouldBe(3), + result => result.ResultSummary.Counters.Passed.ShouldBe(3), + result => result.ResultSummary.Counters.Failed.ShouldBe(0), + ], + new RunOptions().WithForcefulCancellationToken(cts.Token)); + } + + [Test] + public async Task KeyedNotInParallel_DifferentKeys_RunInParallel() + { + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + + await RunTestsWithFilter("/*/*/CrossKeyOverlap5700/*", + [ + result => result.ResultSummary.Outcome.ShouldBe("Completed"), + result => result.ResultSummary.Counters.Total.ShouldBe(2), + result => result.ResultSummary.Counters.Passed.ShouldBe(2), + result => result.ResultSummary.Counters.Failed.ShouldBe(0), + ], + new RunOptions().WithForcefulCancellationToken(cts.Token)); + } +} diff --git a/TUnit.Engine/Scheduling/TestScheduler.cs b/TUnit.Engine/Scheduling/TestScheduler.cs index ea64e58369..fa7b6d834b 100644 --- a/TUnit.Engine/Scheduling/TestScheduler.cs +++ b/TUnit.Engine/Scheduling/TestScheduler.cs @@ -171,13 +171,44 @@ private async Task ExecuteGroupedTestsAsync( // Start dynamic test queue processing in background var dynamicTestProcessingTask = ProcessDynamicTestQueueAsync(cancellationToken); + await ExecuteAllPhasesAsync(groupedTests, cancellationToken).ConfigureAwait(false); + + // Mark the queue as complete and wait for remaining dynamic tests to finish + _dynamicTestQueue.Complete(); + await dynamicTestProcessingTask.ConfigureAwait(false); + } + + #if NET + [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Test execution involves reflection for hooks and initialization")] + #endif + private async Task ExecuteAllPhasesAsync(GroupedTests groupedTests, CancellationToken cancellationToken) + { + // Unconstrained Parallel and KeyedNotInParallel buckets share no state — keyed tests + // only block other tests sharing a key (per docs/parallelism.md). Running them + // sequentially makes a `[Test] T1` always run before `[Test, NotInParallel("k")] T2` + // which is observable as "Test1 never overlaps Test2" (discussion #5700). Run them + // concurrently instead. ParallelGroups still serialize (cross-group exclusion) and + // global NotInParallel still drains last (must run alone). + Task? parallelPhase = null; if (groupedTests.Parallel.Length > 0) { if (_logger.IsTraceEnabled) await _logger.LogTraceAsync($"Starting {groupedTests.Parallel.Length} parallel tests").ConfigureAwait(false); - await ExecuteTestsAsync(groupedTests.Parallel, cancellationToken).ConfigureAwait(false); + parallelPhase = ExecuteTestsAsync(groupedTests.Parallel, cancellationToken); + } + + Task? keyedPhase = null; + if (groupedTests.KeyedNotInParallel.Length > 0) + { + if (_logger.IsTraceEnabled) + await _logger.LogTraceAsync($"Starting {groupedTests.KeyedNotInParallel.Length} keyed NotInParallel tests").ConfigureAwait(false); + keyedPhase = _constraintKeyScheduler.ExecuteTestsWithConstraintsAsync(groupedTests.KeyedNotInParallel, cancellationToken).AsTask(); } + await RunPhasesConcurrentlyAsync(parallelPhase, keyedPhase, cancellationToken).ConfigureAwait(false); + + // Each ParallelGroup runs to completion before the next — tests are bucketed + // into a group precisely because they must not overlap with tests in another group. foreach (var group in groupedTests.ParallelGroups) { var totalCount = 0; @@ -203,26 +234,13 @@ private async Task ExecuteGroupedTestsAsync( if (_logger.IsTraceEnabled) await _logger.LogTraceAsync($"Starting constrained parallel group '{kvp.Key}' with {constrainedTests.UnconstrainedTests.Length} unconstrained and {constrainedTests.KeyedTests.Length} keyed tests").ConfigureAwait(false); - var tasks = new List(); - if (constrainedTests.UnconstrainedTests.Length > 0) - { - tasks.Add(ExecuteTestsAsync(constrainedTests.UnconstrainedTests, cancellationToken)); - } - if (constrainedTests.KeyedTests.Length > 0) - { - tasks.Add(_constraintKeyScheduler.ExecuteTestsWithConstraintsAsync(constrainedTests.KeyedTests, cancellationToken).AsTask()); - } - if (tasks.Count > 0) - { - await WaitForTasksWithFailFastHandling(tasks.ToArray(), cancellationToken).ConfigureAwait(false); - } - } - - if (groupedTests.KeyedNotInParallel.Length > 0) - { - if (_logger.IsTraceEnabled) - await _logger.LogTraceAsync($"Starting {groupedTests.KeyedNotInParallel.Length} keyed NotInParallel tests").ConfigureAwait(false); - await _constraintKeyScheduler.ExecuteTestsWithConstraintsAsync(groupedTests.KeyedNotInParallel, cancellationToken).ConfigureAwait(false); + var unconstrainedPhase = constrainedTests.UnconstrainedTests.Length > 0 + ? ExecuteTestsAsync(constrainedTests.UnconstrainedTests, cancellationToken) + : null; + var groupKeyedPhase = constrainedTests.KeyedTests.Length > 0 + ? _constraintKeyScheduler.ExecuteTestsWithConstraintsAsync(constrainedTests.KeyedTests, cancellationToken).AsTask() + : null; + await RunPhasesConcurrentlyAsync(unconstrainedPhase, groupKeyedPhase, cancellationToken).ConfigureAwait(false); } if (groupedTests.NotInParallel.Length > 0) @@ -231,10 +249,6 @@ private async Task ExecuteGroupedTestsAsync( await _logger.LogTraceAsync($"Starting {groupedTests.NotInParallel.Length} global NotInParallel tests").ConfigureAwait(false); await ExecuteSequentiallyAsync(groupedTests.NotInParallel, cancellationToken).ConfigureAwait(false); } - - // Mark the queue as complete and wait for remaining dynamic tests to finish - _dynamicTestQueue.Complete(); - await dynamicTestProcessingTask.ConfigureAwait(false); } #if NET @@ -262,21 +276,7 @@ private async Task ProcessDynamicTestQueueAsync(CancellationToken cancellationTo if (_logger.IsTraceEnabled) await _logger.LogTraceAsync($"Executing {dynamicTests.Count} dynamic test(s)").ConfigureAwait(false); - // Group and execute just like regular tests - var dynamicTestsArray = dynamicTests.ToArray(); - var groupedDynamicTests = await _groupingService.GroupTestsByConstraintsAsync(dynamicTestsArray).ConfigureAwait(false); - - // Execute the grouped dynamic tests (recursive call handles sub-dynamics) - if (groupedDynamicTests.Parallel.Length > 0) - { - await ExecuteTestsAsync(groupedDynamicTests.Parallel, cancellationToken).ConfigureAwait(false); - } - - if (groupedDynamicTests.NotInParallel.Length > 0) - { - await ExecuteSequentiallyAsync(groupedDynamicTests.NotInParallel, cancellationToken).ConfigureAwait(false); - } - + await ExecuteDynamicBatchAsync(dynamicTests, cancellationToken).ConfigureAwait(false); dynamicTests.Clear(); } } @@ -295,21 +295,22 @@ private async Task ProcessDynamicTestQueueAsync(CancellationToken cancellationTo if (_logger.IsTraceEnabled) await _logger.LogTraceAsync($"Executing {dynamicTests.Count} remaining dynamic test(s)").ConfigureAwait(false); - var dynamicTestsArray = dynamicTests.ToArray(); - var groupedDynamicTests = await _groupingService.GroupTestsByConstraintsAsync(dynamicTestsArray).ConfigureAwait(false); - - if (groupedDynamicTests.Parallel.Length > 0) - { - await ExecuteTestsAsync(groupedDynamicTests.Parallel, cancellationToken).ConfigureAwait(false); - } - - if (groupedDynamicTests.NotInParallel.Length > 0) - { - await ExecuteSequentiallyAsync(groupedDynamicTests.NotInParallel, cancellationToken).ConfigureAwait(false); - } + await ExecuteDynamicBatchAsync(dynamicTests, cancellationToken).ConfigureAwait(false); } } + #if NET + [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Test execution involves reflection for hooks and initialization")] + #endif + private async Task ExecuteDynamicBatchAsync(List dynamicTests, CancellationToken cancellationToken) + { + // Route through the same phase pipeline as the main test set so dynamically-added + // tests with [NotInParallel(key)], [ParallelGroup(...)] etc. honour their constraints + // instead of being silently dropped. + var groupedDynamicTests = await _groupingService.GroupTestsByConstraintsAsync(dynamicTests.ToArray()).ConfigureAwait(false); + await ExecuteAllPhasesAsync(groupedDynamicTests, cancellationToken).ConfigureAwait(false); + } + #if NET [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Test execution involves reflection for hooks and initialization")] #endif @@ -389,6 +390,17 @@ private async Task ExecuteWithGlobalLimitAsync( } #endif + // The cancellation token is forwarded only so WaitForTasksWithFailFastHandling can + // distinguish a fail-fast cancellation from a normal task fault when both phases run. + // Tasks `a` and `b` are already started and carry their own token internally, so the + // single-phase short-circuits do not need to inspect it. + private Task RunPhasesConcurrentlyAsync(Task? a, Task? b, CancellationToken cancellationToken) + { + if (a is null) return b ?? Task.CompletedTask; + if (b is null) return a; + return WaitForTasksWithFailFastHandling([a, b], cancellationToken); + } + private async Task WaitForTasksWithFailFastHandling(IEnumerable tasks, CancellationToken cancellationToken) { try diff --git a/TUnit.TestProject/Bugs/5700/CrossKeyOverlap.cs b/TUnit.TestProject/Bugs/5700/CrossKeyOverlap.cs new file mode 100644 index 0000000000..5df8529b9b --- /dev/null +++ b/TUnit.TestProject/Bugs/5700/CrossKeyOverlap.cs @@ -0,0 +1,40 @@ +using TUnit.TestProject.Attributes; + +namespace TUnit.TestProject.Bugs._5700; + +/// +/// Companion to Repro5700. Verifies the symmetric scenario for #5700: +/// two [NotInParallel] tests with different keys live in the same keyed +/// bucket and must overlap each other (their keys do not intersect). +/// Uses a TaskCompletionSource rendezvous so it does not depend on timing. +/// +[EngineTest(ExpectedResult.Pass)] +public class CrossKeyOverlap5700 +{ + private const string KeyA = "Tests.5700.A"; + private const string KeyB = "Tests.5700.B"; + + private static readonly TaskCompletionSource KeyALive = new(TaskCreationOptions.RunContinuationsAsynchronously); + private static readonly TaskCompletionSource KeyBLive = new(TaskCreationOptions.RunContinuationsAsynchronously); + + // See Repro5700 for why the rendezvous deadline is generous. + private static readonly TimeSpan RendezvousTimeout = TimeSpan.FromSeconds(60); + + [Test] + [NotInParallel(KeyA)] + public async Task KeyedA_RunsAlongsideKeyedB() + { + KeyALive.TrySetResult(); + using var cts = new CancellationTokenSource(RendezvousTimeout); + await KeyBLive.Task.WaitAsync(cts.Token); + } + + [Test] + [NotInParallel(KeyB)] + public async Task KeyedB_RunsAlongsideKeyedA() + { + KeyBLive.TrySetResult(); + using var cts = new CancellationTokenSource(RendezvousTimeout); + await KeyALive.Task.WaitAsync(cts.Token); + } +} diff --git a/TUnit.TestProject/Bugs/5700/Repro.cs b/TUnit.TestProject/Bugs/5700/Repro.cs new file mode 100644 index 0000000000..28d7366e40 --- /dev/null +++ b/TUnit.TestProject/Bugs/5700/Repro.cs @@ -0,0 +1,83 @@ +using TUnit.TestProject.Attributes; + +namespace TUnit.TestProject.Bugs._5700; + +/// +/// Regression test for https://github.com/thomhurst/TUnit/discussions/5700. +/// +/// Test1 has no parallel constraint. Test2 has [NotInParallel(Test2Key)] — a +/// keyed constraint that should only block other tests sharing the key (per +/// docs/execution/parallelism.md). The buggy scheduler ran all unconstrained +/// tests to completion before starting keyed-NotInParallel tests, so Test1 +/// never overlapped Test2. +/// +/// 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. +/// +/// Test2 also asserts the keyed constraint still serializes its own invocations. +/// +[EngineTest(ExpectedResult.Pass)] +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. + 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); + } + + [Test] + [Arguments(1)] + [Arguments(2)] + [NotInParallel(Test2Key)] + public async Task Test2_KeyedSerializes(int param) + { + var concurrent = Interlocked.Increment(ref _test2Active); + try + { + if (concurrent > 1) + { + Interlocked.Increment(ref _test2ConcurrentViolations); + } + + 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); + } + finally + { + Interlocked.Decrement(ref _test2Active); + } + } + + [After(Class)] + public static async Task AssertKeyedSerialization() + { + await Assert.That(_test2ConcurrentViolations).IsZero(); + } +} diff --git a/examples/CloudShop/CloudShop.Tests/Tests/Orders/OrderWorkflowTests.cs b/examples/CloudShop/CloudShop.Tests/Tests/Orders/OrderWorkflowTests.cs index b1c6b53c4c..f8fe4588cc 100644 --- a/examples/CloudShop/CloudShop.Tests/Tests/Orders/OrderWorkflowTests.cs +++ b/examples/CloudShop/CloudShop.Tests/Tests/Orders/OrderWorkflowTests.cs @@ -88,11 +88,13 @@ public async Task Step3_Worker_Fulfills_Order() // The Worker service listens for PaymentProcessed events on RabbitMQ // and updates the order status to Fulfilled. // WaitsFor polls repeatedly until the assertion passes or the timeout expires. + // Generous timeout so the assertion doesn't trip on a busy CI runner where + // RabbitMQ delivery + Worker processing can lag behind under concurrent load. var order = await Assert.That(async () => await Customer.Client.GetFromJsonAsync($"/api/orders/{orderId}")) .WaitsFor( assert => assert.Satisfies(o => o?.Status == OrderStatus.Fulfilled), - timeout: TimeSpan.FromSeconds(25), + timeout: TimeSpan.FromSeconds(120), pollingInterval: TimeSpan.FromMilliseconds(500)); await Assert.That(order).IsNotNull();