Add support for merging results

[basilfx]

This PR adds support for merging results of multiple projects. It is similar to how OpenCover supports this.

It is as simple as adding /p:CoverletIntermediateResult=intermediate.json to the arguments. Just ensure that this file doesn't exist when you would record the test results of multiple projects.

I have created this test project (you have to fix reference to Coverlet package), which demonstrates how two projects that both cover 50% will yield 100% when merging the intermediate results. I have also verified that the report generated by ReportGenerator looks as expected.

Calculating coverage result...

Merging with intermediate result...
  Intermediate result written to '../intermediate.json'

Writing report(s)...
  Generating report 'C:\Projects\GitHub\Coverlet-Test\tests\CoverletTests.FooTests\coverage.opencover.xml'
  Generating report 'C:\Projects\GitHub\Coverlet-Test\tests\CoverletTests.FooTests\coverage.cobertura.xml'

+--------------+--------+--------+--------+
| Module       | Line   | Branch | Method |
+--------------+--------+--------+--------+
| CoverletTest | 100%   | 100%   | 100%   |
+--------------+--------+--------+--------+

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

References:
#36

[basilfx]

Here is a more detail explanation of the changes:

The results are merged CoverageResultTask.cs, just before the reports are generated. I've added Read(string path) to IReporter, to convert a previous result back into a CoverageResult. I have only implemented this for the JsonReporter, but one could extend this for other reporters, if it would make sense.

To merge the coverage results, I have CoverageResultExtensions, which isolates all of the merge operations. However, I needed to rewrite the way branches were recorded. I changed it from

public class Branches : SortedDictionary<int, List<BranchInfo>> { }

into

public class Branches : SortedDictionary<(int Number, int Offset, int EndOffset, int Path, uint Ordinal), HitInfo> { }

I found that a tuple of (Number, Offset, EndOffset, Path, Ordinal) uniquely identifies the branch (therefore its the key), and that the number of hits is its value.

Unfortunately, the JSON serializer (Jil) does not support keys other than strings or integers. And since it also doesn't support converters, I could not rewrite this without converting the CoverageResult into a serializable structure. The easiest option was to move to Newtonsoft.Json.

The story doesn't stop here, as Newtonsoft.Json would serialize tuples as strings (e.g. (1, 2, 3, 4, 5)). I therefore implemented two custom converters (which it supports) to store line information and branch information. This means that the Coverlet JSON format has changed to this:

...
"System.Boolean CoverletTest.Application::DoSomething(System.Boolean)": {
  "Lines": [
    {
      "Key": {
        "Line": 21
      },
      "Value": {
        "Hits": 2
      }
    }
  ],
  "Branches": [
    {
      "Key": {
        "Number": 11,
        "Offset": 4,
        "EndOffset": 22,
        "Path": 1,
        "Ordinal": 1
      },
      "Value": {
        "Hits": 1
      }
    }
  ]
}
...

There are still a few things that I haven't fixed (properly). One of them is the way how the results are stored per assembly. If you take the test project I created, you'll end up with CoverletTest.dll twice: one will reside in CoverletTests.FooTests\bin\Debug and the other in CoverletTests.BarTests\bin\Debug. This will create two assembly entries in the CoverageResult, while technically they are the same. This results in a result like this:

Calculating coverage result...

Merging with intermediate result...
  Intermediate result written to '../intermediate.json'

Writing report(s)...
  Generating report 'C:\Projects\GitHub\Coverlet-Test\tests\CoverletTests.BarTests\coverage.opencover.xml'
  Generating report 'C:\Projects\GitHub\Coverlet-Test\tests\CoverletTests.BarTests\coverage.cobertura.xml'

+--------------+--------+--------+--------+
| Module       | Line   | Branch | Method |
+--------------+--------+--------+--------+
| CoverletTest | 61,1%  | 50%    | 66,7%  |
+--------------+--------+--------+--------+
| CoverletTest | 61,1%  | 50%    | 66,7%  |
+--------------+--------+--------+--------+

+---------+--------+--------+--------+
|         | Line   | Branch | Method |
+---------+--------+--------+--------+
| Average | 61,1%  | 50%    | 66,7%  |
+---------+--------+--------+--------+

