Fix cobertura Jenkins reporter + source link support #614

daveMueller · 2019-11-10T01:33:13Z

closes #197, closes #408 and really closes #295 and maintains #257

update fork

Update fork

daveMueller · 2019-11-10T02:04:00Z

Filename attribute is now relative to system root and the reports can properly be displayed in jenkins running on linux or windows. I also attached a snipped form the generated report where the source tag and the filename tag can be seen.

This doesn't really solve the issue #413 because the introduced relative paths aren't relative to the project base.

MarcoRossignoli · 2019-11-10T12:46:28Z

Thanks David bit busy next week I'll take a look asap!

MarcoRossignoli · 2019-11-21T13:53:28Z

src/coverlet.core/CoverageResult.cs

    {
        public string Identifier;
        public Modules Modules;
+        public bool IsSourceLinkUsed;


nit: I prefer to maintain switch name UseSourceLink

MarcoRossignoli · 2019-11-21T13:54:21Z

src/coverlet.core/Reporters/CoberturaReporter.cs

 using System.Globalization;
 using System.IO;
 using System.Linq;
+using System.Runtime.InteropServices;


Is this necessary?

MarcoRossignoli · 2019-11-21T14:49:09Z

src/coverlet.core/Reporters/CoberturaReporter.cs

            return Encoding.UTF8.GetString(stream.ToArray());
        }
+
+        private static IEnumerable<string> GetRootDirs(Modules modules, bool isSourceLinkUsed)


why not

if (isSourceLinkUsed) { return new[] { string.Empty }; } else { return modules.Values.SelectMany(k => k.Keys).Select(Directory.GetDirectoryRoot).Distinct(); }

Does it work?

Yes I tested it. Done.

MarcoRossignoli · 2019-11-21T15:03:24Z

src/coverlet.core/Reporters/CoberturaReporter.cs

+            return sources.Select(Directory.GetDirectoryRoot).Distinct();
+        }
+
+        private static string GetRelativePathFromBase(IEnumerable<string> basePaths, string path, bool isSourceLinkUsed)


rename rootPaths and useSourceLink pls

MarcoRossignoli · 2019-11-21T15:12:55Z

src/coverlet.core/Reporters/CoberturaReporter.cs

