Skip to content
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

Enable dotnet test in dotnet/runtime #945

Closed
ViktorHofer opened this issue Jan 3, 2019 · 21 comments · Fixed by #35285
Closed

Enable dotnet test in dotnet/runtime #945

ViktorHofer opened this issue Jan 3, 2019 · 21 comments · Fixed by #35285
Labels
area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner
Milestone

Comments

@ViktorHofer
Copy link
Member

Investigate how much work it is to switch away from the deprecated xunit.console runner to dotnet vstest. This is just about the runner, the test infra backend will still be xunit.

UAP will continue using Microsoft.DotNet.XUnitRunnerUap.
UAPAOT is problematic. We currently use xunit.console for it which doesn't work (https://github.com/dotnet/corefx/issues/30423). It may be our best option to add a ifdef configuration in dotnet/arcade#1627 which works fine with uapaot.

@ViktorHofer ViktorHofer changed the title Switch to dotnet vstest as our test runner Switch from xunit.console to dotnet vstest Jan 3, 2019
@clairernovotny
Copy link
Member

xunit for devices supports UWP with .NET Native FYI. The Xamarin Essentials folks have done work to help automate and track that and I welcome improvements there too.

@ViktorHofer
Copy link
Member Author

@onovotny From the readme it seems that you are injecting the MainPage.xaml into the test unit project. Would it be possible to change the test unit project to a UWP console app instead of injecting the MainPage.xaml so that we don't invoke a GUI?

Currently for UWP we use https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.XUnitRunnerUap which consists of a runner and and a launcher. The runner is a UWP console app and xunit runner which just runs the tests that are discover able and the launcher bundles the runner and the UWP test project together as we can't do Assembly.Load in UWP.

The injecting strategy is interesting and something I haven't thought of before. If xunit for devices could inject everything so that a UWP console app would be the result that could eliminate our custom runner for UWP which would be awesome.

@clairernovotny
Copy link
Member

@ViktorHofer a UWP console app is in the plan for xUnit 3.0 and will be supported by "regular xUnit" @bradwilson. xUnit 3 plans to run all tests this way, essentially creating the exe that runs the tests. Whether it's .NET Framework, .NET Core, or UWP, is just a matter of what you target.

@clairernovotny
Copy link
Member

A better question is if vstest supports UWP console apps? Right now, their test adapter for UWP is pretty specific and they provide their own GUI entry point.

If you use the latest xUnit for Devices templates (via the VSIX in the marketplace), it supports both VSTest and running standalone via F5. Either way though has GUI popup. However, I believe VSTest takes care of registering/deploying/running the test app so you don't need that C++ launcher code. I'm not completely sure though...

@ViktorHofer
Copy link
Member Author

Thanks for the info. Does that mean with xunit 3 we will have two ways of testing UWP or will the UWP support be removed from xunit for devices?

Regarding the VSTest support I will open an issue. It would be great if we could get rid of all the bundling, registering, deploying and the whole custom runner. That said I don't expect that to happen anytime soon...

@clairernovotny
Copy link
Member

You have two ways today and you'll have three ways in xUnit 3:

  1. Create a new UWP Test project using the template and switch MSTest to xUnit (and use the xUnit visual studio adapter)
  2. Use xUnit for Devices (supports both VS and standalone execution). IMO, no reason to use option 1.
  3. [New for 3.0]: xUnit UWP console app. This should work with option 1 & 2 but TBD.

@ViktorHofer
Copy link
Member Author

Understood. Thanks!

@ViktorHofer
Copy link
Member Author

As you said option 3 is still tbd but can you guess if it'll be possible to just have a "normal" csproj sdk proejct and not a UWP template? So that it will inject all the UWP stuff into it, i.e. manifest, legacy csproj, etc.

@bradwilson
Copy link

Details are TBD but almost surely we will be telling people to make console apps for their target platform, not class libraries.

@ViktorHofer
Copy link
Member Author

uapaot isn't a concern here anymore. uap will continue to use our custom runner.

@ViktorHofer
Copy link
Member Author

The dotnet test and dotnet vstest commands will probably be merged into a single dotnet test command. Additionally I found out that xunit isn't happy with not generating deps.json files for the test assemblies.

@ViktorHofer
Copy link
Member Author

I got dotnet vstest working locally with corefx. A few hurdles I hit:

  • xunit.runner.visualstudio doesn't allow passing in runsettings from the CLI (-- ...)
  • a direct dotnet test/vstest invocation (without overriding the VSTest target) won't work, unless a) dotnet is passed in as a full path, pointing to the testhost dotnet.exe and that dotnet is fully featured (has a sdk folder, etc. - not just bare bones) or b) https://github.com/dotnet/core-setup/issues/4809 is implemented.
  • the testhost dotnet is just a rip-off, it doesn't allow executing anything besides the exec task. We might want to fully binplace/hardlink the sdk nevertheless for VS Test Explorer support.
  • Warnings of tests with duplicate ids, need to investigate.

@ViktorHofer
Copy link
Member Author

@BruceForstall @sandreenko @hoyosjs do you know if switching to dotnet vstest would cause issues on the coreclr side?

@BruceForstall
Copy link
Member

@BruceForstall @sandreenko @hoyosjs do you know if switching to dotnet vstest would cause issues on the coreclr side?

As long as it supports response files for test exclusion, and doesn't have the bugs we fixed in our fork (dotnet/arcade#1613), it seems like it would be ok. Of course, it would be worth testing that hypothesis.

@hoyosjs
Copy link
Member

hoyosjs commented Jul 1, 2019

@BruceForstall @sandreenko @hoyosjs do you know if switching to dotnet vstest would cause issues on the coreclr side?

So what we do is:

  • Download the zip files
  • Use runtests blindly with our dotnet host.
  • Use the checked in RSP file that the private console runner used to parse.

Off the top of my head, we'd need to confirm the following:

  • The test host is able to run dotnet vstest (would you package the assembly, or we have to reference it) and I'm not sure how well this plays with a direct dotnet test/vstest invocation (without overriding the VSTest target) won't work, unless a) dotnet is passed in as a full path, pointing to the testhost dotnet.exe and that dotnet is fully featured (has a sdk folder, etc. - not just bare bones). As you said, the test host is brittle.
  • Filtering Looking at the vstest docs it seems RSP files are supported and test method exclusions could be made by adding --TestCaseFilter FullyQualifiedName!=TestNamespace.TestClass.TestMethod. The more generic class/namespace exclusions seem to only be available with mstest; in xunit --TestCaseFilter FullyQualifiedName~TestNamespace could help, but it might skip a lot of tests in an undesirable way (i.e. if the namespace/class name is common), so this might warrant investigation. My guess is that it should be able to parse the RSP file if we translate it to this format? Don't know if vstest allows for multiple --TestCaseFilter flags, but if it does then the only concern is class/namespace level exclusion.

@ViktorHofer
Copy link
Member Author

Don't know if vstest allows for multiple --TestCaseFilter flags

Unfortunately not :(

@mayankbansal018 @singhsarab wouldn't it make sense to allow multiple testcasefilter options and concatenate them with the & operator?

The more generic class/namespace exclusions seem to only be available with mstest; in xunit --TestCaseFilter FullyQualifiedName~TestNamespace could help, but it might skip a lot of tests in an undesirable way (i.e. if the namespace/class name is common), so this might warrant investigation.

Right, xunit only allows filtering on:

  • FullyQualifiedName
  • DisplayName
  • Traits

with exact or contains matches. I'm not sure why this limitation applies to xunit. @onovotny should know.

@clairernovotny
Copy link
Member

@ViktorHofer
Copy link
Member Author

The code that actually does the matching is part of VSTest itself

@mayankbansal018 @singhsarab do you know why VSTest doesn't offer the same rich filters for xunit as for MSTest?

@BruceForstall
Copy link
Member

cc @echesakovMSFT @RussKeldorph

@ViktorHofer
Copy link
Member Author

@mayankbansal018 @singhsarab to explain a bit more, we need to map our current rsp file https://github.com/dotnet/coreclr/blob/master/tests/CoreFX/CoreFX.issues.rsp to a format that dotnet test understands.

@ViktorHofer ViktorHofer changed the title Switch from xunit.console to dotnet vstest Switch from xunit.console to dotnet test Sep 8, 2019
@maryamariyan maryamariyan transferred this issue from dotnet/corefx Dec 16, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner labels Dec 16, 2019
@maryamariyan maryamariyan added this to the Future milestone Dec 16, 2019
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Apr 17, 2020

The remaining work to make dotnet test and VS Test Explorer without the -vs switch work for libraries:

I expect this to take about 1 dev weeks. This also fixes the issue that VS Test Explorer discovers ActiveIssue/Conditional (ie OS specific) tests which should not be run.

cc @ericstj @danmosemsft @stephentoub @safern

@ViktorHofer ViktorHofer changed the title Switch from xunit.console to dotnet test Enable dotnet test in dotnet/runtime Apr 22, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants