Support XUnitV3 as test runner #15671
Conversation
1a4aa04 to
b4b8eae
Compare
| <!-- Microsoft.NET.Test.Sdk is VSTest-specific package. --> | ||
| <!-- If EnableMSTestRunner, EnableNUnitRunner, or UseMicrosoftTestingPlatformRunner is true, then the repo is using Microsoft.Testing.Platform --> |
There was a problem hiding this comment.
This is technically breaking.
If a repo is setting EnableMSTestRunner, EnableNUnitRunner, or UseMicrosoftTestingPlatformRunner but is not either opting-in via dotnet.config or TestingPlatformCommandLineArguments, then their setup will actually be using VSTest when invoking dotnet test directly.
This also means that such repos are not properly configured to use MTP, where the intent may be to use it. So I think it may be reasonable to break, but I'll let you decide.
| <PropertyGroup Condition="'$(_TestRuntime)' == 'Core'"> | ||
| <_TestRunnerArgs Condition="'$(UseMicrosoftTestingPlatformRunner)' != 'true'">$(_TestRunnerArgs) -noAutoReporters</_TestRunnerArgs> | ||
| <_TestRunnerArgs Condition="'$(UseMicrosoftTestingPlatformRunner)' == 'true'">$(_TestRunnerArgs) --auto-reporters off</_TestRunnerArgs> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
I'm using the same '$(_TestRuntime)' == 'Core' condition as in xunit v2. But I'm not following why this only applies for .NET Core
84d7db7 to
22cae6c
Compare
| WorkingDirectory="$(_TargetDir)" | ||
| IgnoreExitCode="true" | ||
| Timeout="$(_TestTimeout)" | ||
| EnvironmentVariables="DOTNET_ROOT=$(DotNetRoot);DOTNET_ROOT_X86=$(DotNetRoot)x86" |
There was a problem hiding this comment.
@ViktorHofer Can you please confirm if this is a fine thing to do?
There was a problem hiding this comment.
I'm not sure about this one. @agocke @vitek-karas can you please chime in here?
There was a problem hiding this comment.
With the .runsettings file, we specify the DotNetHostPath which is used to execute vstest's testhost.dll today: https://github.com/dotnet/runtime/blob/b1d93d078659f71068715b5fc5a1843d64395490/eng/testing/.runsettings#L20
How does that work now? IOW how is the DOTNET_ROOT variable used here?
There was a problem hiding this comment.
@ViktorHofer xunit.v3 (both with VSTest and MTP), as well as all frameworks with MTP, will launch test executable directly. The reason I needed to set DOTNET_ROOT here is for the test executable to be able to find the correct runtimes from potentially locally installed SDK. That's how it currently works. As for MTP itself, we don't alter anything in ways that VSTest did in the past.
We may decide to alter DOTNET_ROOT for dotnet test command on the SDK side of things, which is already an open question for us, but dotnet test is irrelevant to what Arcade does here.
There was a problem hiding this comment.
I see, that makes sense to me. Does this mean that with this line, you are setting DOTNET_ROOT to the acquired SDK (which if it isn't available is located under repo/.dotnet/?
We should add an indirection property so that repos that build their own DOTNET_ROOT (runtime, sdk, ...) can set this to something else. I.e. <TestDotNetRoot Condition="'$(TestDotNetRoot)' == ''">$(DotNetRoot)</TestDotNetRoot>.
There was a problem hiding this comment.
It's usually RepoRoot/.dotnet yes. I added the indirection property TestDotNetRoot.
|
@ViktorHofer I addressed/answered your comments. Let me know if you have any further questions/concerns. |
|
I will take a closer look tomorrow. |
|
Converting to draft. I need to do more manual validation here |
src/Microsoft.DotNet.Arcade.Sdk/tools/XUnitV3/XUnitV3.Runner.targets
Outdated
Show resolved
Hide resolved
|
@ViktorHofer This is ready for review |
src/Microsoft.DotNet.Arcade.Sdk/tools/XUnitV3/XUnitV3.Runner.targets
Outdated
Show resolved
Hide resolved
|
As I noticed that you already merged the aspire PR, a follow-up question: Was this an orthogonal effort or will this PR make some of the files in aspire obsolete? |
|
I think the aspire will be cleaned up once this goes in. |
There was a problem hiding this comment.
Looks good so far. We should figure the DOTNET_ROOT one out though. Especially, for repositories like runtime that want to set this to the just live built DOTNET_ROOT (aka testhost folder). It might make sense to allow to overwrite that by using an additional property as indirection. We can't overwrite DotNetRoot directly.
|
@ViktorHofer I added |
|
@ViktorHofer I'm assuming this is hopefully good to merge, and it's low risk in case of bugs anyways. We can then start looking into repos that are easy to switch and see how things will go. |
This PR was manually validated locally, both with
UseMicrosoftTestingPlatformRunnerset to true and false.Related to #15654