Add support for running XUnit and MSTest tests with the VSTest runner

[alexperovich]

No description provided.

[davidfowl]

Sweet! This repo dogfoods itself right? How do we try this out or verify tests?

[alexperovich]

This repo doesn't directly dogfood itself, arcade-validation gets this once it is merged. I tested it locally by copying the targets into my nuget cache on top of the arcade.sdk that arcade was using and ran tests.

[davidfowl]

This repo doesn't directly dogfood itself, arcade-validation gets this once it is merged. I tested it locally by copying the targets into my nuget cache on top of the arcade.sdk that arcade was using and ran tests.

Cool, seems risky 😄. In the future it might be good to have a way to unit test these changes. Also, does it support passing args as well? We need to b able to support passing vstest args to turn on loggers etc.

[sharwell]

Can we get NUnit as well? The required package references can be seen in NUnit.targets here:

dotnet/roslyn-sdk#507

[markwilkie]

Here's how validation works: https://github.com/dotnet/arcade/blob/master/Documentation/Validation/Overview.md

Also, +100 to units tests where possible

[alexperovich]

Also, does it support passing args as well?

The TestRunnerAdditionalArguments property can be set to add more parameters to VSTest.

[davidfowl]

cc @jakubch1

[ChadNedzlek]

What does this value do? It doesn't seem to be used anywhere, and the "MSTest.targets" just calls "dotnet vstest"... all it does it add a handful of package references... but doesn't use them.

[alexperovich]

this actually shouldn't be here. I intended to remove the "UsingTool" properties for test frameworks. Projects should specify TestRunnerName instead.

[ChadNedzlek]

This seems like a fairly complicated matrix of options we are exposing... and I wonder if we have the capacity to manage all this, or if some of the paths are going to atrophy and not really be useful. But if we can't get everyone using the same runner, we don't have much choice other than to pay this maintenance burden until we can.

I expect we're going to break a lot of repositories with arcade updates with a test matrix this large.

[ChadNedzlek]

What do we expect to use this runner type? Is there some benefit to using this rather than vstest?

[alexperovich]

With the current implementation xunit tests default to using this runner. This is the runner it was using before I added vstest as an option.

The xunit console runner works correctly for a few cases where vstest doesn't do the right thing. @tmat can elaborate, but I believe it was tests running on desktop don't get the correct runtime framework version so the compat switches are not set how the test expects.

[ChadNedzlek]

Why are we defaulting to the thing we don't want people using? Is the plan to change the default later and then remove this implementation, so that vstest is the only option?

Is the desktop scenario something that's important for arcade in master to be supporting?

[alexperovich]

I think the desktop scenario is still important because VS is desktop and some of the repos run inside VS.

[ChadNedzlek]

Does that imply some repos are staying on netstandard2.0 forever? That's certainly an expensive proposition from a support perspective.

[davidfowl]

What else is needed here? Can we merge this?

[ViktorHofer]

Are you sure this is the right path forward for enabling the VSTest runner? I added support for VSTest in dotnet/runtime as well but without relying on the /t:Test target and instead "just" enabled the dotnet test scenario.

Asking as our community gave us a strong signal that we should enable the default dotnet CLI verbs instead of adding custom targets, e.g.:

Arcade is the ideal place to enable scenarios like "VSTest", "VS Test Explorer", "Code coverage measurement with coverlet" hence I'm a big fan of this work. Unfortunately with the current state this in, we can't leverage the added features in dotnet/runtime or aspnetcore as we want to support dotnet test.

[alexperovich]

In what way does dotnet test not work? I just tried it and it works fine, both for a single project, and for the solution. It doesn't produce test results in the same place, but it does run all the tests.

[alexperovich]

The VSTest explorer also works fine.

[ViktorHofer]

this should be just dotnet test as both dotnet test and dotnet vstest got merged together to dotnet test.

[ViktorHofer]

these settings should be configured in a .runsettings file which allows to specify additional settings like a datacollector for coverlet code coverage.

[alexperovich]

