coverlet-coverage / coverlet

Cross platform code coverage for .NET
MIT License
2.94k stars 385 forks source link

Double-digit losses of coverage using coverlet.msbuild 3.0.0 #1037

Closed martincostello closed 3 years ago

martincostello commented 3 years ago

In multiple projects' dependabot updates to coverlet.msbuild from 2.9.0 to 3.0.0, there is a double-digit loss of apparent code coverage with zero other changes.

This seems to me like a bug of some kind, rather than a bug fix of aberrant data in a previous release, but if it genuinely is a reflection of true metric misreporting in v2 then fair enough.

Here are links to the generated pull requests and their respective drop in coverage for investigation:

PR Coverage Change
https://github.com/justeat/JustSaying/pull/846#issuecomment-757635829 -21%
https://github.com/martincostello/alexa-london-travel/pull/444#event-4189005977 -26%
https://github.com/martincostello/alexa-london-travel-site/pull/856#issuecomment-757635443 -13.44%
https://github.com/martincostello/api/pull/539#issuecomment-757634627 -22.73%
https://github.com/martincostello/azure-functions/pull/221#issuecomment-757634789 -13.38%
https://github.com/martincostello/website/pull/686#issuecomment-757635334 -17.93%
Dave-EMIS commented 3 years ago

Just tested V3 on my repo and seeing the line coverage drop from 4027 to 2100 (of 4085) using coverlet.collector.

Branch coverage remains the same.

This is a private repo, so I cannot share details.

I'm looking at the HTML in dotnet-reportgenerator-globaltool 4.8.1 using the OpenCover format (set via a runsettings file).

Some of the methods have "random" missing blocks - I cannot determine a pattern for the lines missed, but this is one generic example:

image

This is fully covered in 1.3.0

martincostello commented 3 years ago

Looks like line coverage has been affected where the "logical code line" spans over more than one line in the source file, such as a multiple-line constructor initializer:

var foo = new Bar()
{
    Baz = "qux"
};
MarcoRossignoli commented 3 years ago

Thanks for reporting this. We'll take a look asap.

Dave-EMIS commented 3 years ago

Looks like line coverage has been affected where the "logical code line" spans over more than one line in the source file, such as a multiple-line constructor initializer:

var foo = new Bar()
{
    Baz = "qux"
};

Yeah, that seems to be the case now you mention it, it is obvious :D

MarcoRossignoli commented 3 years ago

Weird, we had some guys using nightly and this issue wasn't reported and also strange for UTs.

MarcoRossignoli commented 3 years ago

cc: @daveMueller is you have some fast idea.

JSkimming commented 3 years ago

I've found the same issue in several of my projects, for example, JSkimming/Castle.Core.AsyncInterceptor#119

Here's the coverage report for reference:

https://2024-60778941-gh.circle-artifacts.com/0/Report/index.html

MarcoRossignoli commented 3 years ago

If you can confirm the issue with a small repro could be great(like the above done by Martin)

JSkimming commented 3 years ago

@MarcoRossignoli I may have time to create something small later today, though JSkimming/Castle.Core.AsyncInterceptor#119 could be exactly what you're looking for.

It's only 5 classes, and it had 100% coverage until this update, so every instance where it's missing coverage is an instance of this issue. There's also a simple bash script (coverage.sh) to execute the tests and produce the coverage report in the root.

daveMueller commented 3 years ago

I couldn't reproduce this with the multiple-line constructor initializer. I also tested the multi-line initializer within generated IL methods but this also worked.

grafik

I will try to look into the repo @JSkimming mentioned later that day. Sorry for not find a fast solution.

MarcoRossignoli commented 3 years ago

I will try to look into the repo @JSkimming mentioned later that day. Sorry for not find a fast solution.

np at all, thx for your invaluable help!

Malivil commented 3 years ago

I created a repo with four reproduction cases using coverlet.msbuild 3.0.0

https://github.com/Malivil/Coverlet300Repro

Run the GenerateCoverageReport.bat in Coverlet300ReproTests

Malivil commented 3 years ago

I did also verify it is unrelated to the SkipAutoProps flag and also unrelated to my recent upgrade to .Net 5 (The only two things that I changed other than updating Coverlet)

MarcoRossignoli commented 3 years ago

@daveMueller pretty sure the regression was introduced with this one https://github.com/coverlet-coverage/coverlet/pull/1006 we should try to revert for fast check.

Dave-EMIS commented 3 years ago

If it matters, I'm using .NET 5.0.101 SDK and building netcoreapp3.1...

Malivil commented 3 years ago

If it matters, I'm using .NET 5.0.101 SDK and building netcoreapp3.1...

I tried .Net 5.0.101 SDK and building netcoreapp3.1 as well as net5.0

I did not try with the .Net 3.1.x SDK though

bkaid commented 3 years ago

Weird, we had some guys using nightly and this issue wasn't reported and also strange for UTs.

I actually did run into this with the nightly and meant to report it but was going to try to put a repro sample together first. Sorry about that.

MarcoRossignoli commented 3 years ago

I actually did run into this with the nightly and meant to report it but was going to try to put a repro sample together first. Sorry about that.

Oh no problem at all...I'm curious to know, we have some UT's with multiline and lambda etc(like David sample above) so it's weird we didn't catch before. Anyway np we'll fix it.

daveMueller commented 3 years ago

@MarcoRossignoli I guess i found the issue and as you assumed it is related to #1006. Here https://github.com/coverlet-coverage/coverlet/blob/d43f83d8240bb7e9efa8aed5841329a30f6da7c6/src/coverlet.core/Coverage.cs#L397-L404

