Different results when rerunning CoverageReportParser against same input files

[afscrome]

If I re-run CoverageReportParser.ParseFiles against the same input coverage files, I'm getting different total line counts.

string[] coverageFiles = { /*...*/ };

var parallelism = 1;
CoverageReportParser parser = new CoverageReportParser(parallelism, parallelism, new string[] { }, new DefaultFilter(new string[] { }),
    new DefaultFilter(new string[] { }),
    new DefaultFilter(new string[] { }));

ReadOnlyCollection<string> collection = new ReadOnlyCollection<string>(coverageFiles);
var results = parser.ParseFiles(collection);

var lines = results.Assemblies.Select(x => x.CoveredLines).Sum();
Console.WriteLine($"LINES: {lines}");
// The above is printing different results from run to run

I believe the problem is coming from some kind of concurrency race condition - after removing the parallelism from the following line, I start getting consistent results.

ReportGenerator/src/ReportGenerator.Core/Parser/CoberturaParser.cs

Line 135 in 4444f14

Parallel.ForEach(classNames, c => this.ProcessClass(modules, assembly, c.Name, c.DisplayName));

Digging further, I came across the following, and confirmed that my report files are ending up with multiple classes with the same name. When the earlier parallelism is enabled, these duplicates get added in different orders, meaning the FirstOrDefault can return different results if re-run against the same inputs.

ReportGenerator/src/ReportGenerator.Core/Parser/Analysis/Assembly.cs

Line 180 in 4444f14

var existingClass = this.classes.FirstOrDefault(c => c.Name == @class.Name);

At this point I'm getting a bit lost in the weeds, but I have two theories for the root cause.

The first is that the Equals method for ClassNameParserResult compares both Name and DisplayName, meaning we get all unique PAIRS of Name and DisplayName - giving us opportunities for later duplicates

ReportGenerator/src/ReportGenerator.Core/Parser/CoberturaParser.cs

Lines 122 to 128 in 4444f14

	var classNames = modules
	.Where(m => m.Attribute("name").Value.Equals(assemblyName))
	.Elements("classes")
	.Elements("class")
	.Select(c => ClassNameParser.ParseClassName(c.Attribute("name").Value, this.RawMode))
	.Where(c => c.Include)
	.Distinct()

The other possible cause is a mismatch between the following bits of logic in filtering for elements for a given class:

ReportGenerator/src/ReportGenerator.Core/Parser/CoberturaParser.cs

Lines 122 to 128 in 4444f14

	var classNames = modules
	.Where(m => m.Attribute("name").Value.Equals(assemblyName))
	.Elements("classes")
	.Elements("class")
	.Select(c => ClassNameParser.ParseClassName(c.Attribute("name").Value, this.RawMode))
	.Where(c => c.Include)
	.Distinct()

ReportGenerator/src/ReportGenerator.Core/Parser/CoberturaParser.cs

Lines 149 to 157 in 4444f14

	var files = modules
	.Where(m => m.Attribute("name").Value.Equals(assembly.Name))
	.Elements("classes")
	.Elements("class")
	.Where(c => c.Attribute("name").Value.Equals(className)
	|| (!this.RawMode
	&& (c.Attribute("name").Value.StartsWith(className + "$", StringComparison.Ordinal)
	|| c.Attribute("name").Value.StartsWith(className + "/", StringComparison.Ordinal)
	|| c.Attribute("name").Value.StartsWith(className + ".", StringComparison.Ordinal))))

ReportGenerator/src/ReportGenerator.Core/Parser/CoberturaParser.cs

Lines 190 to 198 in 4444f14

	var classes = modules
	.Where(m => m.Attribute("name").Value.Equals(@class.Assembly.Name))
	.Elements("classes")
	.Elements("class")
	.Where(c => c.Attribute("name").Value.Equals(className)
	|| (!this.RawMode
	&& (c.Attribute("name").Value.StartsWith(className + "$", StringComparison.Ordinal)
	|| c.Attribute("name").Value.StartsWith(className + "/", StringComparison.Ordinal)
	|| c.Attribute("name").Value.StartsWith(className + ".", StringComparison.Ordinal))))

[danielpalme]

Can you share the coverage files you are using?

You can send them privately via email: reportgenerator@palmmedia.de or share them here.

[AlexanderBartoshZ]

Same for me.
Regenerating reports with history on the same cobertura file generates reports with differences.
@danielpalme Do you need any additional info to fix this ?

[danielpalme]

@AlexanderBartoshZ
A sample cobertura file and history files would be great.
And some instructions which commands you are executing.

[AlexanderBartoshZ]

@danielpalme Thank you for quick reaction!
You should have the coverage report in your inbox.
Repro:
Run this command 2 times:
reportgenerator -reporttypes:html -reports:output.cobertura.o.xml -targetdir:htmlh -historydir:h

Open the html report and check the difference - you expect none, but you get some:

Let me know if you need any other info

The coverage is collected using dotnet-coverage collect -f cobertura from muplitple processes.
Some of the "changed" coverage lines are about the code that only runs within one process

[danielpalme]

Thank you! Will have a look as soon as possible!

[danielpalme]

@AlexanderBartoshZ
I was able to fix the problem with the unexpected changes in history comparison.

One remaining problem is, that some classes appear twice in the report (e.g. Aspire.Dashboard.Otlp.Storage.CircularBuffer<T>).

I guess you are using Microsoft CodeCoverage, therefore nested classes appear differently in the Cobertura file (compared to tools like coverlet).
Details can be found here: #663.

[afscrome]

Sorry for missing this @danielpalme . Not sure I'm allowed to share my files I'm afraid.

That said, I do believe I have a variant of the the same duplicate classes issue you highlighted above, although my issue is happening specifically withe Merge. I've tested locally and #697 fixes my inconsistency.

[AlexanderBartoshZ]

Thank you @danielpalme. As soon as there is a binary I can verify it with - Will gladly do that!
Yes, as stated in the initial comment, dotnet-coverage (aka https://github.com/microsoft/codecoverage) is used

With regards to #663 :
What would be the right argument to change the . to / :

1. You cannot distinguish between nested classes and non-nested ones (?)

If there are enough arguments IMO the pressure for a change of https://github.com/microsoft/codecoverage can be built up.
IF those do not work correctly with VS it could be a heavy argument

[danielpalme]

@AlexanderBartoshZ

If there are enough arguments IMO the pressure for a change of https://github.com/microsoft/codecoverage can be built up.
IF those do not work correctly with VS it could be a heavy argument

Microsoft does not want to change form . to '/'. See:
microsoft/codecoverage#124 (comment)

[AlexanderBartoshZ]

Yes. Have read that ...
But IMO, If there is a good argument they will have to