+            {
+                if (path.StartsWith(basePath))
+                {
+                    relativePath = path.Substring(basePath.Length);


what do you think about

if (isSourceLinkUsed) { return path; } foreach (var root in basePaths) { if (path.StartsWith(root)) { return path.Substring(root.Length); } } Debug.Assert(false, "Unexpected, we should find at least one path starts with one pre-build roots list"); return path;

it seems more logic to me...when we found one valid root we can return immediatly, if we don't found any valid root(unexpected) we return original path, so we avoid exception and we'll generate a file(better than nothing), I added also an assertion so on debug we'll catch possible problem.

Yes, I agree. Done.

MarcoRossignoli · 2019-11-21T15:17:46Z

Thank's @daveMueller for the effort here and sorry for the delay.

I'd like to add more tests on this

one test with 2 file from two different windows roots i.e. c:\proj\file.cs and e:\proj\file.cs
one test with source link parameter true

MarcoRossignoli · 2019-11-28T13:14:39Z

@daveMueller let me know if you want complete this or if can finish this PR on top of your precious work!

daveMueller · 2019-11-28T23:29:42Z

@daveMueller let me know if you want complete this or if can finish this PR on top of your precious work!

@MarcoRossignoli Yes I will complete it. I already did most of the things you mentioned locally. My daytime job has been keeping me busy last week but I think I will find some free time on the weekend.

MarcoRossignoli · 2019-11-29T06:18:26Z

No worries no rush, only asked to know!
And feel free to say "I don't have time" you're not obligated, thanks a lot for your effort!

daveMueller · 2019-12-05T00:36:39Z

@MarcoRossignoli I still want to refactore the tests. Had some problems to get them running on X plat and so they are still not really clean. If you have some ideas what could be changed, please let me know.

MarcoRossignoli · 2019-12-05T08:55:46Z

test/coverlet.core.tests/Reporters/CoberturaReporterTests.cs

+            CoberturaReporter reporter = new CoberturaReporter();
+            string report = reporter.Report(result);
+
+            var doc = XDocument.Load(new MemoryStream(Encoding.UTF8.GetBytes(report)));


What do you think about

List<string> rootPaths = doc.Element("coverage").Element("sources").Elements().Select(e => e.Value).ToList(); List<string> relativePaths = doc.Element("coverage").Element("packages").Element("package").Element("classes").Elements().Select(e => e.Attribute("filename").Value).ToList(); List<string> possiblePaths = new List<string>(); foreach (string root in rootPaths) { foreach (string relativePath in relativePaths) { possiblePaths.Add(Path.Combine(root, relativePath)); } } Assert.Contains(absolutePath1, possiblePaths); Assert.Contains(absolutePath2, possiblePaths);

This should be the expected alg of every reporter that can read a cobertura format...we try every permutation root+path and check if exists

Yes I changed it that way.

MarcoRossignoli · 2019-12-05T09:00:55Z

test/coverlet.core.tests/Reporters/CoberturaReporterTests.cs

+            CoberturaReporter reporter = new CoberturaReporter();
+            string report = reporter.Report(result);
+
+            var doc = XDocument.Load(new MemoryStream(Encoding.UTF8.GetBytes(report)));


ok maybe cleanup a bit with doc.Element("coverage").Element("packages").Element("package").Element("classes").Elements().Select(e => e.Attribute("filename").Value).Single();

MarcoRossignoli · 2019-12-05T09:24:39Z

@daveMueller can you test and attach also a reportgenerator result?Only to be sure to be compatible with it(most used tool).

daveMueller · 2019-12-07T21:50:33Z

@MarcoRossignoli are the attached screenshots sufficient?

Here is the relevant part of a cobertura report:

And this is the reportgenerator output for the cobertura report.

MarcoRossignoli · 2019-12-07T21:56:46Z

yep thank you

MarcoRossignoli · 2019-12-07T21:57:43Z

@danielpalme can you take a look at report generated at #614 (comment) and double confirm that it's ok?
This is the idea of how a reported should interpret a cobertura report https://github.com/tonerdo/coverlet/pull/614/files#diff-1eb780449e930439d94fbbcd49400e39R176 for us, are we close enough?

danielpalme · 2019-12-08T10:19:07Z

Your code looks fine to me.
ReportGenerator iterates through the source elements and combines the value with the filename attribute. The first combined file path that exists on disk is used.
Another option would be to just use the absolute path for the the filename attribute. Then no "guessing" is required.
Advantages:

Faster
More reliable (example a class with same name exists in several directories/namespaces)

See:
https://github.com/danielpalme/ReportGenerator/blob/master/src/ReportGenerator.Core/Parser/Preprocessing/CoberturaReportPreprocessor.cs

MarcoRossignoli · 2019-12-08T10:32:49Z

Another option would be to just use the absolute path for the the filename attribute. Then no "guessing" is required

This is the old behavior, but we need to split root+relative due to Jenkins reporter, it doesn't work well without sources

MarcoRossignoli

Looks fine!

MarcoRossignoli · 2019-12-08T16:09:26Z

Thank's Dave!

daveMueller added 10 commits August 18, 2019 01:49

Merge pull request #1 from tonerdo/master

a6e0687

update fork

Merge pull request #2 from tonerdo/master

84c0aff

Update fork

Merge pull request #3 from tonerdo/master

5b4e316

Update fork

Merge pull request #4 from tonerdo/master

45075e1

Update fork

Merge pull request #5 from tonerdo/master

ef7d0b0

Update fork

Merge pull request #6 from tonerdo/master

a50f143

Update fork

Merge pull request #7 from tonerdo/master

28e038c

Update fork

Merge pull request #8 from tonerdo/master

a8198b6

Update fork

cobertura report depending on the sourcelink switch

83cae50

nit

26e4959

This was referenced Nov 10, 2019

Wrong source path in cobertura report with similar project name starts #197

Closed

Cobertura reporter breaks on 2.6.0 #408

Closed

daveMueller changed the title ~~#197, #408, #413 Coberturoa Reporter Jenkins + Source Link Support~~ #197, #408 Coberturoa Reporter Jenkins + Source Link Support Nov 10, 2019

MarcoRossignoli added the tenet-reporters Issue related to coverage output files(reporters) label Nov 10, 2019

MarcoRossignoli reviewed Nov 21, 2019

View reviewed changes

daveMueller added 5 commits December 4, 2019 23:47

code review

7ffb1de

changed to absolute windows paths

ab54646

added switch for OS

529437f

changes to filepaths

595194f

change to test

761662b

MarcoRossignoli reviewed Dec 5, 2019

View reviewed changes

refactored tests

bc24dd4

MarcoRossignoli changed the title ~~#197, #408 Coberturoa Reporter Jenkins + Source Link Support~~ Fix cobertura Jenkins reporter + source link support Dec 7, 2019

MarcoRossignoli approved these changes Dec 8, 2019

View reviewed changes

MarcoRossignoli merged commit 57ec8d3 into coverlet-coverage:master Dec 8, 2019

This was referenced Dec 8, 2019

Update Changelog.md #648

Merged

Normalize File Paths In Generated Reports (Linux / Windows) #649

Closed

daveMueller mentioned this pull request Dec 17, 2019

Unable to change paths in cobertura report #413

Closed

daveMueller deleted the 197_WrongSourcePath branch October 27, 2023 20:18

Fix cobertura Jenkins reporter + source link support #614

Fix cobertura Jenkins reporter + source link support #614

Uh oh!

Conversation

daveMueller commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daveMueller commented Nov 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarcoRossignoli commented Nov 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daveMueller Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli commented Nov 21, 2019

Uh oh!

MarcoRossignoli commented Nov 28, 2019

Uh oh!

daveMueller commented Nov 28, 2019

Uh oh!

MarcoRossignoli commented Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daveMueller commented Dec 5, 2019

Uh oh!

MarcoRossignoli Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli commented Dec 5, 2019

Uh oh!

daveMueller commented Dec 7, 2019

Uh oh!

MarcoRossignoli commented Dec 7, 2019

Uh oh!

MarcoRossignoli commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielpalme commented Dec 8, 2019

Uh oh!

MarcoRossignoli commented Dec 8, 2019

Uh oh!

MarcoRossignoli left a comment

Choose a reason for hiding this comment

Uh oh!

MarcoRossignoli commented Dec 8, 2019

Uh oh!

Reviewers

Assignees

Labels

daveMueller commented Nov 10, 2019 •

edited

Loading

daveMueller commented Nov 10, 2019 •

edited

Loading

MarcoRossignoli Nov 21, 2019 •

edited

Loading

MarcoRossignoli Nov 21, 2019 •

edited

Loading

daveMueller Dec 5, 2019 •

edited

Loading

MarcoRossignoli commented Nov 29, 2019 •

edited

Loading

MarcoRossignoli Dec 5, 2019 •

edited

Loading

MarcoRossignoli commented Dec 7, 2019 •

edited

Loading