-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Update to xunit.v3 #20331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Update to xunit.v3 #20331
Conversation
d68fac2 to
00c2c31
Compare
00c2c31 to
1eee916
Compare
...s/Avalonia.Headless.XUnit.PerTest.UnitTests/Avalonia.Headless.XUnit.PerTest.UnitTests.csproj
Show resolved
Hide resolved
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
@maxkatz6 Can you please review this? Thanks! |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
| <PackageReference Include="xunit.runner.visualstudio" Version="3.1.5" Condition="'$(TargetFramework)' != 'netstandard2.0'" /> | ||
| <PackageReference Include="Xunit.SkippableFact" Version="1.5.23" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="18.0.1" /> | ||
| <PackageReference Include="YTest.MTP.XUnit2" Version="1.0.2" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to reference a third party package just for enabling xUnit 2 with MTP here. Especially since we're going to update our headless packages to xUnit 3 anyway.
cc @maxkatz6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrJul What's the concern of referencing a "third party" package? Xunit.SkippableFact is also a third party package.
This is going to be temporary anyways until the next Avalonia major if next major will drop XUnit 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just adding an extra layer that we have to maintain: while nice, we don't really need xUnit 2 support on MTP. If something goes wrong for any reason, there will be absolutely no support here. (I'm aware that you're the package author.)
Since it's temporary anyway, this is indeed less of a concern, but I still fail to see the immediate benefit instead of running our old xUnit 2 tests "the old way" :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It creates a lot of hassle to mix VSTest and Microsoft.Testing.Platform in the same solution and is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming YTest.MTP.XUnit2 is not a transitive dependency of any of Avalonia packages, it should be fine as an intermediate step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxkatz6 Yes. It's not going to be a dependency of any Avalonia packages. In that case, this PR is ready to merge on its current state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not with the added warnings, but otherwise yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. I'll fix the warnings.
|
Note: a lot of new warnings coming from xUnit analyzers are generated by this upgrade. |
I think you should really work on getting clean builds and enable warnings as errors in CI. |
I agree, and that's exactly what I am working on. #20312 solves the analyzer warnings. |
|
Sure, I'll work on the newly introduced warnings. But we need to first align on how to handle XUnit 2. Related question: is master now building 12.x packages? So I can actually update everything to xunit.v3 in this PR? |
Yes, master has been building 12.x packages for a while now.
While we should be able to update everything in this PR, I know that the underlying xUnit runner API has changed a bit and our headless infrastructure for this framework might need a rework. If that's the case, that kind of work would probably be better done in another PR, to keep things contained. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
What does the pull request do?
Updates tests to run with xunit.v3 (when possible) and Microsoft.Testing.Platform v2. NUnit headless tests are still xunit 2.
What is the current behavior?
Tests are run with xunit 2 and VSTest.
What is the updated/expected behavior with this PR?
Test-infra changes only. This is a modernization of the test infra to use the latest and greatest tools available.
How was the solution implemented (if it's not obvious)?
Use xunit.v3.mtp-v2 NuGet package and make necessary infra changes to support it.
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues