-
Notifications
You must be signed in to change notification settings - Fork 352
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
Add support for code coverage using Coverlet + ReportGenerator #3919
Conversation
I had a bit of time this week to play with enabling CodeCoverage in our repo. I was able to get this working on AzDo for one of AspNetCore's repos: https://github.com/aspnet/Extensions/pull/2308/files. I started off looking at the targets that CoreFx does, but they run their tests differently than the Arcade way, so I ended up having to go down this path. |
I'll take a look next week. |
Thanks for exploring that path. Just a few things to point out. You are using coverlet.console here which is a global tool which means that repositories need to add a dotnet-tools.json file (https://github.com/dotnet/corefx/blob/master/.config/dotnet-tools.json) to their repository. Arcade already incrementally calls into dotnet tool restore during the toolset restore. The problem with using coverlet.console is that it doesn't work well with dotnet test and you need to use an in-proc data collector. We are currently exploring how much work it would be to switch away from the legacy xunit.console runner to dotnet test, that's why I'm mentioning this. That said, this looks good for the current xunit.console runner but there might be some work when changing to dotnet test. |
Do you have any idea of what the time line for this change is? It was fairly trivial to get Related, prior to Arcade, AspNetCore used this target to do parallel test runs: https://github.com/aspnet/BuildTools/blob/master/files/KoreBuild/modules/vstest/module.targets. It loosely relied on the VSTest target which might make it much more malleable to use with coverlet. |
This is used as the tracking issue on the VSTest's side: microsoft/vstest#2065. The HtmlLogger was just implemented in microsoft/vstest#2103 and should be ready. Besides that I don't know if anything else needs to happen before we can switch. In corefx we haven't switched yet as we need to run the code coverage instrumentation not as part of the build (hooked into msbuild) but as part of the test in our RunTest scripts. That's currently working fine with the combination of coverlet.console and xunit.console but I'm unsure if that's still possible with coverlet.console and dotnet test (see coverlet-coverage/coverlet#110 for more information - TL;DR writing the hitcounts during the assembly unloading can take longer than what VSTest allows before they terminate the process). |
Hooking the instrumentation of the assemblies into the build pipeline should be trivial and doesn't even require a global tool. See https://github.com/tonerdo/coverlet/blob/master/Documentation/VSTestIntegration.md for the instructions. |
Actually corefx will be able to soon switch to dotnet test and coverlet.collector as I just found a workaround for the blocking issue which is completely specific to corefx. Coverlet runs on the same shared framework assemblies that it tries to instrument which causes FileNotFoundExceptions, as the files are locked. With the option to specify environment variables in the .runsettings file we can now run the instrumentation code on a different shared framework than the one which we instrument and for which collect coverage numbers by setting DOTNET_ROOT which will be respected by the testhost which is an apphost. The only remaining issue is that the apphost feature is only used on Windows. I don't see any remaining issues for arcade to not switch to dotnet test. The HtmlLogger is implemented and the .runsettings file allows to specify environment variables if necessary. |
/cc @mjanecke who was looking at coverlet for code coverage |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@pranavkm @ViktorHofer is this still something we're interested in merging? |
It did work when I tried this a year ago, but I'm not sure how out of date this PR is. That said, we would certainly be very interested in getting automated coverage going and I'd be happy to contribute to make this PR work. |
No description provided.