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

Fix coverage paths sanitization issue #1411

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix coverage paths sanitization issue #1411

wants to merge 3 commits into from
+37 −14

Conversation

marschattha
Copy link
Member

@marschattha marschattha commented Jan 7, 2025 •
edited
Loading

Attempt to fix #1392

As we can see in the image below of the coverage data in the issue, the file paths have the prefix home/circleci/project/ in the coverage changes table, which I think are not matching with the files table computed from diff below causing this issue. This PR attempts to make existing path sanitization more robust and clear as the one we currently have as that seems error prone.

image

@qltysh qltysh
Copy link
Contributor

qltysh bot commented Jan 7, 2025 •
edited
Loading

The code coverage on the diff in this pull request is 96.3%

Drilldown
Path File Coverage Δ
qlty-config/src/library.rs -10.4
qlty-coverage/src/transformer.rs 3.1
qlty-cli/src/commands/plugins/enable.rs -5.1

brynary
brynary reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@brynary brynary left a comment

Choose a reason for hiding this comment

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

Can you please add unit tests for this?

@brynary
Copy link
Member

brynary commented Jan 7, 2025

Also, for the benefit of the future, could you please add content to the PR description with the bug you are working on and the theory behind the fix?

@qltysh qltysh
Copy link
Contributor

qltysh bot commented Jan 7, 2025 •
edited
Loading

All good ✅

brynary
brynary reviewed Jan 8, 2025
View reviewed changes
Copy link
Member

@brynary brynary left a comment

Choose a reason for hiding this comment

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

This new test looks simple and direct - what I am not understanding is why the code doesn't pass that test before this change. Can you please explain that?

@marschattha
Copy link
Member Author

Since we are using file paths to match coverage of different files as strings the strings need to be exact matches, and some reporters can report in absolute paths instead of paths relative to project root or working directory.

So we had this StripPrefix transformer in place, the issue with the existing implementation was that it was using strings as input and striping strings, which does not account for extra backslashes that might remain after striping base paths.

For example, if you strip /home/user/project_root from /home/user/project_root/app/nested/file.rb as strings you will get /app/nested/file.rb where as converting them into paths and then striping will get you app/nested/file.rb also that will take care of windows/cross platform compatibility seamlessly.

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

@qltysh qltysh[bot] qltysh[bot] left review comments

@brynary brynary brynary left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: code coverage PR status: Not applicable. There was no coverage data reported for the files in this diff.
2 participants
@marschattha @brynary