Skip to content

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Nov 16, 2025

Motivation and Context

xunit 2 doesn't seem to be receiving any updates. This PR moves to the newer xunit.v3, along with Microsoft.Testing.Platform.

Description

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the Contribution Guidelines
  • All unit tests pass, and I have added new tests where possible
  • Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

Copilot AI review requested due to automatic review settings November 16, 2025 21:03
@github-actions github-actions bot changed the title Migrate to xunit.v3 .NET: Migrate to xunit.v3 Nov 16, 2025
Copilot finished reviewing on behalf of Youssef1313 November 16, 2025 21:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the test infrastructure from xUnit v2 to xUnit v3, updating test fixtures, package references, and build configuration to use the Microsoft Testing Platform runner.

Key changes:

  • Migrated from xUnit v2 to xUnit v3 with Microsoft Testing Platform integration
  • Updated all test fixture interfaces from Task to ValueTask for IAsyncLifetime methods
  • Removed unused Xunit.Abstractions namespace imports across test files
  • Updated .NET SDK version to 10.0.100-rc.2 and configured test projects with OutputType=Exe

Reviewed Changes

Copilot reviewed 103 out of 103 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
dotnet/Directory.Packages.props Updated xUnit packages from v2 to v3, replaced test SDK packages with Microsoft Testing Platform equivalents
dotnet/tests/Directory.Build.props Changed test infrastructure to use Microsoft Testing Platform runner, updated package references
dotnet/global.json Updated SDK version to 10.0.100-rc.2 and configured Microsoft Testing Platform as test runner
dotnet/.editorconfig Added suppression for xUnit1051 diagnostic
.github/workflows/dotnet-build-and-test.yml Updated test commands to use new Microsoft Testing Platform syntax (--project, --report-trx, --coverage)
dotnet/tests//.csproj (multiple files) Added <OutputType>Exe</OutputType> to all test projects for standalone execution
dotnet/tests/*/Fixture.cs (multiple files) Updated IAsyncLifetime implementations from Task to ValueTask return types
dotnet/tests/**/*.cs (multiple test files) Removed unused using Xunit.Abstractions; statements
dotnet/tests/AgentConformance.IntegrationTests/*.csproj Changed property from IsTestProject=false to IsTestUtilityProject=true

Copy link
Member

@rogerbarreto rogerbarreto Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is global moved to the repo root? This is a dotnet specific setting

Suggest not changing it to .Net 10 as this is not the purpose of the PR.

Copy link
Member Author

@Youssef1313 Youssef1313 Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The move to repo root is to allow running dotnet test --project dotnet/path/to/csproj as done in CI. Otherwise, the .NET CLI won't be able to find the global.json and then you have to cd dotnet first before invoking dotnet test. (also if you moved from GH Actions to Azure Pipelines and used DotnetCoreCLI task, it looks for global.json at root as well)

The switch to .NET 10 is kinda related, as that's what works best with xunit.v3 and Microsoft.Testing.Platform v2.

I'm fine getting a PR for updating to .NET 10 first separately though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@DeagleGross DeagleGross Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you have changed the CI as well to point to a different location:

global-json-file: ${{ github.workspace }}/global.json

instead of

global-json-file: ${{ github.workspace }}/dotnet/global.json

I dont think there is a problem with having global.json under dotnet/ directory, can we rollback this change? It seems strange to have a dotnet related artifact in the repo root

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The global-json-file in YML is aligned with the change.

Having it under dotnet/ is really problematic as it means you cannot invoke dotnet test from repo root.

@rogerbarreto
Copy link
Member

@Youssef1313 Thank you very much for the contribution and changes. Had some small comments and suggestions! Overall LGTM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move global.json outside of the dotnet folder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered in #2259 (comment)

Copy link
Contributor

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

<PropertyGroup>
<TargetFrameworks>$(ProjectsCoreTargetFrameworks)</TargetFrameworks>
<TargetFrameworks Condition="'$(Configuration)' == 'Debug'">$(ProjectsDebugCoreTargetFrameworks)</TargetFrameworks>
<OutputType>Exe</OutputType>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider defining test specific build properties here (instead of fanning out into each child project):

https://github.com/microsoft/agent-framework/blob/main/dotnet/tests/Directory.Build.props

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants