Fix issues with handling of multiple assemblies with the same name #426

lancehuang2021 · 2021-05-28T21:15:50Z

Thanks for creating this great tool!

Regarding this PR: In our scenarios, we have one DLL that built separately for x86 and x64. Then we run our tests against this DLL on x86 machines and x64 machines separately, also collect the code coverage data using VSTest.console.exe from Visual Studio. Eventually, we merge all the code coverage files into one file. This resulted in 2 modules (actually same module for different architectures) with the same assembly name with different IDs in the merged file.

Although ReportGenerator is able to handle multiple assemblies, but it expects the assemblies' name to be unique. Our data fails this expectation, and the report generated by ReportGenerator is mixing the coverage data from different modules together.

Another issue is that, when multiple source files are invoked in a method of the same class, that method is repeated multiple times on the class coverage page. It is not very convenient to understand which file is associated with those method entries.

This PR tries to resolve above issues.

lancehuang2021 · 2021-05-28T21:17:40Z

src/ReportGenerator.Core/Parser/DynamicCodeCoverageParser.cs

@@ -83,7 +83,7 @@ public ParserResult Parse(XContainer report)
        /// <returns>The <see cref="Assembly"/>.</returns>
        private Assembly ProcessAssembly(XElement module)
        {
-            string assemblyName = module.Attribute("name").Value;
+            string assemblyName = module.Attribute("name").Value + "_" + module.Attribute("id").Value;


This will prevent the tool from mixing code coverage data from different versions of the same module (same name) together (when those coverage data exists in the same coverage file). E.g. x86 and x64 version of the same DLL.

Are you generating the XML file by hand? I.e. are you manually copying the relevant section from the x86 coverage file to the x64 file or vice versa?
If yes: How about renaming the modules to "???_x86" and "???_x64" if you want separate results? The id looks "ugly" in the report.
In generell users would expect that modules with the same name are handled as a single module. I'm not willing to change that behavior, because users rely on it. Often ReportGenerator is used to merge several coverage files.
You are the first user that does not want to merge the results :-)

lancehuang2021 · 2021-05-28T21:18:27Z

src/ReportGenerator.Core/Parser/DynamicCodeCoverageParser.cs

@@ -187,7 +187,9 @@ private static CodeFile ProcessFile(XElement module, string fileId, ClassWithNam
            var linesOfFile = methods
                .Elements("ranges")
                .Elements("range")
+                .Where(r => r.Attribute("source_id").Value == fileId)


This prevent the tool from applying the same coverage for different files invoked in the method being processed. It should only apply the associated coverage for the file.

lancehuang2021 · 2021-05-28T21:19:27Z

src/ReportGenerator.Core/Reporting/Builders/Rendering/HtmlRenderer.cs

@@ -299,7 +299,7 @@ public void TestMethods(IEnumerable<TestMethod> testMethods, IEnumerable<FileAna
                            codeElement.FirstLine,
                            codeElement.CoverageQuota.HasValue ? coverageRounded.ToString() : "undefined",
                            codeElement.CoverageQuota.HasValue ? ReportResources.Coverage2 + " " + codeElement.CoverageQuota.Value.ToString(CultureInfo.InvariantCulture) + "% - " : string.Empty,
-                            WebUtility.HtmlEncode(codeElement.Name),
+                            WebUtility.HtmlEncode($"File {item.Key} => " + codeElement.Name),


Add file index before the method name, so that we know which file is associated with this method instance. This is to handle the scenario that multiple files may be invoke for the same method, and there are multiple method instances listed on the class coverage page.

I like the idea, but in the sidebar the horizontal space is limited. I would prefer to show the file index only in the tooltip.

lancehuang2021

Hi @danielpalme,

Could you please help to review this PR when you have time?

I am not sure why my changes would fail the 2 checks in the Checks page. Any idea?

Once this PR is approved and merge, we will contact Azure team to ask them to get a updated version of this tool for their Publish Code Coverage Result task.

Thanks!
Lance

lancehuang2021 · 2021-05-28T21:22:33Z

src/ReportGenerator.Core/Reporting/Builders/Rendering/HtmlRenderer.cs

@@ -626,7 +626,8 @@ public void KeyValueRow(string key, string value)
        /// <param name="files">The files.</param>
        public void KeyValueRow(string key, IEnumerable<string> files)
        {
-            string value = string.Join("<br />", files.Select(v => string.Format(CultureInfo.InvariantCulture, "<a href=\"#{0}\" class=\"navigatetohash\">{1}</a>", WebUtility.HtmlEncode(StringHelper.ReplaceNonLetterChars(v)), WebUtility.HtmlEncode(v))));
+            int fileIndex = 0;
+            string value = string.Join("<br />", files.Select(v => string.Format(CultureInfo.InvariantCulture, "<a href=\"#{0}\" class=\"navigatetohash\">{1}</a>", WebUtility.HtmlEncode(StringHelper.ReplaceNonLetterChars(v)), WebUtility.HtmlEncode($"File {fileIndex++}: " + v))));


Assign a file index to each file to shown on the class coverage page. So that we can reference to this file index in those method instances - when there are multiple instances of the same method listed on that page.

lancehuang2021 · 2021-05-28T21:22:56Z

src/ReportGenerator.Core/Reporting/Builders/Rendering/HtmlRenderer.cs

@@ -686,11 +687,11 @@ public void MetricsTable(Class @class)
                            WebUtility.HtmlEncode(methodMetric.FullName),
                            fileIndex,
                            methodMetric.Line,
-                            WebUtility.HtmlEncode(methodMetric.ShortName));
+                            WebUtility.HtmlEncode($"File {fileIndex} => " + methodMetric.ShortName));


Add file index before the method name, so that we know which file is associated with this method instance. This is to handle the scenario that multiple files may be invoke for the same method, and there are multiple method instances listed on the class coverage page.

lancehuang2021 · 2021-05-28T21:23:03Z

src/ReportGenerator.Core/Reporting/Builders/Rendering/HtmlRenderer.cs

                    }
                    else
                    {
-                        this.reportTextWriter.Write("<td title=\"{0}\">{1}</td>", WebUtility.HtmlEncode(methodMetric.FullName), WebUtility.HtmlEncode(methodMetric.ShortName));
+                        this.reportTextWriter.Write("<td title=\"{0}\">{1}</td>", WebUtility.HtmlEncode(methodMetric.FullName), WebUtility.HtmlEncode(file.Path + " => " + methodMetric.ShortName));


Add file index before the method name, so that we know which file is associated with this method instance. This is to handle the scenario that multiple files may be invoke for the same method, and there are multiple method instances listed on the class coverage page.

danielpalme · 2021-05-29T07:48:42Z

Thanks for your PR. I will have a closer look within the next days.

danielpalme · 2021-06-03T09:26:25Z

Can you please share a sample XML file which let's me reproduce your scenario?
If you don't want to share it publicly, you can send it by email: reportgenerator@palmmedia.de

lancehuang2021 · 2021-06-04T19:13:46Z

Can you please share a sample XML file which let's me reproduce your scenario?
If you don't want to share it publicly, you can send it by email: reportgenerator@palmmedia.de

Hi Daniel, although I can't share the actual file because of the company policy, I have created some dummy files which can repro the issue. Please see attachment.

ReproExample.zip

danielpalme · 2021-06-05T08:27:40Z

src/ReportGenerator.Core/Parser/DynamicCodeCoverageParser.cs

                .Where(l => l.Attribute("start_line").Value != "15732480")
+                .Distinct()


Does Distinct() make any difference here? You apply Distinct() to a list of XElement objects, this will always return the full list of elements. I guess you can remove that line.

Will remove it.

danielpalme · 2021-06-05T08:29:33Z

src/ReportGenerator.Core/Reporting/Builders/Rendering/HtmlRenderer.cs

@@ -299,7 +299,7 @@ public void TestMethods(IEnumerable<TestMethod> testMethods, IEnumerable<FileAna
                            codeElement.FirstLine,
                            codeElement.CoverageQuota.HasValue ? coverageRounded.ToString() : "undefined",
                            codeElement.CoverageQuota.HasValue ? ReportResources.Coverage2 + " " + codeElement.CoverageQuota.Value.ToString(CultureInfo.InvariantCulture) + "% - " : string.Empty,
-                            WebUtility.HtmlEncode(codeElement.Name),
+                            WebUtility.HtmlEncode($"File {item.Key} => " + codeElement.Name),


I like the idea, but in the sidebar the horizontal space is limited. I would prefer to show the file index only in the tooltip.

danielpalme · 2021-06-05T08:37:13Z

src/ReportGenerator.Core/Parser/DynamicCodeCoverageParser.cs

@@ -83,7 +83,7 @@ public ParserResult Parse(XContainer report)
        /// <returns>The <see cref="Assembly"/>.</returns>
        private Assembly ProcessAssembly(XElement module)
        {
-            string assemblyName = module.Attribute("name").Value;
+            string assemblyName = module.Attribute("name").Value + "_" + module.Attribute("id").Value;


Are you generating the XML file by hand? I.e. are you manually copying the relevant section from the x86 coverage file to the x64 file or vice versa?
If yes: How about renaming the modules to "???_x86" and "???_x64" if you want separate results? The id looks "ugly" in the report.
In generell users would expect that modules with the same name are handled as a single module. I'm not willing to change that behavior, because users rely on it. Often ReportGenerator is used to merge several coverage files.
You are the first user that does not want to merge the results :-)

lancehuang2021 · 2021-06-07T23:35:55Z

For some reason, I can't reply your last 2 comments, so I am replying them here:

"I like the idea, but in the sidebar the horizontal space is limited. I would prefer to show the file index only in the tooltip."

Will investigate and see how to show the file index in the tooltip. If we are to show them to the tooltip, perhaps we can show the file path instead of the index?

_"Are you generating the XML file by hand? I.e. are you manually copying the relevant section from the x86 coverage file to the x64 file or vice versa?
If yes: How about renaming the modules to "???_x86" and "???x64" if you want separate results? The id looks "ugly" in the report.
In generell users would expect that modules with the same name are handled as a single module. I'm not willing to change that behavior, because users rely on it. Often ReportGenerator is used to merge several coverage files.
You are the first user that does not want to merge the results :-) "

I used the APIs in Microsoft.VisualStudio.Coverage.IO to merge those x86 and amd64 .coverage files into one file, then convert that merged .coverage file into XML, then used ReportGenerator to convert that XML to HTML report/cobertura format. The x86 and amd64 coverage data are treated as 2 modules with different ids in the merged file. We would like both of them to be in the same report. I agree that the id does look "ugly", perhaps use "_1", "_2", ..., as the postfix to the module name when generating the report? How about an switch to enable this behavior of "not merge", and let the default behavior as merge?

danielpalme · 2021-06-08T19:27:15Z

Will investigate and see how to show the file index in the tooltip. If we are to show them to the tooltip, perhaps we can show the file path instead of the index?

I would like the following format: "File 1: PATH...."

_"Are you generating the XML file by hand? I.e. are you manually copying the relevant section from the x86 coverage file to the x64 file or vice versa?
If yes: How about renaming the modules to "???_x86" and "???x64" if you want separate results? The id looks "ugly" in the report.
In generell users would expect that modules with the same name are handled as a single module. I'm not willing to change that behavior, because users rely on it. Often ReportGenerator is used to merge several coverage files.
You are the first user that does not want to merge the results :-) "

I used the APIs in Microsoft.VisualStudio.Coverage.IO to merge those x86 and amd64 .coverage files into one file, then convert that merged .coverage file into XML, then used ReportGenerator to convert that XML to HTML report/cobertura format. The x86 and amd64 coverage data are treated as 2 modules with different ids in the merged file. We would like both of them to be in the same report. I agree that the id does look "ugly", perhaps use "_1", "_2", ..., as the postfix to the module name when generating the report? How about an switch to enable this behavior of "not merge", and let the default behavior as merge?

I have always tried to keep the number of possible command line parameters as small as possible and only added new parameters, when I was convinced that they are easy to understand and useful for the majority of the users.
Otherwise ReportGenerator would end up with (too) many parameters that no one understands.
In this case it there's another reason: ReportGenerator supports several input formats (Cobertura, OpenCover, etc). Not all of them have an additional ID available. That would make it hard to explain, that a new switch only works for CodeCoverage format.
My suggestion: We don't change the behavior and you add another step to your build process which renames one or both modules to a unique name. A little Powershell command can solve this:

((Get-Content -path CodeCoverage.xml -Raw) -replace '(.*?)(Module)(.*)', '$1Module_x86$3') | Set-Content -Path CodeCoverage.xml

lancehuang2021 · 2021-06-09T23:05:02Z

Will investigate and see how to show the file index in the tooltip. If we are to show them to the tooltip, perhaps we can show the file path instead of the index?

I would like the following format: "File 1: PATH...."

