search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.62k stars 4.56k forks source link

Road to code coverage #960

Open ViktorHofer opened 5 years ago

ViktorHofer commented 5 years ago

The list of remaining work necessary for automated code coverage:

Full code coverage

Currently cc for System.Private.CoreLib is broken. These steps are necessary to fix that:

Make code coverage faster (optional)

Integrate codecov into corefx

Integrate cc and codecov into continuous PRs

cc @danmosemsft @safern @ericstj @stephentoub

ViktorHofer commented 5 years ago

@safern do we use Helix for official runs too? It would be nice if we could use helix for the official code coverage run(s).

danmoseley commented 5 years ago

We also need to solve the early termination problem right? I see in the vstest issue they say it needs to be a DiagnosticCollector to avoid early termination.

Re speed, I dont believe we often use branch coverage today . Is there a flag to only instrument for block coverage, would that help?

ViktorHofer commented 5 years ago

No that issue doesn't affect us as we don't use vstest right now.

ViktorHofer commented 5 years ago

Re speed, I dont believe we often use branch coverage today . Is there a flag to only instrument for block coverage, would that help?

There isn't such a flag yet. Fixing the above linked issue would be the ideal first step to increase perf. The current instrumentation logic is suboptimal.

stephentoub commented 5 years ago

I dont believe we often use branch coverage today

@danmosemsft, what do you mean by this? Where do we "use" line/block coverage but don't branch coverage? I personally look at both.

danmoseley commented 5 years ago

If it made a significant difference in perf it might mean we could afford to get at least some coverage data automatically in CI rather than none. If we can get both great.

stephentoub commented 5 years ago

Ok

sharwell commented 5 years ago

Re speed, I dont believe we often use branch coverage today . Is there a flag to only instrument for block coverage, would that help?

I strongly recommend not going down this path. Branch and/or sequence point coverage is an extremely valuable metric for the primary value-add of code coverage (guided code reviews).

If it made a significant difference in perf ...

It will not have a substantial impact on performance. Your biggest performance gains will come from --single-hit (tonerdo/coverlet#309) and, if required, selective exclusions of extremely-frequently-hit helpers. The latter is a PerfView-guided process that I'm happy to help with after coverage tooling is set up.

sharwell commented 5 years ago

Add a Windows, Linux (Ubuntu) and OSX leg to AzDO that runs with every PR and enable codecov uploads.

I recommend starting with Windows Debug and observing stable uploads for at least a week. After that, we can add one additional Debug upload (Linux Debug or Mac Debug), and let it stablize for at least a week. Our uploads will stress the report merge process used by codecov on multiple fronts:

  1. The reports will be large, possibly approaching or exceeding the system limits even without multiple uploads. Reporting a single report avoids the need to merge reports on codecov servers, which improves our chances of success with all the different features they offer.
  2. The merge process used by codecov is a complicated process intended to accurately represent the combined coverage of multiple runs. For example, if you have an if (IsWindows) statement with a true branch covered in one build and a false branch covered in a different build, the combined report should show that the statement is fully covered by the combined report but only partially covered when you filter the view to just Windows or Linux. However, the merge process assumes that the IL offset of the branch will not change between the two uploads, and does a suboptimal job of handling cases where it does change. This is especially problematic when attempting to upload Debug and Release reports, and less problematic when uploading reports combining deterministic+unconditional compilation.
safern commented 5 years ago

@safern do we use Helix for official runs too? It would be nice if we could use helix for the official code coverage run(s).

Yes, we do use Helix for official runs. Yes we should figure out how to hook up code coverage with helix 😄

ViktorHofer commented 5 years ago

The codecov dotnet tool is now available: https://www.nuget.org/packages/Codecov.Tool/

Remaining external blocker is the release of an updated coverlet.console package.

ViktorHofer commented 5 years ago

Updated issue with current status.

MarcoRossignoli commented 5 years ago

@ViktorHofer new package with your fixes https://www.nuget.org/packages/coverlet.console/

ViktorHofer commented 5 years ago

Status: dotnet test 3.x soon supports coverlet inbox which would help a lot here. Maybe we should give https://github.com/dotnet/corefx/issues/34338 another try.

MarcoRossignoli commented 5 years ago

@ViktorHofer we have also a nightly build https://www.myget.org/feed/coverlet-dev/package/nuget/coverlet.collector Could be interesting if "sometimes" we can "propose" in some way(throught a PR on some deps config) a usage of particular nightly version to catch issue asap, I mean corefx repo has got a lot of users and if we introduce some bug it's highly probable that someone here hit issue immediatly. Clearly should be possible quickly "revert" to stable one. If it's too invasive risky, complex or simply a bad idea, never mind.

Use single hit option for non official builds: tonerdo/coverlet#309

Done https://github.com/tonerdo/coverlet/blob/master/Documentation/VSTestIntegration.md#advanced-options-supported-via-runsettings

cc: @tonerdo and @AArnott that helping with build worfklow.