coverlet.msbuild 2.8.0 does not combine coverage when using MergeWith

[martincostello]

Dependabot pushed a number of updates to coverlet.msbuild to repositories I maintain just now to bump from 2.7.0 to 2.8.0, and for projects where code coverage is aggregated together using the MergeWith MSBuild property, the stats are no longer being combined.

Here's such an example PR: justeattakeaway/httpclient-interception#171

This is causing builds to fail as their minimum coverage level specified by the Threshold MSBuild property is now no longer being met.

[MarcoRossignoli]

The issue seem different...from this version coverlet generate different name for different target frameworks (old bug #177 we now append current framework moniker to avoid to override same file for different frameworks) so there is a possibility that you expect file merging between different project but now coverlet generate different file name

'C:\projects\httpclient-interception\artifacts\coverage.netcoreapp3.1.json'
'C:\projects\httpclient-interception\artifacts\coverage.json'

Maybe you expect only one file for all projects, is it correct?

[MarcoRossignoli]

You've two project one with multiple target frameworks and the other with single

https://github.com/justeat/httpclient-interception/blob/195c8a0f22423f23919f5043db024d75bd404bcd/src/HttpClientInterception/JustEat.HttpClientInterception.csproj#L13
https://github.com/justeat/httpclient-interception/blob/195c8a0f22423f23919f5043db024d75bd404bcd/samples/SampleApp.Tests/SampleApp.Tests.csproj#L4

If you simply change SampleApp.Tests.csproj target framework to <TargetFrameworks>netcoreapp3.1</TargetFrameworks> it should work as expected.
Some note, remember to never run merge coverage directly to sln without -m:1 flag because build/test will run in parallel and you could have issue with results(race on parallel write to report file)https://github.com/tonerdo/coverlet/blob/master/Documentation/Examples/MSBuild/MergeWith/HowTo.md btw this not seem your case.
We're trying to understand if there is a way to be more friendly with this feature but we don't have too much support from msbuild #357 (comment) and also this feat(merge) is no more available on collector integration due to fact that we cannot chose full path name(there is a guid injected by vstest plat)
Also you need to update the name of generated file

2020-01-06T05:46:46: The report file 'd:\a\1\a\coverage.cobertura.xml' is invalid. File does not exist (Full path: 'd:\a\1\a\coverage.cobertura.xml').

You should use wilcard here https://github.com/justeat/httpclient-interception/blob/master/Build.ps1#L110 for report generator

Pipeline output https://dev.azure.com/justeatoss/httpclient-interception/_build/results?buildId=242&view=logs&jobId=2d2b3007-3c5c-5840-9bb0-2b1ea49925f3&j=2d2b3007-3c5c-5840-9bb0-2b1ea49925f3&t=ebb4541d-067d-557a-31a9-bfa53beb4d16

[martincostello]

Maybe you expect only one file for all projects, is it correct?

That's correct, as that's always been the behaviour previously.

Thanks for explaining what's wrong and how to fix it - it's unfortunate that this breaking change to the behaviour was made in a minor release and/or didn't require a command-line flag to enable, though.

[MarcoRossignoli]

Actually it was a bug 🙂 , if you check the issue related you can show what other users had to do as workaround.

BTW let me know if it's all ok or need further help, sorry for the inconvenience Martin!

[MarcoRossignoli]

Feel free to close if solved!

[martincostello]

I've gotten all my dependabot PRs fixed-up manually now - thanks for the assistance!

[MarcoRossignoli]

And thanks for the advice we'll keep more attention on versioning (related to missing breaking behavior)!

[roryprimrose]

Just to clarify, the fix to get a single merged code coverage file using 2.8.0 in a multi-target framework unit test project is to change it so it targets a single framework. If that is the case, then we forgo the ability to unit test against all our target frameworks for the sake of a single coverage output. This is a bad outcome unless I'm misunderstanding the situation.

My scenario is that I have test projects that use

<TargetFrameworks>netcoreapp2.1;netcoreapp3.0</TargetFrameworks>

in order to test a library that uses

<TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>

Testing each target framework is important because the library has target framework specific code to validate. I am also sending the data to coveralls.io via GitHub actions which requires a single lcov file.

2.7.0 did work this way with

 dotnet test /p:CollectCoverage=true  /p:CoverletOutput=../CoverageResults/ /p:MergeWith="../CoverageResults/coverage.json" /p:CoverletOutputFormat="lcov%2cjson" -m:1 

but as mentioned, 2.8.0 produces framework specific coverage files.

Is there a way around this or do I have to choose between unit testing all target frameworks and code coverage metrics?

[MarcoRossignoli]

Hi @roryprimrose, some question and thoughts:

If that is the case, then we forgo the ability to unit test against all our target frameworks for the sake of a single coverage output

I'd like to understand better your scenario and how you evaluate coverage. In case of merging different runtime how do you understand if the coverage is correct, decrease or increase?
I mean if I remove for instance netcoreapp2.1 test coverage (for a mistake) but on netcoreapp3.1 the coverage is in place yet...how do you understand that you're shipping a lib not fully covered for a netcoreapp2.1?
I've omitted netstandard tfms because they're only "contracts" and not runtimes.

I am also sending the data to coveralls.io via GitHub actions which requires a single lcov file.

Seem that coveralls.io support merging of multiple file https://docs.coveralls.io/parallel-build-webhook (btw I'm not a coverall experts)

Thoughts:
I'm convinced that "reports tool" should merge reports(but it's only my personal idea, coverlet is oss and wants solve community problem), I think that this situation is also related to misconception about solutions and project. Msbuild the engine that builds our code can build only "projects" and not solution AFAIK(from guide "Visual Studio uses MSBuild to load and build managed projects. The project files in Visual Studio (.csproj, .vbproj, .vcxproj, and others) contain MSBuild XML code that executes when you build a project by using the IDE."), so the solution is only a way to group projects and share some settings, I mean one project could be part of different solutions and will be build two time. Also the build/run of tests are by design parallel(in fact to avoid possible race on file write we append -m:1 to make run sequential, an ugly hack that slow down test workflow). When we run a test we run our built artifacts against one and only one runtime, we have to suppose that every runtime could behave differently(possible breaking changes that we're indeed testing). Again our code could behave differently between different runtime, merge more runtime toghether could make coverage unuseful. I see merge different runtime toghether an edge case scenario and not a by-design one, because to avoid that issue you have to know very well code base(in big project with a lot of devs is not so easy).
Unfortunately also from msbuild perspective is not possible today to know when all tests of a solutions are ends(or sync them in some way) #357 (comment) and merge feat is not present on coverlet for vstest integration(the recommended way to consume coverlet if you've know issue https://github.com/tonerdo/coverlet/blob/master/Documentation/KnowIssues.md#1-vstest-stops-process-execution-earlydotnet-test) because vstest doesn't allow to specify full output file name #662 (comment)), so with merge feat we're trying to do our best but we don't have a great support from "environment".

That said if this issue is a common issue we could add a new switch to preserve old(to me buggy and dangerous) behaviour for instance /p:MergeWithInSingleFile=true false by default or something like that.
@roryprimrose if I didn't convince you feel free to open a new issue with this problem and we'll wait for some community thumbs up/agreement.

cc: @tonerdo @petli