.NET: Upgrade to XUnit 3 and Microsoft Testing Platform#4176
.NET: Upgrade to XUnit 3 and Microsoft Testing Platform#4176westey-m merged 13 commits intomicrosoft:feature-xunit3-mtp-upgradefrom
Conversation
6c3b9d4
into
microsoft:feature-xunit3-mtp-upgrade
There was a problem hiding this comment.
Pull request overview
This PR upgrades the .NET test infrastructure from XUnit 2 to XUnit 3 and migrates to the Microsoft Testing Platform (MTP) to enable better test parallelization and improve integration test performance.
Changes:
- Upgraded XUnit from v2 (2.9.3) to v3 (3.2.2) with MTP support
- Switched code coverage from coverlet.collector to Microsoft.Testing.Extensions.CodeCoverage
- Updated IAsyncLifetime implementations to use ValueTask instead of Task
- Migrated test skip patterns from [Fact(Skip)] and [SkippableFact] to Assert.Skip/Assert.SkipWhen
- Updated CI/CD workflows to use new MTP command-line syntax
- Removed Xunit.Abstractions imports that are no longer needed
Reviewed changes
Copilot reviewed 113 out of 113 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/Directory.Packages.props | Updated package versions for XUnit v3, xRetry, and code coverage |
| dotnet/tests/Directory.Build.props | Added MTP configuration properties and updated package references |
| dotnet/global.json | Added MTP test runner configuration |
| dotnet/tests/coverage.runsettings | New code coverage settings file for MTP |
| dotnet/tests/*/Fixture.cs (multiple) | Changed IAsyncLifetime methods from Task to ValueTask and added GC.SuppressFinalize |
| dotnet/tests/*Tests.cs (multiple) | Replaced [Fact(Skip)] and [SkippableFact] with Assert.Skip/Assert.SkipWhen patterns |
| dotnet/tests//.cs (multiple) | Removed unused Xunit.Abstractions imports |
| .github/workflows/dotnet-build-and-test.yml | Updated dotnet test commands to use MTP syntax with --project, --report-xunit-trx, --ignore-exit-code 8, and --filter-not-trait |
| dotnet/.github/skills/build-and-test/SKILL.md | Updated documentation with MTP usage examples and command syntax |
| dotnet/tests/Microsoft.Agents.AI.CosmosNoSql.UnitTests/*.csproj | Removed Xunit.SkippableFact package reference |
| dotnet/tests/IntegrationTests/.csproj (multiple) | Added NoWarn for CS8793 (return type mismatch warning) |
| public override Task RunWithResponseFormatReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(SkipReason is not null, SkipReason!); |
There was a problem hiding this comment.
The condition SkipReason is not null will always be true because SkipReason is a const string. This means the test will always be skipped. If the intent is to conditionally skip the test, consider using a nullable string with a null default value, or remove the null check entirely if the test should always be skipped.
| public override Task RunWithGenericTypeReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(SkipReason is not null, SkipReason!); |
There was a problem hiding this comment.
The condition SkipReason is not null will always be true because SkipReason is a const string. This means the test will always be skipped. If the intent is to conditionally skip the test, consider using a nullable string with a null default value, or remove the null check entirely if the test should always be skipped.
| public override Task RunWithPrimitiveTypeReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(SkipReason is not null, SkipReason!); |
There was a problem hiding this comment.
The condition SkipReason is not null will always be true because SkipReason is a const string. This means the test will always be skipped. If the intent is to conditionally skip the test, consider using a nullable string with a null default value, or remove the null check entirely if the test should always be skipped.
| public override Task RunWithChatMessageReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithChatMessageReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithChatMessagesReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithChatMessagesReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithNoMessageDoesNotFailAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithNoMessageDoesNotFailAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithStringReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithStringReturnsExpectedResultAsync(); | ||
| } |
There was a problem hiding this comment.
The condition ManualVerification is not null will always be true because ManualVerification is a const string. This means these tests will always be skipped. According to the comment "Set to null to run the tests", it appears the intent is to conditionally skip. Consider changing ManualVerification to private const string? ManualVerification = "For manual verification"; to make it nullable, allowing developers to set it to null when needed.
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithChatMessageReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithNoMessageDoesNotFailAsync() | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithNoMessageDoesNotFailAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithChatMessagesReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithChatMessagesReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithStringReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithStringReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task SessionMaintainsHistoryAsync() | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); |
There was a problem hiding this comment.
The condition AnthropicChatCompletionFixture.SkipReason is not null will always be true because SkipReason is a const string. This means these tests will always be skipped. If the intent is to conditionally skip these tests, consider making SkipReason nullable or using a different pattern such as conditional compilation directives.
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithChatMessageReturnsExpectedResultAsync(); | |
| } | |
| public override Task RunWithNoMessageDoesNotFailAsync() | |
| { | |
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithNoMessageDoesNotFailAsync(); | |
| } | |
| public override Task RunWithChatMessagesReturnsExpectedResultAsync() | |
| { | |
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithChatMessagesReturnsExpectedResultAsync(); | |
| } | |
| public override Task RunWithStringReturnsExpectedResultAsync() | |
| { | |
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithStringReturnsExpectedResultAsync(); | |
| } | |
| public override Task SessionMaintainsHistoryAsync() | |
| { | |
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | |
| Assert.Skip(AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithChatMessageReturnsExpectedResultAsync(); | |
| } | |
| public override Task RunWithNoMessageDoesNotFailAsync() | |
| { | |
| Assert.Skip(AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithNoMessageDoesNotFailAsync(); | |
| } | |
| public override Task RunWithChatMessagesReturnsExpectedResultAsync() | |
| { | |
| Assert.Skip(AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithChatMessagesReturnsExpectedResultAsync(); | |
| } | |
| public override Task RunWithStringReturnsExpectedResultAsync() | |
| { | |
| Assert.Skip(AnthropicChatCompletionFixture.SkipReason!); | |
| return base.RunWithStringReturnsExpectedResultAsync(); | |
| } | |
| public override Task SessionMaintainsHistoryAsync() | |
| { | |
| Assert.Skip(AnthropicChatCompletionFixture.SkipReason!); |
| public override Task RunWithFunctionsInvokesFunctionsAndReturnsExpectedResultsAsync() | ||
| => base.RunWithFunctionsInvokesFunctionsAndReturnsExpectedResultsAsync(); | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithFunctionsInvokesFunctionsAndReturnsExpectedResultsAsync(); | ||
| } | ||
|
|
||
| [Fact(Skip = AnthropicChatCompletionFixture.SkipReason)] | ||
| public override Task RunWithInstructionsAndNoMessageReturnsExpectedResultAsync() | ||
| => base.RunWithInstructionsAndNoMessageReturnsExpectedResultAsync(); | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithInstructionsAndNoMessageReturnsExpectedResultAsync(); | ||
| } |
There was a problem hiding this comment.
The condition AnthropicChatCompletionFixture.SkipReason is not null will always be true because SkipReason is a const string. This means these tests will always be skipped. If the intent is to conditionally skip these tests, consider making SkipReason nullable or using a different pattern such as conditional compilation directives.
| } | ||
|
|
||
| async Task IAsyncLifetime.DisposeAsync() | ||
| async ValueTask IAsyncDisposable.DisposeAsync() |
There was a problem hiding this comment.
In XUnit v3, IAsyncLifetime.DisposeAsync should return ValueTask, not be implemented as IAsyncDisposable.DisposeAsync. The interface should be explicitly implemented as ValueTask IAsyncLifetime.DisposeAsync() to match the IAsyncLifetime interface definition.
| } | ||
|
|
||
| async Task IAsyncLifetime.DisposeAsync() | ||
| async ValueTask IAsyncDisposable.DisposeAsync() |
There was a problem hiding this comment.
In XUnit v3, IAsyncLifetime.DisposeAsync should return ValueTask, not be implemented as IAsyncDisposable.DisposeAsync. The interface should be explicitly implemented as ValueTask IAsyncLifetime.DisposeAsync() to match the IAsyncLifetime interface definition.
| public override Task RunWithChatMessageReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithChatMessageReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithChatMessagesReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithChatMessagesReturnsExpectedResultAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithNoMessageDoesNotFailAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithNoMessageDoesNotFailAsync(); | ||
| } | ||
|
|
||
| public override Task RunWithStringReturnsExpectedResultAsync() | ||
| { | ||
| Assert.SkipWhen(ManualVerification is not null, ManualVerification!); | ||
| return base.RunWithStringReturnsExpectedResultAsync(); | ||
| } |
There was a problem hiding this comment.
The condition ManualVerification is not null will always be true because ManualVerification is a const string. This means these tests will always be skipped. According to the comment "Set to null to run the tests", it appears the intent is to conditionally skip. Consider changing ManualVerification to private const string? ManualVerification = "For manual verification"; to make it nullable, allowing developers to set it to null when needed.
| public override Task RunWithFunctionsInvokesFunctionsAndReturnsExpectedResultsAsync() | ||
| => base.RunWithFunctionsInvokesFunctionsAndReturnsExpectedResultsAsync(); | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithFunctionsInvokesFunctionsAndReturnsExpectedResultsAsync(); | ||
| } | ||
|
|
||
| [Fact(Skip = AnthropicChatCompletionFixture.SkipReason)] | ||
| public override Task RunWithInstructionsAndNoMessageReturnsExpectedResultAsync() | ||
| => base.RunWithInstructionsAndNoMessageReturnsExpectedResultAsync(); | ||
| { | ||
| Assert.SkipWhen(AnthropicChatCompletionFixture.SkipReason is not null, AnthropicChatCompletionFixture.SkipReason!); | ||
| return base.RunWithInstructionsAndNoMessageReturnsExpectedResultAsync(); | ||
| } |
There was a problem hiding this comment.
The condition AnthropicChatCompletionFixture.SkipReason is not null will always be true because SkipReason is a const string. This means these tests will always be skipped. If the intent is to conditionally skip these tests, consider making SkipReason nullable or using a different pattern such as conditional compilation directives.
Motivation and Context
Our integration tests are running too slowly, but with XUnit 3 and MTP we have more options for running tests in parallel.
Description
Contribution Checklist