From 1e280cd24852e90a31c432bda5e638a2ea426f3f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 14:29:39 +0000 Subject: [PATCH 1/6] Initial plan From 92407d87e3d19014f03d74336db334d381bbca6a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 14:44:41 +0000 Subject: [PATCH 2/6] Fix flaky DiagnosticTests by clearing Activity.Current and forcing flush Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- tests/ModelContextProtocol.Tests/DiagnosticTests.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs index 55a3b4932..81a9d0fa7 100644 --- a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs +++ b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs @@ -15,6 +15,9 @@ public class DiagnosticTests [Fact] public async Task Session_TracksActivities() { + // Ensure no ambient activity from previous tests + Activity.Current = null; + var activities = new List(); var clientToServerLog = new List(); @@ -32,6 +35,9 @@ await RunConnected(async (client, server) => var tool = tools.First(t => t.Name == "DoubleValue"); await tool.InvokeAsync(new() { ["amount"] = 42 }, TestContext.Current.CancellationToken); }, clientToServerLog); + + // Ensure all activities are flushed before disposal + tracerProvider.ForceFlush(); } Assert.NotEmpty(activities); @@ -77,6 +83,9 @@ await RunConnected(async (client, server) => [Fact] public async Task Session_FailedToolCall() { + // Ensure no ambient activity from previous tests + Activity.Current = null; + var activities = new List(); using (var tracerProvider = OpenTelemetry.Sdk.CreateTracerProviderBuilder() @@ -89,6 +98,9 @@ await RunConnected(async (client, server) => await client.CallToolAsync("Throw", cancellationToken: TestContext.Current.CancellationToken); await Assert.ThrowsAsync(async () => await client.CallToolAsync("does-not-exist", cancellationToken: TestContext.Current.CancellationToken)); }, []); + + // Ensure all activities are flushed before disposal + tracerProvider.ForceFlush(); } Assert.NotEmpty(activities); From d92872a512a29d9ffca1b506fc3128030765b91a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 15:16:19 +0000 Subject: [PATCH 3/6] Use ExclusiveExecution collection fixture to ensure DiagnosticTests don't run concurrently with any other tests Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../DiagnosticTests.cs | 2 +- .../TestAttributes.cs | 35 +++++++++++++++---- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs index 81a9d0fa7..9cc9e42cf 100644 --- a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs +++ b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs @@ -9,7 +9,7 @@ namespace ModelContextProtocol.Tests; -[Collection(nameof(DisableParallelization))] +[Collection(nameof(ExclusiveExecution))] public class DiagnosticTests { [Fact] diff --git a/tests/ModelContextProtocol.Tests/TestAttributes.cs b/tests/ModelContextProtocol.Tests/TestAttributes.cs index 8a0140db8..a4d534fb7 100644 --- a/tests/ModelContextProtocol.Tests/TestAttributes.cs +++ b/tests/ModelContextProtocol.Tests/TestAttributes.cs @@ -1,9 +1,32 @@ -// Uncomment to disable parallel test execution for the whole assembly -//[assembly: CollectionBehavior(DisableTestParallelization = true)] +/// +/// Test collection for tests that require exclusive execution (no parallel execution with any other tests). +/// This is needed for tests that interact with global static state like ActivitySource in OpenTelemetry. +/// +/// +/// The fixture ensures mutual exclusion across ALL tests by using a static semaphore. +/// Tests in this collection will acquire the semaphore before running and release it after, +/// preventing any other test from running concurrently. +/// +[CollectionDefinition(nameof(ExclusiveExecution))] +public sealed class ExclusiveExecution : ICollectionFixture; /// -/// Enables test classes to individually be attributed as [Collection(nameof(DisableParallelization))] -/// to have those tests run non-concurrently with any other tests. +/// Fixture that ensures only one test runs at a time across the entire test assembly. /// -[CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)] -public sealed class DisableParallelization; \ No newline at end of file +public sealed class ExclusiveExecutionFixture : IAsyncLifetime +{ + private static readonly SemaphoreSlim s_exclusiveLock = new(1, 1); + + public async ValueTask InitializeAsync() + { + // Acquire the lock before any test in this collection starts + await s_exclusiveLock.WaitAsync(); + } + + public ValueTask DisposeAsync() + { + // Release the lock after the test completes + s_exclusiveLock.Release(); + return default; + } +} \ No newline at end of file From 259d8dfd19be2bb67b4897bb40fab4b2c18d9f01 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 15:33:00 +0000 Subject: [PATCH 4/6] Revert to original DisableParallelization collection, remove Activity.Current and ForceFlush changes Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../DiagnosticTests.cs | 14 +------- .../TestAttributes.cs | 35 ++++--------------- 2 files changed, 7 insertions(+), 42 deletions(-) diff --git a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs index 9cc9e42cf..55a3b4932 100644 --- a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs +++ b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs @@ -9,15 +9,12 @@ namespace ModelContextProtocol.Tests; -[Collection(nameof(ExclusiveExecution))] +[Collection(nameof(DisableParallelization))] public class DiagnosticTests { [Fact] public async Task Session_TracksActivities() { - // Ensure no ambient activity from previous tests - Activity.Current = null; - var activities = new List(); var clientToServerLog = new List(); @@ -35,9 +32,6 @@ await RunConnected(async (client, server) => var tool = tools.First(t => t.Name == "DoubleValue"); await tool.InvokeAsync(new() { ["amount"] = 42 }, TestContext.Current.CancellationToken); }, clientToServerLog); - - // Ensure all activities are flushed before disposal - tracerProvider.ForceFlush(); } Assert.NotEmpty(activities); @@ -83,9 +77,6 @@ await RunConnected(async (client, server) => [Fact] public async Task Session_FailedToolCall() { - // Ensure no ambient activity from previous tests - Activity.Current = null; - var activities = new List(); using (var tracerProvider = OpenTelemetry.Sdk.CreateTracerProviderBuilder() @@ -98,9 +89,6 @@ await RunConnected(async (client, server) => await client.CallToolAsync("Throw", cancellationToken: TestContext.Current.CancellationToken); await Assert.ThrowsAsync(async () => await client.CallToolAsync("does-not-exist", cancellationToken: TestContext.Current.CancellationToken)); }, []); - - // Ensure all activities are flushed before disposal - tracerProvider.ForceFlush(); } Assert.NotEmpty(activities); diff --git a/tests/ModelContextProtocol.Tests/TestAttributes.cs b/tests/ModelContextProtocol.Tests/TestAttributes.cs index a4d534fb7..8a0140db8 100644 --- a/tests/ModelContextProtocol.Tests/TestAttributes.cs +++ b/tests/ModelContextProtocol.Tests/TestAttributes.cs @@ -1,32 +1,9 @@ -/// -/// Test collection for tests that require exclusive execution (no parallel execution with any other tests). -/// This is needed for tests that interact with global static state like ActivitySource in OpenTelemetry. -/// -/// -/// The fixture ensures mutual exclusion across ALL tests by using a static semaphore. -/// Tests in this collection will acquire the semaphore before running and release it after, -/// preventing any other test from running concurrently. -/// -[CollectionDefinition(nameof(ExclusiveExecution))] -public sealed class ExclusiveExecution : ICollectionFixture; +// Uncomment to disable parallel test execution for the whole assembly +//[assembly: CollectionBehavior(DisableTestParallelization = true)] /// -/// Fixture that ensures only one test runs at a time across the entire test assembly. +/// Enables test classes to individually be attributed as [Collection(nameof(DisableParallelization))] +/// to have those tests run non-concurrently with any other tests. /// -public sealed class ExclusiveExecutionFixture : IAsyncLifetime -{ - private static readonly SemaphoreSlim s_exclusiveLock = new(1, 1); - - public async ValueTask InitializeAsync() - { - // Acquire the lock before any test in this collection starts - await s_exclusiveLock.WaitAsync(); - } - - public ValueTask DisposeAsync() - { - // Release the lock after the test completes - s_exclusiveLock.Release(); - return default; - } -} \ No newline at end of file +[CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)] +public sealed class DisableParallelization; \ No newline at end of file From 57b9aaf01beabc40ca253136adb9c234551d421e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 16:02:44 +0000 Subject: [PATCH 5/6] Add ExclusiveExecution collection to prevent DiagnosticTests from running in parallel with tests that create activities Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../DiagnosticTests.cs | 2 +- .../TestAttributes.cs | 36 +++++++++++++++---- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs index 55a3b4932..45b097704 100644 --- a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs +++ b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs @@ -9,7 +9,7 @@ namespace ModelContextProtocol.Tests; -[Collection(nameof(DisableParallelization))] +[Collection(nameof(ExclusiveExecution))] public class DiagnosticTests { [Fact] diff --git a/tests/ModelContextProtocol.Tests/TestAttributes.cs b/tests/ModelContextProtocol.Tests/TestAttributes.cs index 8a0140db8..87071b175 100644 --- a/tests/ModelContextProtocol.Tests/TestAttributes.cs +++ b/tests/ModelContextProtocol.Tests/TestAttributes.cs @@ -1,9 +1,33 @@ -// Uncomment to disable parallel test execution for the whole assembly -//[assembly: CollectionBehavior(DisableTestParallelization = true)] +/// +/// Test collection for tests that require exclusive execution (no parallel execution with any other tests). +/// This is needed for tests that use OpenTelemetry TracerProvider with the global static ActivitySource. +/// Without this, the TracerProvider would collect activities from ALL tests running in parallel. +/// +/// +/// The fixture ensures mutual exclusion across ALL tests by using a static semaphore. +/// Tests in this collection will acquire the semaphore before running and release it after, +/// preventing any other test from running concurrently. +/// +[CollectionDefinition(nameof(ExclusiveExecution))] +public sealed class ExclusiveExecution : ICollectionFixture; /// -/// Enables test classes to individually be attributed as [Collection(nameof(DisableParallelization))] -/// to have those tests run non-concurrently with any other tests. +/// Fixture that ensures only one test runs at a time across the entire test assembly. /// -[CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)] -public sealed class DisableParallelization; \ No newline at end of file +public sealed class ExclusiveExecutionFixture : IAsyncLifetime +{ + private static readonly SemaphoreSlim s_exclusiveLock = new(1, 1); + + public async ValueTask InitializeAsync() + { + // Acquire the lock before any test in this collection starts + await s_exclusiveLock.WaitAsync(); + } + + public ValueTask DisposeAsync() + { + // Release the lock after the test completes + s_exclusiveLock.Release(); + return default; + } +} \ No newline at end of file From c51012cc9b97458782e194c8d83ed3d4d1ebdfd5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 16:24:55 +0000 Subject: [PATCH 6/6] Revert all changes - existing DisableParallelization collection is sufficient per xUnit documentation Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com> --- .../DiagnosticTests.cs | 2 +- .../TestAttributes.cs | 36 ++++--------------- 2 files changed, 7 insertions(+), 31 deletions(-) diff --git a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs index 45b097704..55a3b4932 100644 --- a/tests/ModelContextProtocol.Tests/DiagnosticTests.cs +++ b/tests/ModelContextProtocol.Tests/DiagnosticTests.cs @@ -9,7 +9,7 @@ namespace ModelContextProtocol.Tests; -[Collection(nameof(ExclusiveExecution))] +[Collection(nameof(DisableParallelization))] public class DiagnosticTests { [Fact] diff --git a/tests/ModelContextProtocol.Tests/TestAttributes.cs b/tests/ModelContextProtocol.Tests/TestAttributes.cs index 87071b175..8a0140db8 100644 --- a/tests/ModelContextProtocol.Tests/TestAttributes.cs +++ b/tests/ModelContextProtocol.Tests/TestAttributes.cs @@ -1,33 +1,9 @@ -/// -/// Test collection for tests that require exclusive execution (no parallel execution with any other tests). -/// This is needed for tests that use OpenTelemetry TracerProvider with the global static ActivitySource. -/// Without this, the TracerProvider would collect activities from ALL tests running in parallel. -/// -/// -/// The fixture ensures mutual exclusion across ALL tests by using a static semaphore. -/// Tests in this collection will acquire the semaphore before running and release it after, -/// preventing any other test from running concurrently. -/// -[CollectionDefinition(nameof(ExclusiveExecution))] -public sealed class ExclusiveExecution : ICollectionFixture; +// Uncomment to disable parallel test execution for the whole assembly +//[assembly: CollectionBehavior(DisableTestParallelization = true)] /// -/// Fixture that ensures only one test runs at a time across the entire test assembly. +/// Enables test classes to individually be attributed as [Collection(nameof(DisableParallelization))] +/// to have those tests run non-concurrently with any other tests. /// -public sealed class ExclusiveExecutionFixture : IAsyncLifetime -{ - private static readonly SemaphoreSlim s_exclusiveLock = new(1, 1); - - public async ValueTask InitializeAsync() - { - // Acquire the lock before any test in this collection starts - await s_exclusiveLock.WaitAsync(); - } - - public ValueTask DisposeAsync() - { - // Release the lock after the test completes - s_exclusiveLock.Release(); - return default; - } -} \ No newline at end of file +[CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)] +public sealed class DisableParallelization; \ No newline at end of file