I solved this (for now) by just taking the file name and stripping the full path. A better solution would be preferred, but this will change the resulting JSON again (similar to the branches above).

[sjk07]

@basilfx All this work is awesome! I really want this to happen. Just a few things that came up when thinking this problem though that i wanted to put out there.

I'm not familiar with the code base at all, so please excuse me for being so naive.

One question i would ask is why are we bothering to output the intermediate results to a file in the first place? I would think the most efficient and straight forward way to make this happen is to have an in memory collection of all tests ran and keep adding the results until all found tests have completed. The user could also input a directory / list where all the test projects are located. Then a single file can be output to a default / location of the users liking.

I would think this would eliminate your serialization issues and knowing all the test projects before hand will allow you to merge all the assemblies

As i said originally, this is probably very naive since I have not taken the time to look though how the code currently works. Just wanted to put it out there

[basilfx]

@sjk07 Thanks for the feedback. The reason why I use a intermediate results file is because they are used across multiple processes, so you cannot keep it in memory. From a coverlet point of view, it is not aware if a particular run is the first or the last, so you would need an additional step to process the results (doable, but increases complexity).

I also don’t know the tests on beforehand. I could use a wildcard to run all tests, or run dotnet test on the solution.

My approach is easy. It doesn’t require scripting (e.g. to discover the test) so it runs very easily in environments such as VSTS.

[tonerdo]

@basilfx thanks for taking the time out to look into this. Will take a look once I get the chance. Don't worry about the Travis failure for now, the .NET Core runtime installed on it doesn't support assembly signing. Will move to Appveyor Linux build agents

[tonerdo]

Hey @basilfx, please rebase master so that the CI build can kick in. I've transitioned the Linux tests over to Appveyor which already runs all Windows tests

[basilfx]

@tonerdo Rebased. It seems to work :-)

[tonerdo]

Hey @basilfx, I've taken a look through your code and I do appreciate the effort. I have a couple of questions as well as thoughts on how to make this implementation much simpler.

• Any particular reason you renamed LineInfo to HitsInfo?

• In terms of the implementation I'm a bit confused why you decided to go with using tuples as the key for branches. Wasn't the existing information in the coverage result json representative enough and the BranchInfo object directly usable.

• In terms of user experience I'd expect something along the lines of /p:Merge=/path/to/coverage.json where the file specified is the coverage result output of a previous run from another project

• I don't think there's a need to call merge specifically in the msbuild tasks. A much simpler implementation that comes to mind will be to simply pass the supplied file in the Merge property to the Coverage class, deserialize it and perform the merge within that class. Merge functionality should be a function of the core library, msbuild tasks should be limited entirely to presentation of results.

• which demonstrates how two projects that both cover 50% will yield 100% when merging the intermediate results.

  From my knowledge of how merging coverage results work I should expect that the situation above should only be true if each of the two projects cover a different 50% of the assembly under test. Is that the case with your implementation?

• It's ok for Coverlet to only support merging results in its own json format.

In summary, I think it'll be simpler and a lot cleaner to just accept a coverage.json pass that down to Coverage class and have a single merge method within that class that performs the merging (alternatively you should create a CoverageMerger class as opposed to extension methods). I do have a merge algorithm in mind that should greatly simplify things and remove the duplicate entry issue you're encountering and also remove the need to make any major changes to the structure of existing types.

Let me know your thoughts

[basilfx]

Thanks @tonerdo for looking at the code. Here are my thoughts:

In terms of the implementation I'm a bit confused why you decided to go with using tuples as the key for branches. Wasn't the existing information in the coverage result json representative enough and the BranchInfo object directly usable.

No, the BranchInfo object wasn't directly usable. As far as I understand, every branch is uniquely identified by a combination of values. Therefore you have a mapping of (Number, Offset, EndOffset, Path, Ordinal) -> Hits. In the old situation you had (Number) -> (Offset, EndOffset, Path, Ordinal, Hits), which is a bit odd (and wrong, IMHO). It also requires more effort when merging the results, because you have to do multiple lookups.

I think that this should be changed, apart from merging the coverage results. That's why I added it as a separate commit.

I guess it should be easy to convert this tuple into a object, e.g. BranchPoint, but that would require more work. This was the easiest.