I could make it dotnet test here, but that will do a bunch of work msbuilding the project again when that has already happened. We are required to use dotnet vstest for executing tests in helix because the project files are gone. We do that here (https://github.com/dotnet/helix-diagnostic-prototype/blob/master/src/overrides/Helix.Sdk/CreateXUnitWorkItems.cs#L104). I am using the same logic here for consistency.

[ViktorHofer]

You probably misunderstood me. dotnet test supports invoking an assembly now in addition to a project or solution file.

[ViktorHofer]

dotnet/toolset@1214ec9#diff-3e71c85821fdc972adec6f9e60e2541e

[alexperovich]

that should be updated in the docs then, the dotnet test command docs don't say anything about that.

[ViktorHofer]

cc @nohwnd who is part of the VSTest team.

[ViktorHofer]

Filed microsoft/vstest#2423

[ViktorHofer]

Why are you changing the default verbosity which is minimal to normal?

[alexperovich]

Because we want logging of what tests are running when so we can diagnose crashes more easily.

[ViktorHofer]

The test execution order usually isn't in order. If you want to improve diagnosability you want to enable the blame collector: https://github.com/microsoft/vstest-docs/blob/master/docs/extensions/blame-datacollector.md

[ViktorHofer]

In what way does dotnet test not work? I just tried it and it works fine, both for a single project, and for the solution. It doesn't produce test results in the same place, but it does run all the tests.

I was talking about the indirection that you are adding here. You are expecting devs to invoke the /t:Test target which then invokes dotnet test which then invokes /t:VSTest. I don't understand why we keep the separate Test target for VSTest.

[ViktorHofer]

cc @nohwnd

[ViktorHofer]

VSTest already does all the test logging.

[alexperovich]

VSTest output doesn't get placed in the build log.

[ViktorHofer]

Right, that's because of the indirection that I mentioned. I don't we should continue doing this with VSTest and just directly invoke it.

[ViktorHofer]

VSTest already logs the generated log files like trx and html to stdout. Why are we duplicating that logic?

[alexperovich]

Same reason as above, the output from VSTest is written into a txt file, this shows up in the build log as an error and causes the build to fail when tests fail.

[ViktorHofer]

For F5 and /t:Run support, you want to add these properties as well: https://github.com/dotnet/runtime/blob/dffeff8b6672806375ad82a52f1ed44a4a7564be/eng/testing/xunit/xunit.targets#L2-L15.

[tmat]

UsingToolXUnit [](start = 5, length = 14)

It seems we are still using this property. We also use UsingToolVSTest now. Both should be listed here with their defaults (either opt-in or opt-out).

[alexperovich]

I am only reading UsingToolXUnit as a back-compat for cases where it was set to false for some reason. I will add UsingToolVSTest here.

[tmat]

It still should be here, perhaps with a comment. It is useful to see all the values here for reference.

[alexperovich]

UsingToolVSTest is a bit more strange too. Its default changes based on what test framework you are using. Its false by default if you are using XUnit, but it is required to be true if using another test framework.

[tmat]

So then I'm not sure naming it UsingToolXxx is appropriate. These switches are used for repository-wide configuration, not something that should differ project to project.

[alexperovich]

I will name it UseVSTestRunner then.

[alexperovich]

I was talking about the indirection that you are adding here. You are expecting devs to invoke the /t:Test target which then invokes dotnet test which then invokes /t:VSTest. I don't understand why we keep the separate Test target for VSTest.

There is no expectation that devs use /t:Test. The CI builds do, but a dev running tests locally should just use either VS or dotnet test.

[ViktorHofer]

There is no expectation that devs use /t:Test. The CI builds do, but a dev running tests locally should just use either VS or dotnet test.

I still don't understand why we want to keep the /t:Test target? dotnet test works on both assemblies and project and solution files. It either builds the project(s) or just invokes the assembly/assemblies. For CI dotnet test x.dll --settings .runsettings should be enough for what you are trying to accomplish here.

[tmat]

Getting rid of /t:Test would require some more significant changes. Some repos use the TestArchitectures to run AnyCPU tests on multiple platforms (32 and 64-bit).
I don't think that can be easily done with dotnet test. We'd need to move away from using TestArchitectures in these repos.

Also, we would also need to change build.ps1 to invoke dotnet test instead of passing /t:Test to the build.proj.

One more thing - there is still one more issue to address before repos can move to vstest: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1002535

[ViktorHofer]

Please don't get me wrong, I'm asking for not leveraging the test target for VSTest but keeping it for xunit.console until we can remove the xunit.console dependency.

Regarding the linked issue, I'm sure we can get that fixed near term by collaborating with the vstest folks. They have been very open and helpful in the past.

Right, looks like TestArchitectures is very custom tailored. Do our repositories absolutely need that feature? We only run on the current arch in dotnet/runtime for AnyCPU specific libraries and CI then fans out.

[alexperovich]

For F5 and /t:Run support, you want to add these properties as well: https://github.com/dotnet/runtime/blob/dffeff8b6672806375ad82a52f1ed44a4a7564be/eng/testing/xunit/xunit.targets#L2-L15.

How would you expect this to work? These targets run tests for each TFM separately, I can't set these properties for the whole project. There isn't a single TFM to run tests for, there can be many.

[tmat]

Right, looks like TestArchitectures is very custom tailored. Do our repositories absolutely need that feature? We only run on the current arch in dotnet/runtime for AnyCPU specific libraries and CI then fans out.

It can indeed be implemented differently, but it's work that needs to be done. Let's file an issue to deprecate this feature.

[tmat]

For F5 and /t:Run support, you want to add these properties as well: https://github.com/dotnet/runtime/blob/dffeff8b6672806375ad82a52f1ed44a4a7564be/eng/testing/xunit/xunit.targets#L2-L15.

I'm not sure we should add F5 support like that. That should be a VS feature. Typically, one runs tests in VS using Test Explorer or right-click on project "run tests".

BTW, what's /t:Run?

[ViktorHofer]

How would you expect this to work? These targets run tests for each TFM separately, I can't set these properties for the whole project. There isn't a single TFM to run tests for, there can be many.

Inside VS you select the TargetFramework from the TargetFrameworkPicker which lists TargetFramework entries for all TargetFrameworks 🤔😅

These properties are part of the inner build hence they are tfm specific.

[alexperovich]

Inside VS you select the TargetFramework from the TargetFrameworkPicker which lists TargetFramework entries for all TargetFrameworks 🤔😅

These properties are part of the inner build hence they are tfm specific.

Does F5 in vs pick a TFM somehow? I've never had a runnable project with more than one TFM.

[ViktorHofer]

I'm not sure we should add F5 support like that. That should be a VS feature. Typically, one runs tests in VS using Test Explorer or right-click on project "run tests".

BTW, what's /t:Run?

Can you please elaborate on your concerns? We always set those in corefx/runtime to allow F5 debugging for people who prefer that.

The Run target is an sdk target which was added recently and uses the RunCommand, RunArguments and RunWorkingDirectory properties. Quite similar to what VS does.

[tmat]

Can you please elaborate on your concerns?

Feel free to use it in dotnet/runtime if devs find it useful. I'm just saying we should push on VS test team to improve the built-in experience of running tests, so that setting these build props is not necessary.

[davidfowl]

Can somebody summarize what's blocking this PR. The goal is to get this vstest support merged this week.

[alexperovich]

I don't think anything is blocking this functionally. This change allows using vstest instead of the xunit console runner so we can use the advanced diagnostic features in vstest. I think that the moving off of using our custom /t:Test logic is a much bigger work item and should not block this PR.

[ChadNedzlek]

In that case, I'm going to merge it unless I hear any strong objections. We can look into refactoring the logic later, as it doesn't seem directly tied to this PR. My understanding is that we already do things this way, and this is just broadening our current infrastructure a bit.

[tmat]

[davidfowl]

Also docs on how to enable and customize this would be great

[safern]

The RunTests target in Xunit.Runner.Targets no longer has any Inputs-- I know this is probably wrong indotnet/runtimeas we're calling the test target even if we don't want to run tests, but this seems to be a supported feature by arcade. You can call-testprobably to build the test projects, but you can skip the execution by settingSkipTests=true` property.

arcade/src/Microsoft.DotNet.Arcade.Sdk/tools/Tests.targets

Lines 40 to 44 in 5f124ea

	<Target Name="_InnerGetTestsToRun"
	Outputs="%(_TestArchitectureItems.Identity)"
	Returns="@(TestToRun)"
	Condition="'$(TestRuntime)' != '' and '$(SkipTests)' != 'true' and
	('$(TestTargetFrameworks)' == '' or $([System.String]::new(';$(TestTargetFrameworks);').Contains(';$(TargetFramework);')))">

This target should either take @(TestToRun) as inputs or condition the target to only run when SkipTests != true

We're hitting this when trying to update to arcade in dotnet/runtime.

Running tests:  []
  /bin/sh: 2: /tmp/tmpb3f998934c814bb691a6823577778e81.exec.cmd: cannot create : Directory nonexistent
/root/runtime/.packages/microsoft.dotnet.arcade.sdk/5.0.0-beta.20255.6/tools/XUnit/XUnit.Runner.targets(97,5): error MSB4044: The "WriteLinesToFile" task was not given a value for the required parameter "File". [/root/runtime/src/installer/test/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/AppHost.Bundle.Tests.csproj]

[ViktorHofer]

Feel free to use it in dotnet/runtime if devs find it useful. I'm just saying we should push on VS test team to improve the built-in experience of running tests, so that setting these build props is not necessary.

@tmat just opened microsoft/vstest#2425 to track this.