Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve cobertura absolute/relative path report generation #661

Merged
merged 14 commits into from
Dec 21, 2019
Merged

Improve cobertura absolute/relative path report generation #661

merged 14 commits into from
Dec 21, 2019

Conversation

daveMueller
Copy link
Collaborator

closes #413

@daveMueller
Copy link
Collaborator Author

I've tested it on Windows and Linux both with Jenkins build server and Cobertura Plugin. ReportGenerator is also still working.

@MarcoRossignoli MarcoRossignoli changed the title 413_ProjectRootAsBasePath Improve cobertura absolute/relative path report generation Dec 20, 2019
@MarcoRossignoli MarcoRossignoli added tenet-reporters Issue related to coverage output files(reporters) enhancement General enhancement request labels Dec 20, 2019
MarcoRossignoli
MarcoRossignoli reviewed Dec 20, 2019
View reviewed changes
test/coverlet.core.tests/Reporters/CoberturaReporterTests.cs Outdated
string absolutePath3;
string absolutePath4;
string absolutePath5;
string absolutePath6;

if (isWindows)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isWindows is used only in one place use directly RuntimeInformation.IsOSPlatform(OSPlatform.Windows)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

MarcoRossignoli
MarcoRossignoli reviewed Dec 20, 2019
View reviewed changes
src/coverlet.core/Reporters/CoberturaReporter.cs Outdated

var basePathSegments = new List<string>();

splittedPaths[0].Select((value, index) => (value, index)).ToList().ForEach(x =>
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli Dec 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, rename x in fragmentIndexPair

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@MarcoRossignoli
Copy link
Collaborator

Some comments but idea is ok to me, thank's!

@MarcoRossignoli
Copy link
Collaborator

FYI after merge of this dotnet/runtime#934 (comment) we'll do splits with zero allocation.

MarcoRossignoli
MarcoRossignoli approved these changes Dec 21, 2019
View reviewed changes
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!Many thank's!

@MarcoRossignoli MarcoRossignoli merged commit 8f9d705 into coverlet-coverage:master Dec 21, 2019
@MarcoRossignoli MarcoRossignoli mentioned this pull request Dec 21, 2019
Merged
@daveMueller daveMueller deleted the 413_ProjectRootAsBasePath branch December 21, 2019 22:38
@abbotware abbotware mentioned this pull request Dec 22, 2019
Closed
@abbotware
Copy link

running the nightly build - I noticed the slashes are not normalized

on linux the paths are all:
folder/folder/file

and windows:
folder\folder\file

Might not be an issue.. but perhaps worth noting

@MarcoRossignoli
Copy link
Collaborator

running the nightly build - I noticed the slashes are not normalized

I don't think that's a problem, reporters should understand all OS path format and @daveMueller can confirm that it works on some reporters like jenkins/report generator...btw before this update were they normalized in some way?

@daveMueller
Copy link
Collaborator Author

daveMueller commented Dec 22, 2019 •
edited
Loading

Yes I can confirm that it works on the reporters Jenkins Cobertura Plugin and Report Generator. I checked the old code and there it also wasn't normalized. https://github.com/tonerdo/coverlet/blob/b3c99fffe757431403a0f4f3c4cc1e798ff97863/src/coverlet.core/Reporters/CoberturaReporter.cs#L155
Also Path.DirectorySeparatorChar was used to concatenate the strings and thus the format also was bound to the OS specific path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcoRossignoli MarcoRossignoli MarcoRossignoli approved these changes

Assignees

@daveMueller daveMueller

Labels
enhancement General enhancement request tenet-reporters Issue related to coverage output files(reporters)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to change paths in cobertura report
3 participants
@daveMueller @MarcoRossignoli @abbotware