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

Road to code coverage #960

Open
6 of 12 tasks
ViktorHofer opened this issue Jan 27, 2019 · 16 comments
Open
6 of 12 tasks

Road to code coverage #960

ViktorHofer opened this issue Jan 27, 2019 · 16 comments
Labels
area-Infrastructure-libraries backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity
Milestone

Comments

@ViktorHofer
Copy link
Member

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

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

cc @danmosemsft @safern @ericstj @stephentoub

@ViktorHofer
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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

@ViktorHofer
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Ok

@sharwell
Copy link
Member

sharwell commented Jan 28, 2019

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 (coverlet-coverage/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
Copy link
Member

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
Copy link
Member

safern commented Jan 28, 2019

@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
Copy link
Member Author

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 ViktorHofer self-assigned this Mar 25, 2019
@ViktorHofer
Copy link
Member Author

Updated issue with current status.

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented May 9, 2019

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

@ViktorHofer
Copy link
Member Author

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
Copy link
Member

MarcoRossignoli commented Jun 7, 2019

@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: coverlet-coverage/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.

@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 ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Jun 23, 2020
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this issue Jul 1, 2021
@ViktorHofer ViktorHofer removed their assignment Jul 22, 2024
Copy link
Contributor

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

@dotnet-policy-service dotnet-policy-service bot added backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity labels Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Infrastructure-libraries backlog-cleanup-candidate An inactive issue that has been marked for automated closure. no-recent-activity
Projects
None yet
Development

No branches or pull requests

8 participants