_"Are you generating the XML file by hand? I.e. are you manually copying the relevant section from the x86 coverage file to the x64 file or vice versa?
If yes: How about renaming the modules to "???_x86" and "???x64" if you want separate results? The id looks "ugly" in the report.
In generell users would expect that modules with the same name are handled as a single module. I'm not willing to change that behavior, because users rely on it. Often ReportGenerator is used to merge several coverage files.
You are the first user that does not want to merge the results :-) "

I used the APIs in Microsoft.VisualStudio.Coverage.IO to merge those x86 and amd64 .coverage files into one file, then convert that merged .coverage file into XML, then used ReportGenerator to convert that XML to HTML report/cobertura format. The x86 and amd64 coverage data are treated as 2 modules with different ids in the merged file. We would like both of them to be in the same report. I agree that the id does look "ugly", perhaps use "_1", "_2", ..., as the postfix to the module name when generating the report? How about an switch to enable this behavior of "not merge", and let the default behavior as merge?

I have always tried to keep the number of possible command line parameters as small as possible and only added new parameters, when I was convinced that they are easy to understand and useful for the majority of the users.
Otherwise ReportGenerator would end up with (too) many parameters that no one understands.
In this case it there's another reason: ReportGenerator supports several input formats (Cobertura, OpenCover, etc). Not all of them have an additional ID available. That would make it hard to explain, that a new switch only works for CodeCoverage format.
My suggestion: We don't change the behavior and you add another step to your build process which renames one or both modules to a unique name. A little Powershell command can solve this:
((Get-Content -path CodeCoverage.xml -Raw) -replace '(.*?)(Module)(.*)', '$1Module_x86$3') | Set-Content -Path CodeCoverage.xml

Make sense. Thanks for the suggestion. I will remove the change of appending the ID. Will update the PR later.

danielpalme · 2021-06-20T17:09:05Z

I just merged your PR and made the open adjustments myself.
I hope that's OK for you.

lancehuang2021 · 2021-06-20T22:29:31Z

I just merged your PR and made the open adjustments myself.
I hope that's OK for you.

That's totally fine. Thank you very much for doing that. I am still too busy working on our internal code coverage project, and was planning to update the PR later after I finished that. Glad that you have made the change.

Fix issues with handling of multiple assemblies with the same name

3f6fb3a

lancehuang2021 commented May 28, 2021

View reviewed changes

danielpalme reviewed Jun 5, 2021

View reviewed changes

danielpalme added the awaiting user label Jun 16, 2021

danielpalme merged commit f1171ac into danielpalme:master Jun 20, 2021

nikosatwork mentioned this pull request Sep 21, 2021

'File 1:' shows even if there is a single file/assembly #450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with handling of multiple assemblies with the same name #426

Fix issues with handling of multiple assemblies with the same name #426

lancehuang2021 commented May 28, 2021 •

edited

lancehuang2021 May 28, 2021 •

edited

danielpalme Jun 5, 2021

lancehuang2021 May 28, 2021

lancehuang2021 May 28, 2021 •

edited

danielpalme Jun 5, 2021

lancehuang2021 left a comment

lancehuang2021 May 28, 2021

lancehuang2021 May 28, 2021

lancehuang2021 May 28, 2021

danielpalme commented May 29, 2021

danielpalme commented Jun 3, 2021

lancehuang2021 commented Jun 4, 2021

danielpalme Jun 5, 2021

lancehuang2021 Jun 7, 2021

danielpalme Jun 5, 2021

danielpalme Jun 5, 2021

lancehuang2021 commented Jun 7, 2021

danielpalme commented Jun 8, 2021

lancehuang2021 commented Jun 9, 2021

danielpalme commented Jun 20, 2021

lancehuang2021 commented Jun 20, 2021

		.Where(l => l.Attribute("start_line").Value != "15732480")
		.Distinct()

Fix issues with handling of multiple assemblies with the same name #426

Fix issues with handling of multiple assemblies with the same name #426

Conversation

lancehuang2021 commented May 28, 2021 • edited

lancehuang2021 May 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lancehuang2021 May 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lancehuang2021 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielpalme commented May 29, 2021

danielpalme commented Jun 3, 2021

lancehuang2021 commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lancehuang2021 commented Jun 7, 2021

danielpalme commented Jun 8, 2021

lancehuang2021 commented Jun 9, 2021

danielpalme commented Jun 20, 2021

lancehuang2021 commented Jun 20, 2021

lancehuang2021 commented May 28, 2021 •

edited

lancehuang2021 May 28, 2021 •

edited

lancehuang2021 May 28, 2021 •

edited