we don't consider the docIndex of the HitCandidates which means we are comparing lines of different classes. For a quick test I just added

HitCandidate hitCandidateToCompare in result.HitCandidates.Where(x => x.docIndex.Equals(hitCandidate.docIndex))

and it seemed to work. I will double check this tomorrow and create a PR. This would also explain why we didn't see this in our UT because there is only one class loaded per test. I hope this is the only issue and I don't find other problems tomorrow.

Thanks @Malivil for the great repro. I compiled my 'quick' fix and tested it on the project.

grafik

grafik

Malivil commented 3 years ago

Glad to be of assistance, more glad that you found the cause =)

MarcoRossignoli commented 3 years ago

Good catch @daveMueller! Can you try to fix and build package from your branch using

dotnet pack -c release /p:TF_BUILD=true /p:PublicRelease=true

And attach the three packages to this issue? So our kind/patiences friend here can try on their projects and confirm the fix?

I expect a package with "preview" tag on version.

This would also explain why we didn't see this in our UT because there is only one class loaded per test.

😢

skovalyova commented 3 years ago

The same issue for .NET 5 after updating coverlet to 3.0.0 - coverage is 20% less than for 2.9.0 (json, opencover, coberture report types). FineCodeCoverage (https://github.com/FortuneN/FineCodeCoverage) shows correct coverage.

Malivil commented 3 years ago

FineCodeCoverage installs an old version by default but will use a newer version if you update the coverlet tool yourself. If you were to update the tool it would show the same coverage problem.

daveMueller commented 3 years ago

Sorry for the late response but now here I have attached the packages with the potential fix. Would be cool if someone could test this and give some feedback. :smirk: coverlet.3.0.1-preview.4.zip

JSkimming commented 3 years ago

@daveMueller I've just tried it on Castle.Core.AsyncInterceptor and it looks like you've fixed the issue: 100% (328 of 328), whereas 3.0.0 counted 89% (196 of 220).

Here's the output.

+------------------------------+------+--------+--------+
| Module                       | Line | Branch | Method |
+------------------------------+------+--------+--------+
| Castle.Core.AsyncInterceptor | 100% | 100%   | 100%   |
+------------------------------+------+--------+--------+

+---------+------+--------+--------+
|         | Line | Branch | Method |
+---------+------+--------+--------+
| Total   | 100% | 100%   | 100%   |
+---------+------+--------+--------+
| Average | 100% | 100%   | 100%   |
+---------+------+--------+--------+

Interestingly, the numbers are now much closer to OpenCover (I used this before coverlet) which counts 100% (333 of 333).

MarcoRossignoli commented 3 years ago

Thanks @JSkimming for the fast test @Dave-EMIS @martincostello can you try to double check 🙇 ?

@daveMueller open the PR that we'll discuss the fix.

martincostello commented 3 years ago

Using the 3.0.1-preview.4 package with https://github.com/martincostello/api/pull/539 gives the following results:

+-----------+--------+--------+--------+
| Module    | Line   | Branch | Method |
+-----------+--------+--------+--------+
| API       | 84.17% | 57.08% | 98.46% |
+-----------+--------+--------+--------+
| API.Views | 88.99% | 66.41% | 83.33% |
+-----------+--------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Total   | 84.76% | 60.25% | 96.62% |
+---------+--------+--------+--------+
| Average | 86.58% | 61.74% | 90.89% |
+---------+--------+--------+--------+

So that actually improves on the previous coverage in the main branch of 84.20%. 🥳

Malivil commented 3 years ago

I'm seeing good results with the 3.0.1 preview as well

2.9.0 - 92.2% (1726 of 1871) 3.0.0 - 85.9% (1559 of 1813) 3.0.1 - 96% (1742 of 1813)

EDIT: Disregard my previous edit - I'm dumb =)

MarcoRossignoli commented 3 years ago

Thanks to super @daveMueller this fix tomorrow will be on nightly https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/ConsumeNightlyBuild.md Asap I'll push on nuget(bit busy atm).

skovalyova commented 3 years ago

Bug is closed, but update is still not available from nuget package manager in VS - when will it be added?

MarcoRossignoli commented 3 years ago

Just released new version with fix to nuget.org

ulrichb commented 3 years ago

Okay, it got way better with 3.0.1, but there are still false negatives.

Example 1: image

Example 2: image

MarcoRossignoli commented 3 years ago

@ulrichb can you open a new issue and provide a small repro? I think this is a different issue unrelated to this one, as we tested for that specific issue where the bug drops the numbers a lot.

AraHaan commented 2 years ago

Using the Following Packages on my Source Generator Tests produce the following Results:

+--------+------+--------+--------+
| Module | Line | Branch | Method |
+--------+------+--------+--------+

+---------+------+--------+--------+
|         | Line | Branch | Method |
+---------+------+--------+--------+
| Total   | 0%   | 0%     | 0%     |
+---------+------+--------+--------+
| Average | 0%   | 0%     | 0%     |
+---------+------+--------+--------+
MarcoRossignoli commented 2 years ago

@AraHaan are you using the collector integration? If not that can be the issue as it's a known one

https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/KnownIssues.md

AraHaan commented 2 years ago

I discovered the issue, the source generator known as: Microsoft.CodeAnalysis.ResxSourceGenerator generates it's code without the .g.cs ending (which my source generator uses to generate code to it's resx file).

My issue will now be able to be fixed with: https://github.com/dotnet/roslyn-analyzers/pull/5959