fix(engine): allow keyed NotInParallel tests to run alongside unconstrained tests (#5700)

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.
 /// </summary>
 /// <remarks>
-/// Serialized against sibling auto-wire tests because <see cref="OpenTelemetry.Sdk"/>
-/// attaches a process-global <see cref="ActivityListener"/> per <c>TracerProvider</c>,
-/// 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 <c>TestId</c> tag, so foreign spans observed
+/// via the global ASP.NET Core <see cref="System.Diagnostics.ActivitySource"/> do not
+/// affect it — no key needed. Parallel <see cref="WebApplicationTest{TFactory,TEntryPoint}"/>
+/// instances stamp activities (subscribed via <c>AddAspNetCoreInstrumentation</c>) on
+/// background continuations, so the exporter sink must be thread-safe — polling
+/// <see cref="LockedActivityList"/> snapshots its items under a lock.
 /// </remarks>
-[NotInParallel(nameof(AutoConfigureOpenTelemetryTests))]
 public class AutoConfigureOpenTelemetryTests : WebApplicationTest<TestWebAppFactory, Program>
 {
-    private readonly List<Activity> _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))]
+/// <remarks>
+/// Global <see cref="NotInParallelAttribute"/> (no key): asserts <em>absence</em> of any
+/// <see cref="TUnitActivitySource.TagTestId"/> tag, but every <see cref="TracerProvider"/>
+/// with <c>AddAspNetCoreInstrumentation()</c> subscribes to the process-global
+/// <c>Microsoft.AspNetCore</c> <see cref="System.Diagnostics.ActivitySource"/> and a parallel
+/// factory's correlation processor stamps spans with its own <c>TestId</c> before they reach
+/// this exporter. A keyed constraint cannot enumerate every parallel <see cref="WebApplicationTest{TFactory,TEntryPoint}"/>;
+/// draining alone is the only reliable isolation until per-factory filtering lands.
+/// </remarks>
+[NotInParallel]
 public class AutoConfigureOpenTelemetryOptOutTests : WebApplicationTest<TestWebAppFactory, Program>
 {
-    private readonly List<Activity> _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();
         }
     }
 }
+
+/// <summary>
+/// OpenTelemetry's <c>AddInMemoryExporter</c> calls <c>ICollection&lt;T&gt;.Add</c> on a
+/// background batch thread without locking, so a plain <see cref="List{T}"/> races with
+/// any reader (the test's poll loop) and intermittently throws "Collection was modified".
+/// This wrapper synchronises every mutation and exposes a <see cref="Snapshot"/> for
+/// safe iteration.
+/// </summary>
+internal sealed class LockedActivityList : ICollection<Activity>
+{
+    private readonly List<Activity> _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<Activity> GetEnumerator() => ((IEnumerable<Activity>)Snapshot()).GetEnumerator();
+
+    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+}

TUnit.Engine.Tests/Scheduling/KeyedNotInParallelCrossPhaseTests.cs

@@ -0,0 +1,43 @@
+using Shouldly;
+using TUnit.Engine.Tests.Enums;
+
+namespace TUnit.Engine.Tests.Scheduling;
+
+/// <summary>
+/// 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.
+/// </summary>
+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));
+    }
+}

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<Task>();
-            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<AbstractExecutableTest> 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<Task> tasks, CancellationToken cancellationToken)
     {
         try

TUnit.TestProject/Bugs/5700/CrossKeyOverlap.cs

@@ -0,0 +1,40 @@
+using TUnit.TestProject.Attributes;
+
+namespace TUnit.TestProject.Bugs._5700;
+
+/// <summary>
+/// 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.
+/// </summary>
+[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);
+    }
+}