Removes the not instrumented/unused instrumenter results so coverage file doesn't get polluted with unused modules

[pape77]

@tonerdo for when you have a sec to review :)

Sorry for the easy fix on the test though. I could not quickly think in a more clever way for fixing and testing that.

This addresses #223 in which I describe that sometimes unused instrumented results introduce new branches with hits=0 , polluting the final coverage and making the branch coverage to go down when it's not supposed to.

The actual tests that reference and use that instrumented result don't include such branches, so when the final result is produced using MergeWith, those branches will always remain with hits=0, lowering coverage, which is not correct.

With this change, only actually used instrumented results will be included in the output coverage file. And when using MergeWith the output will be as expected and described in #223

[codecov]

Codecov Report

Merging #225 into master will decrease coverage by 0.7%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   94.91%   94.21%   -0.71%     
==========================================
  Files          15       15              
  Lines        1633     1641       +8     
==========================================
- Hits         1550     1546       -4     
- Misses         83       95      +12

Impacted Files	Coverage Δ
src/coverlet.core/Coverage.cs	99.51% <100%> (+0.01%)	⬆️
src/coverlet.core/Helpers/InstrumentationHelper.cs	92.34% <0%> (-6.13%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b54c93c...b5b9354. Read the comment docs.

[pape77]

I updated to add the exclusion of non called/instrumented files as an option, to keep current behaviour as default

[tonerdo]

Not a fan of adding an option for every anomaly we encounter, it could lead to option bloat. I'm in the process of adding verbose logging functionality to coverlet, it'll spit out info when it encounters a non-called assembly and the user can choose to exclude it with the regular Exclude option

[pape77]

@tonerdo but again, the exclude option only works when testing each test project separately.
If you want to run a dotnet test over the whole solution (as I'm trying to do) then you can't exclude that non called assembly on only one test project because it IS being used by the other test projects, that will actually cover the branches on that same assembly.

I'm not sure if the problem is completely clear to you, this is leading to wrong branch coverage % and it's affecting all coverages when merging if not solved anyway (maybe if you are lucky, those non called assemblies won't introduce extra non covered branches. Or maybe users don't notice it. I was lucky to spot it by chance, when I thought about why my coverage was so low on branch level).

In addition to having to run adotnet test per test project with it's corresponding excludes (which you should know beforehand), you cannot use coverlet in, say, Dockerfile or a generic script for your CI/CD to make it generic for all your solutions (each test project will have to exclude a different assembly, so you have to specify which one for each of them).

My aim is to have one line for running all tests with coverage (so dotnet test over any solution, using coverlet) without including non called assemblies which introduce fake branch coverage with hits =0 on the final coverage file. How would you suggest to solve that? (I know this PR makes that possible)

[pape77]

@tonerdo I'm not a fan of adding options also, I just did beacause you mentioned you wanted to keep the original behaviour. Personally, I see no reason for not doing this, since it's ruining the coverage when merging and giving a false one. So the tool is not working properly, and that's not something that should be fixed using an Exclude everytime, anyway. Could you please reconsider? An entire organization wants this to happen :) (I can tell them to upvote this if you want)

[tonerdo]

Hi @pape77, sorry I seem to have let this slide. Will take a look and see if I can come up with an idea that works without having to add an extra option

[pape77]

@tonerdo well the no extra options version of this was my original pr :P I just added it because you said you wanted to keep the original behaviour (for some reason I still don't get)

[pape77]

@MarcoRossignoli I saw you marked this as enhancement, is something going to happen about this?

[MarcoRossignoli]

@pape77 I maked all PRs to have an idea of "contex" and keep thinghs organized.
I didn't go deep on details and I didn't discuss with @tonerdo about it.

[pape77]

@MarcoRossignoli I still notice the same behavior with current version of coverlet console (1.5.3) while running tests projects that reference some dlls that are not part of the code that we write ourselves. This dlls get included as part of the coverage (which will never be covered by tests of course, since they are downloaded from somewhere else).

Example:

My test bin folder looks like this, and when i run coverlet for this, i get:

Calculating coverage result...
  Generating report '.\test-results\coverage\coverage.opencover.xml'
  Generating report '.\test-results\coverage\coverage.json'
  Generating report '.\test-results\coverage\coverage.cobertura.xml'
+--------------------------------------------------+--------+--------+--------+
| Module                                           | Line   | Branch | Method |
+--------------------------------------------------+--------+--------+--------+
| Rtl.ContentMetadata.Client                       | 100%   | 100%   | 100%   |
+--------------------------------------------------+--------+--------+--------+
| Rtl.ContentMetadata.Models                       | 63.79% | 100%   | 78.94% |
+--------------------------------------------------+--------+--------+--------+
| xunit.runner.reporters.netcoreapp10              | 1.1%   | 0.48%  | 5.08%  |
+--------------------------------------------------+--------+--------+--------+
| xunit.runner.utility.netcoreapp10                | 15.64% | 9.2%   | 20.93% |
+--------------------------------------------------+--------+--------+--------+
| xunit.runner.visualstudio.dotnetcore.testadapter | 45.59% | 35.56% | 47.76% |
+--------------------------------------------------+--------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Total   | 24.18% | 17.89% | 28.01% |
+---------+--------+--------+--------+
| Average | 4.836% | 3.578% | 5.602% |
+---------+--------+--------+--------+

as you see xunit.runner dlls are included as part of the coverage, which we don't want since we won't ever cover this. And i think i solved this by what i changed in this pr by then. Not sure if it's still appliable to current code, but probably worth a look?
You can try including this kind of dlls to your test proj and run it to get the same thing (hopefully)

[MarcoRossignoli]

xunit will be escluded by default in next version #462

[MarcoRossignoli]

@tonerdo have you got opinions on this old PR?

[pape77]

@MarcoRossignoli this made the magic.
I just included a parameter for not doing this by default, because @tonerdo wanted to keep original behaviour (for some reason)
But this solved excluding any specific stuff that's not part of your code. Together with loop in line 245
You may want to try if it's still applicable. The referenced issue is probably now deprecated.
Current problem is more like coverlet trying to cover dlls that are not part of your code

[MarcoRossignoli]

At the moment I don't have a strong opinion, but I think that if we support this feature should be throught switch(like in this PR) because could be also useful understand why some module are not "tested", in big project someone could lose "the problem" of "disabled" tests.

[pape77]

Not sure if i can think an scenario in which you wouldn't see not tested modules which are part of your code.
Maybe if you don't test Api lvl ,then api.dll won't pop up and coverage will be higher. That's what you mean? but then you immediately don't see it covered in your coverage report or console output

[MarcoRossignoli]

but then you immediately don't see it covered in your coverage report or console output

Yes but I think a scenario where I have tens of modules...if I see module with 0% it's immediatly clear that something went wrong(for instance temporary commented tests and not re-enabled) if you trim that result I could lose the issue, make sense?

[pape77]

@MarcoRossignoli Sounds like a monolith case. Makes sense in that case I guess. But worth to make a option parameter I think. For us who are really building microservices with couple of csprojs in it which are usually having the same structure

[MarcoRossignoli]

Sounds like a monolith case. Makes sense in that case I guess.

Yep could be(I have this case 😄) btw a switch options would satisfy everyone, what do you think?

[pape77]

That's why i did this pr this way :) Without fully understanding the reasons for keeping original behavior.
Would be nice to go out of my fork and back to using coverlet again and contributing of course

[MarcoRossignoli]

Wait for @tonerdo opinion.

[pape77]

@tonerdo i still need this for making use of coverlet for an entire organization across Europe :) (RTL).
Can we consider it so i can stop using my own fork and join you guys on improving coverlet together?

[MarcoRossignoli]

@tonerdo I think we could add a parameter to exclude empty hit files...but I wouldn't do that by "default" because could hide important issue.
Again with merge of new logging capabilities on tracker also in case of flag usage we can ask to enable logging to check if tracker has wrote or note.
I propose --exclude-untested-modules because is the only "lecit"(no bug) reason.

[pape77]

@tonerdo @MarcoRossignoli i was always fine with a parameter solution for this. Will make our life easier, instead of constantly syncing with the master coverlet version. I made some code duplication removal on my version as well which could have been a contribution to the main repo for example.

[pape77]

@MarcoRossignoli would it be an idea to create a pr? Or is it a lot of work.
Maybe if the idea gets materialized @tonerdo won't mind. Similar to what i did here with a param

[MarcoRossignoli]

@MarcoRossignoli would it be an idea to create a pr? Or is it a lot of work.
Maybe if the idea gets materialized @tonerdo won't mind. Similar to what i did here with a param

The work is not small thing I'd like to be sure that @tonerdo and @petli agree with feature and avoid to waste time.

[MarcoRossignoli]

@pape77 are you using your fork with this feat?
I think that we could add this feature, but should be supported by all drivers msbuild/collector/.net tool, would you like to taking up this PR again(not obligated at all)?
We should add log(warning) in case of result removal due to missing hit file https://github.com/tonerdo/coverlet/blob/master/src/coverlet.core/Coverage.cs#L329

We have enough thumbs up and yes we can use exclude but indeed it's not so comfortable to setup for every project in large solutions.

[pape77]

@MarcoRossignoli we are using the VSTest version, i don't remember know if anything else. It's working for splitted test projects. I can look at the specifics when I'm at the office if it's useful for including it in the wiki or somewhere.
The other versions of the tool still do the same behaviour that this pr intended to prevent, at least last time i checked (two months ago?)
I've been away from the project for too long now, so no idea what's the status now for this change to be done. Maybe it's still as simple as adding an if as I did here, maybe not.
(I saw a lot of stuff changed like not even printing the grid with results anymore in console).
Plus, given the fact i made a pr back then and we asked several times for the need of this with no answer until now, what would it be different now in that matter?

[MarcoRossignoli]

@MarcoRossignoli we are using the VSTest version, i don't remember know if anything else. It's working for splitted test projects. I can look at the specifics when I'm at the office if it's useful for including it in the wiki or somewhere.

You mean you're using VSTest Integration https://github.com/tonerdo/coverlet#vstest-integration-preferred-due-to-know-issue?

The other versions of the tool still do the same behaviour that this pr intended to prevent, at least last time i checked (two months ago?)

All version do the same thing, the core is the same(instrumentation) the only difference are supported parameters(for instance merge is not supported on vstest integration)

I've been away from the project for too long now, so no idea what's the status now for this change to be done. Maybe it's still as simple as adding an if as I did here, maybe not.
(I saw a lot of stuff changed like not even printing the grid with results anymore in console).

The core isn't changed so much, and the grid is not printing on vstest integration because is not supported at the moment on vstest side. On vstest coverlet is injected as a collectors and by design console output is captured and handled by vstest plat. To support console output we have to add a custom console logger #681

At the moment there is a drift in features supported by vstest integration and other driver(msbuild/.net tool), the roadmap is filling this gaps now that we're out with some stable versions https://github.com/tonerdo/coverlet/issues?q=is%3Aissue+is%3Aopen+label%3Adriver-collectors

Also there is a new idea for the most requested cross feat, merging #683

Plus, given the fact i made a pr back then and we asked several times for the need of this with no answer until now, what would it be different now in that matter?

Because the "issue" with "non used" asm is present yet for all drivers(msbuild/vstest/.net tool) and it's not related to only msbuild one.
Since I'm planning next release(in 3 month more or less) I'm checking PR/Issue and as I've said if it's not so comfortable for big solutions add exclude filter for different project could be acceptable to me add that flag.

BTW if this is no more necessary feel free to close this PR!

Thank's a lot for your help!Very appreciated!

[pape77]

@MarcoRossignoli that is really strange because i remember using the latest msbuild and necore tool and it still did this include unused libraries thing. I'll check what we do now with vstest and come back with this

[MarcoRossignoli]

@MarcoRossignoli that is really strange because i remember using the latest msbuild and necore tool and it still did this include unused libraries thing

Maybe in past you hit often https://github.com/tonerdo/coverlet/blob/master/Documentation/KnowIssues.md#1-vstest-stops-process-execution-earlydotnet-test that lead to that 0% because hit file wasn't flushed to disk before process kill, with vstest we're synched with plat and process won't be killed until we signal "our" flush.

[p4p3]

@MarcoRossignoli I have no clue. Anyhow, this is what we do (we use merge with in VSTest version of the tool, so the XPlat thing, together with coverlet.collector):

# set MergeWith value in runsettings.xml
$runsettingsFile = "$pipelineFolder/runsettings.xml"
$coverageJsonFullPath = "$testResultsDirectory/coverage.json"
(Get-Content $runsettingsFile).Replace('#mergewith#', $coverageJsonFullPath) | Set-Content $runsettingsFile

# calculate code coverage
Get-ChildItem -Path "test/unit","test/integration","test/component" -Recurse -Filter *.csproj | 
Foreach-Object {
  $dir = "$testResultsDirectory/$($_.BaseName)"
  dotnet add  $_.FullName package coverlet.collector -v 1.1.0
  dotnet test $_.FullName --collect:"XPlat Code Coverage" --settings $runsettingsFile --no-build --logger trx --results-directory $dir
  Copy-Item -Path "$dir/*/coverage.json" -Destination $coverageJsonFullPath -Force
}

# copy results to $testResultsDirectory
Copy-Item -Path "$dir/*/coverage.cobertura.xml" -Destination $testResultsDirectory
Copy-Item -Path "$dir/*/coverage.opencover.xml" -Destination $testResultsDirectory

runsettings look like this:

<?xml version="1.0" encoding="utf-8" ?>
<RunSettings>
    <DataCollectionRunSettings>
        <DataCollectors>
            <DataCollector friendlyName="XPlat code coverage">
                <Configuration>
                    <Format>opencover,json,cobertura</Format>
                    <MergeWith>#mergewith#</MergeWith>
                    <Exclude>[*]*Migrations*</Exclude> <!-- [Assembly-Filter]Type-Filter -->
                    <ExcludeByAttribute>Obsolete,GeneratedCodeAttribute,CompilerGeneratedAttribute</ExcludeByAttribute>
                    <ExcludeByFile>**/Program.cs,**/test/**/*</ExcludeByFile> <!-- Absolute or relative file paths -->
                    <SingleHit>false</SingleHit>
                </Configuration>
            </DataCollector>
        </DataCollectors>
    </DataCollectionRunSettings>
    <InProcDataCollectionRunSettings>
        <InProcDataCollectors>
            <InProcDataCollector assemblyQualifiedName="Coverlet.Collector.DataCollection.CoverletInProcDataCollector, coverlet.collector, Version=1.1.0.0, Culture=neutral, PublicKeyToken=null"
                                 friendlyName="XPlat Code Coverage"
                                 enabled="True"
                                 codebase="coverlet.collector.dll" />
        </InProcDataCollectors>
    </InProcDataCollectionRunSettings>
</RunSettings>

note that in the first snippet, we keep the latest merged file in $dir which is set to the directory of the latest test lib that has been run, that's why we just copy the cobertura and opencover versions of it after the loop. This works fine

[p4p3]

And since we want to check the merged file for a threshold:

# compare code coverage to $coverageThreshold
$coverage =  Select-Xml -Path "$testResultsDirectory/coverage.cobertura.xml" -XPath "/coverage/@branch-rate" | % {[double]::Parse($_.Node.Value) * 100}
Write-Output "Coverage is $coverage"
if ($coverage -lt $coverageThreshold) {
  throw "Code coverage $coverage is less than threshold $coverageThreshold"
}

This may be useful for some other people that want to do this

[MarcoRossignoli]

@MarcoRossignoli I have no clue. Anyhow, this is what we do (we use merge with in VSTest version of the tool, so the XPlat thing, together with coverlet.collector):

Strange mergewith has got some issue on vstest #662 (comment) how do you merge reports if file is generated in different folder for different projects?

[pape77]

It's always taking the mergeWith param, so it's always creating a new file by merging with the one at $coverageJsonFullPath. Then it just force copies the newly generated coverage.json to $coverageJsonFullPath every time, to overwrite it

[MarcoRossignoli]

It's always taking the mergeWith param, so it's always creating a new file by merging with the one at $coverageJsonFullPath. Then it just force copies the newly generated coverage.json to $coverageJsonFullPath every time, to overwrite it

Uh missed that piece I see good idea!

[MarcoRossignoli]

@pape77 move to 1.2.0!

[MarcoRossignoli]

For now I'll close this PR because seem no more useful.

Feel free to re-open if needed.