-
Notifications
You must be signed in to change notification settings - Fork 258
feat(coverage): add automatic path fixing for CI/CD environments #2338
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.
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
base: main
Are you sure you want to change the base?
Conversation
Implement DefaultPathFixer transformer that automatically removes common CI/CD path prefixes from code coverage reports when manual path fixing options are not specified. This improves the user experience by eliminating the need for manual configuration in most standard CI environments. Supported CI/CD systems: - CircleCI (/home/circleci/) - GitHub Actions (/home/runner/work/, /Users/runner/work/) - Travis CI (/home/travis/build/) - Jenkins (/var/lib/jenkins/workspace/) - GitLab CI (/builds/) - Bitbucket Pipelines (/opt/atlassian/pipelines/agent/build/) - AWS CodeBuild (/codebuild/output/) - Azure Pipelines (/home/vsts/work/) The DefaultPathFixer is automatically applied in both publish and transform commands when users don't specify --path-replace or --path-prefix options, making code coverage integration seamless across different CI/CD platforms. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
No issue mentions found. Please mention an issue in the pull request description. Use GitHub automation to close the issue when a PR is merged |
Diff Coverage for ubuntu-latest: The code coverage on the diff in this pull request is 98.0%. Total Coverage for ubuntu-latest: This PR will increase coverage by 0.15%. File Coverage Changes
🛟 Help
|
Diff Coverage for macos-15: The code coverage on the diff in this pull request is 98.0%. Total Coverage for macos-15: This PR will increase coverage by 0.16%. File Coverage Changes
🛟 Help
|
The pattern r'^/?home/[^/]+/src/([^/]+/){3}' was too generic and could match legitimate project paths. Removed to avoid false positives.
@brynary I did a risk assessment of this pull request and also some thoughts on possible enhancements. Take away is that this is probably good to go as is. RisksRisk: over-eager pattern matching which transforms paths that shouldn't get transformedThese are the patterns:
Of course, anything is possible, but these seem largely safe. We could introduce a flag like Risk: doesn't work in practice (missed or incorrect patterns)There are unit tests for the patterns which are good. The risk I guess is that these CI providers have different paths than the ones we've specified, and in which case, the behavior will be the same as today, which is that a strip prefix argument must be supplied. Seems ok. Risk: isn't backwards compatible -- doesn't properly apply a customers add or strip prefix transformationThe tests in EnhancementsThink this can largely wait, but was curious on your take:
|
Thanks this is super helpful! We can release on a weekend to limit any thundering herd of problems if we make a mistake. One thing we should add is output like a line like this:
I don't think that has to be dynamic. In other words I think it's fine to render Enabled if we are attempting auto path fixing regardless of whether any paths are adjusted. (Since path fixing is applied per path, the question of "was auto path fixing applied?" is not binary anyway. We could in theory print the counts but I don't think that's needed yet.) |
Depends on perspective I guess -- it's binary if any path fixing was applied. That said, it's certainly simpler to state that Auto-Path fixing is enabled. |
@noahd1 we need to add a path fix for: |
Yeah, better than just "enabled" would be "Enabled but not applied" vs. "Enabled and applied" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements automatic path fixing for code coverage reports in CI/CD environments by introducing a DefaultPathFixer
transformer that removes common CI/CD path prefixes when manual path fixing options are not specified.
- Added
DefaultPathFixer
transformer with support for 8+ CI/CD systems (CircleCI, GitHub Actions, Travis CI, etc.) - Updated both publish and transform planners to automatically apply path fixing when manual options are absent
- Added comprehensive test coverage for the new functionality and maintained backward compatibility
Reviewed Changes
Copilot reviewed 7 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
qlty-coverage/src/transformer.rs | Implements DefaultPathFixer transformer with regex patterns for CI/CD path detection and Windows path normalization |
qlty-coverage/src/transform/planner.rs | Conditionally applies DefaultPathFixer to transform operations when manual path options aren't provided |
qlty-coverage/src/publish/planner.rs | Conditionally applies DefaultPathFixer to publish operations and tracks auto-fixing status |
qlty-coverage/src/publish/plan.rs | Adds auto_path_fixing_enabled field to track when automatic path fixing is active |
qlty-coverage/src/publish/processor.rs | Passes auto_path_fixing_enabled flag from plan to report |
qlty-coverage/src/publish/report.rs | Adds auto_path_fixing_enabled field to Report struct |
qlty-coverage/src/validate.rs | Updates test helper to include auto_path_fixing_enabled field |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
let pattern_strings = vec![ | ||
r"^/?home/circleci/project/", | ||
r"^/?(home|Users)/runner/work/[^/]+/[^/]+/", | ||
r"^/?github.com/[^/]+/[^/]+/", | ||
r"^/?(home|Users)/travis/build/[^/]+/[^/]+/", | ||
r"^/?(home|Users)/jenkins/jobs/[^/]+/workspace/", | ||
r"^/?Users/distiller/[^/]+/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex patterns lack documentation explaining which CI/CD systems they target. Consider adding comments like // CircleCI
, // GitHub Actions
, etc. to clarify which pattern matches which CI system.
let pattern_strings = vec![ | |
r"^/?home/circleci/project/", | |
r"^/?(home|Users)/runner/work/[^/]+/[^/]+/", | |
r"^/?github.com/[^/]+/[^/]+/", | |
r"^/?(home|Users)/travis/build/[^/]+/[^/]+/", | |
r"^/?(home|Users)/jenkins/jobs/[^/]+/workspace/", | |
r"^/?Users/distiller/[^/]+/", | |
let pattern_strings = vec![ | |
// CircleCI | |
r"^/?home/circleci/project/", | |
// GitHub Actions | |
r"^/?(home|Users)/runner/work/[^/]+/[^/]+/", | |
// GitHub Actions (checkout path) | |
r"^/?github.com/[^/]+/[^/]+/", | |
// Travis CI | |
r"^/?(home|Users)/travis/build/[^/]+/[^/]+/", | |
// Jenkins | |
r"^/?(home|Users)/jenkins/jobs/[^/]+/workspace/", | |
// CircleCI (macOS) | |
r"^/?Users/distiller/[^/]+/", | |
// Azure Pipelines |
Copilot uses AI. Check for mistakes.
return stripped.to_string(); | ||
} | ||
// Handle any single letter drive | ||
if path.len() >= 3 && path.chars().nth(1) == Some(':') && path.chars().nth(2) == Some('/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling path.chars().nth()
multiple times is inefficient as it iterates through the string each time. Consider using path.chars().take(3).collect::<Vec<_>>()
or checking bytes directly if ASCII is sufficient.
if path.len() >= 3 && path.chars().nth(1) == Some(':') && path.chars().nth(2) == Some('/') { | |
if path.as_bytes().get(1) == Some(&b':') && path.as_bytes().get(2) == Some(&b'/') { |
Copilot uses AI. Check for mistakes.
if let Some(stripped) = path.strip_prefix("C:/") { | ||
return stripped.to_string(); | ||
} | ||
if let Some(stripped) = path.strip_prefix("D:/") { | ||
return stripped.to_string(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded drive letter checks for C:/ and D:/ are redundant since the generic check on lines 247-248 handles any single letter drive. Remove the specific C:/ and D:/ checks to eliminate duplication.
if let Some(stripped) = path.strip_prefix("C:/") { | |
return stripped.to_string(); | |
} | |
if let Some(stripped) = path.strip_prefix("D:/") { | |
return stripped.to_string(); | |
} |
Copilot uses AI. Check for mistakes.
let auto_path_fixing_enabled = transformers | ||
.iter() | ||
.any(|t| format!("{:?}", t).contains("DefaultPathFixer")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using format!("{:?}", t).contains("DefaultPathFixer")
to detect transformer type is fragile and depends on debug formatting. Consider adding a trait method like transformer_type()
or using std::any::TypeId
for more reliable type checking.
Copilot uses AI. Check for mistakes.
let has_default_fixer = transformers | ||
.iter() | ||
.any(|t| format!("{:?}", t).contains("DefaultPathFixer")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using format!("{:?}", t).contains("DefaultPathFixer")
to detect transformer type is fragile and depends on debug formatting. Consider adding a trait method like transformer_type()
or using std::any::TypeId
for more reliable type checking.
Copilot uses AI. Check for mistakes.
Summary
Details
This PR introduces the DefaultPathFixer transformer to automatically handle path normalization for code coverage reports generated in various CI/CD environments. When users don't specify manual path fixing options (
--path-replace
or--path-prefix
), the system will automatically remove common CI/CD path prefixes to ensure coverage paths match the repository structure.Supported CI/CD Systems
The DefaultPathFixer recognizes and removes path prefixes from:
/home/circleci/
)/home/runner/work/
,/Users/runner/work/
)/home/travis/build/
)/var/lib/jenkins/workspace/
)/builds/
)/opt/atlassian/pipelines/agent/build/
)/codebuild/output/
)/home/vsts/work/
)Changes
Added DefaultPathFixer transformer (
qlty-coverage/src/transformer.rs
)Updated publish planner (
qlty-coverage/src/publish/planner.rs
)Updated transform planner (
qlty-coverage/src/transform/planner.rs
)Test Plan
🤖 Generated with Claude Code