fix: correct patch coverage calculation and baseline artifact lookup#56
fix: correct patch coverage calculation and baseline artifact lookup#56MathurAditya724 merged 2 commits intomainfrom
Conversation
- Add suffix-based path matching in PatchAnalyzer to handle absolute vs relative path mismatches (e.g. cargo llvm-cov emits absolute paths while PR diffs use repo-relative paths) - Change artifact lookup from status:'success' to status:'completed' with conclusion filtering (success/failure), so base coverage artifacts from workflow runs that failed due to unrelated jobs are still found - Default filesMode to 'changed' so PR comments only show files touched in the PR rather than all project files with uncovered lines - Add bidirectional suffix matching in report-formatter file filter for consistency with the patch analyzer - Add clear warning when no diff files match coverage data (0/0 = 100%) - Replace O(n) line lookups with Map-based O(1) lookups in patch analyzer - Add normalized equality check between exact match and suffix match Fixes: getsentry/codecov-action reporting 100% patch coverage when coverage tools emit absolute file paths (observed in pydantic/monty#288)
c29fa12 to
d042d2e
Compare
Codecov Results 📊✅ 179 passed | Total: 179 | Pass Rate: 100% | Execution Time: 239ms 📊 Comparison with Base Branch
All tests are passing successfully. ❌ Patch coverage is 65.00%. Project has 757 uncovered lines. Files with missing lines (5)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 55.46% 55.94% +0.48%
==========================================
Files 24 24 —
Lines 1679 1713 +34
Branches 1208 1232 +24
==========================================
+ Hits 929 956 +27
- Misses 750 757 +7
- Partials 95 97 +2Generated by Codecov Action |
…ranch On feature branch pushes (non-PR), patchCoverage is null so changedFiles is empty, causing the file table to be hidden entirely. The previous check only fell back to 'all' when currentBranch === baseBranch, but feature branches also lack PR context and need the same fallback.
| normalizedCoverage.endsWith(`/${normalizedDiff}`) || | ||
| normalizedDiff.endsWith(`/${normalizedCoverage}`) |
There was a problem hiding this comment.
Bug: The endsWith() check for matching coverage files can cause false positives by incorrectly matching files that share a common path suffix in different directories.
Severity: MEDIUM
Suggested Fix
The matching logic should be made more robust. Instead of a simple endsWith() check, consider a more precise matching algorithm that verifies the full path segments. For example, ensure that the character preceding the matched suffix is a path separator ('/') or that the paths are identical.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/analyzers/patch-analyzer.ts#L75-L76
Potential issue: The patch coverage calculation in `findCoverageFile()` uses a simple
`endsWith()` check to match changed files with files in the coverage report. This can
lead to incorrect matches in monorepos where different files share a common path suffix.
For example, a changed file `lib/file.ts` could be incorrectly associated with coverage
data from `src/lib/file.ts` because `normalizedCoverage.endsWith('/' + normalizedDiff)`
evaluates to true. This results in inaccurate patch coverage calculations by attributing
coverage to the wrong file.
Summary
Fixes incorrect patch coverage reporting (100% instead of actual value) caused by two independent bugs, plus improves default behavior for PR comments.
Context: Observed in pydantic/monty#288 where the codecov-action reported
Patch coverage is 100.00%while the official Codecov bot correctly reported61.01%.Root Causes & Fixes
1. Path mismatch in patch coverage calculation (primary fix)
Coverage tools like
cargo llvm-covemit absolute paths (e.g.,/home/runner/work/monty/monty/crates/monty/src/args.rs) while PR diffs use repo-relative paths (crates/monty/src/args.rs). The patch analyzer only did exactMap.get()lookups, so no files ever matched, resulting in0/0 = 100%.Fix: Added a 3-tier path resolution strategy in
PatchAnalyzer:./src/file.tsvssrc/file.ts)Results are cached per diff path for performance. Also replaced O(n)
Array.find()line lookups with O(1)Map.get().2. Baseline artifact lookup too restrictive
The artifact download filtered workflow runs by
status: "success", but in repos like pydantic/monty, the overall workflow conclusion is"failure"due to unrelated jobs failing — even when the job that uploads the coverage artifact succeeded.Fix: Changed to
status: "completed"with post-filter onconclusionto accept"success"and"failure"(excluding"cancelled","timed_out", etc. which may have incomplete artifacts).3. Default
filesModechanged from"all"to"changed"PR comments were showing all 89 project files with uncovered lines instead of just the 4 changed files. Now defaults to showing only changed files unless explicitly overridden.
4. Clear warning on path mismatch
When no diff files match coverage data (0/0 case), a
core.warning()is now emitted with sample paths from both sides to help users diagnose the issue.5. Bidirectional suffix matching in report-formatter
The file list filter in the report formatter now checks suffix matching in both directions (coverage→diff and diff→coverage) for consistency with the patch analyzer.
Files Changed
src/analyzers/patch-analyzer.tssrc/utils/artifact-manager.tsstatus: "completed"+ conclusion filtersrc/config/config-loader.tsfilesMode→"changed"src/formatters/report-formatter.tsfilesMode→"changed", bidirectional suffix matchsrc/index.tseffectiveFilesModelogicsrc/__tests__/patch-analyzer.test.tssrc/__tests__/config-loader.test.tssrc/__tests__/report-formatter.test.tsTesting
filesModebehavior