Any particular reason you renamed LineInfo to HitsInfo?

Since BranchInfo was stripped down, both LineInfo and BranchInfo only contained the number of hits. I therefore renamed them to HitsInfo.

In terms of user experience I'd expect something along the lines of /p:Merge=/path/to/coverage.json where the file specified is the coverage result output of a previous run from another project

I fully agree, but the point is that Coverlet support multiple output formats. I think it is therefore more confusing for the end-user that it doesn't work whenever JSON format is not selected, so have you any idea's about that?

I don't think there's a need to call merge specifically in the msbuild tasks. A much simpler implementation that comes to mind will be to simply pass the supplied file in the Merge property to the Coverage class, deserialize it and perform the merge within that class. Merge functionality should be a function of the core library, msbuild tasks should be limited entirely to presentation of results.

Agreed. This was, however, the easiest for me to implement :-)

From my knowledge of how merging coverage results work I should expect that the situation above should only be true if each of the two projects cover a different 50% of the assembly under test. Is that the case with your implementation?

Here is the code. One test project covers the if-part, the other the else-part.

I don't have much time coming period to work on this. so I am fine with considering this a first attempt, and start from scratch. Especially since you have an idea on how to merge properly :-)

[ErikSchierboom]

Great to see this is being worked on!

[tonerdo]

I fully agree, but the point is that Coverlet support multiple output formats. I think it is therefore more confusing for the end-user that it doesn't work whenever JSON format is not selected, so have you any idea's about that?

Just like OpenCover which has it's primary format, Coverlet also has it's own JSON format. The support for other formats is entirely for compatibility reasons and to make it easy to integrate Coverlet with existing tools.

Here is the code. One test project covers the if-part, the other the else-part.

Ah ok. That's fine then

No, the BranchInfo object wasn't directly usable. As far as I understand, every branch is uniquely identified by a combination of values. Therefore you have a mapping of (Number, Offset, EndOffset, Path, Ordinal) -> Hits. In the old situation you had (Number) -> (Offset, EndOffset, Path, Ordinal, Hits), which is a bit odd (and wrong, IMHO). It also requires more effort when merging the results, because you have to do multiple lookups.

I'm not very sold on the new format you came up with. It feels a bit counter-intuitive to have Key and Value fields. We need to be sure to not impulsively change the json output format as it'll cause thrid party tools that leverage the output to perform certain functionality to break. I'm trying to avoid introducing constant breaking changes. I'll make some modifications to the output format within the next couple of hours that should not be that much of a drastic change from how it is now. Hopefully, that'll be easier to work with.

[basilfx]

Just like OpenCover which has it's primary format, Coverlet also has it's own JSON format. The support for other formats is entirely for compatibility reasons and to make it easy to integrate Coverlet with existing tools.

Point is that whenever someone doesn't output Coverlet JSON, they cannot merge results. So IMHO it is more confusing to the end-user, as opposed to having a /p:<insert name here> that will always write the results to that file.

I'm not very sold on the new format you came up with. It feels a bit counter-intuitive to have Key and Value fields. We need to be sure to not impulsively change the json output format as it'll cause thrid party tools that leverage the output to perform certain functionality to break. I'm trying to avoid introducing constant breaking changes. I'll make some modifications to the output format within the next couple of hours that should not be that much of a drastic change from how it is now. Hopefully, that'll be easier to work with.

It should be very easy to output the old format by changing the serializer. I just wanted to demonstrate that the Key composes of multiple numbers, instead just one. That's something you have to take into account I think.

[tonerdo]

Apologies for the edit on your last comment. Didn't realize I was not editing my post, didn't even know I could do that!

[tonerdo]

Point is that whenever someone doesn't output Coverlet JSON, they cannot merge results. So IMHO it is more confusing to the end-user, as opposed to having a /p: that will always write the results to that file.

I totally get your point. If a user defaults to opencover as their format of choice they still have to output and extra file to support merging with results from another project. I'd much prefer that extra file to be the primary json coverage result also I don't want to introduce any new properties if I can help it

[risogolo]

Guys when this feature will be merged to master? any schedule?

[basilfx]

I currently have no time/intention to work on this. I'll close it for now.