ci: drop stale coverage objects before the Rust workspace test step - #4751
QuantumExplorer wants to merge 1 commit into
Conversation
cargo-llvm-cov builds the report from every executable it finds under target/llvm-cov-target/debug whose name matches a workspace package or target, not from the binaries the current run built. Because the test step runs with --no-report (which implies --no-clean) nothing ever removes the test binaries that a version bump, Cargo.lock change or toolchain bump re-keyed under a new hash, so they keep being read as coverage objects. Their regions describe the tree they were built from and carry no counts, so files that changed since pick up misses on comment, attribute and blank lines and lose hits on signature lines. That made the `rust` codecov flag swing between ~74% and ~84% depending on what a runner's retained target/ happened to hold (the same mac went 74.87% -> 83.74% within hours after the disk guard wiped it), and failed codecov/project on PRs that never touched the affected crates. It is a property of the retained build, not of the runner OS. Remove the executables (test binaries, uplifted bins, examples and workspace cdylibs) before the test step, using the same name filter cargo-llvm-cov applies, so the report can only see this run's objects. The workspace rlibs and the dependency cache stay, so only the workspace's own test, bin and example targets recompile. Also drop target/lcov-tree-hash together with the lcov files: a PR run regenerates the reports without rewriting the marker, so a later run of the marker's tree could have reused another tree's coverage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe CI workflow now removes stale coverage markers and workspace executables before collecting coverage. Comments describe when pruning occurs and how retained executables are handled between runs. ChangesCoverage cleanup
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The coverage cleanup is scoped to stale executable outputs and does not leave an established failure path for the subsequent CI test and reporting steps. It is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 02984cb) · triage: low · Phase 2 only (queue backlog) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.3-dev #4751 +/- ##
============================================
+ Coverage 83.05% 83.41% +0.36%
============================================
Files 2843 2846 +3
Lines 390754 390452 -302
============================================
+ Hits 324523 325685 +1162
+ Misses 66231 64767 -1464
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The workflow change correctly removes stale workspace coverage executables while preserving dependency artifacts, and it invalidates the retained lcov tree marker whenever fresh coverage is required. The cleanup is gated consistently with the test path and uses metadata-derived workspace/package target names, so I found no in-scope correctness or architecture issues.
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — This is a contained CI workflow change that removes stale coverage executables and marker data, with no impact on production behavior or critical surfaces, and its shell logic is straightforward to validate. - Phase 1 reviewers: not run (skipped for throughput: 25 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer
2 participants
Issue being fixed or feature implemented
codecov/projectfails on PRs whoserustcoverage drops several percent in crates the PR never touched (seen on #4748: base 83.52% vs head 75.95%, with dpp at -6.74%, drive -3.73%, drive-proof-verifier -5.58%, none of them changed). The head report had ~30k more instrumented lines with fewer hits than the base for the same test set, and the extra lines were comments, attributes, struct fields and blank lines marked as misses, with function signature lines marked as misses while their bodies were hits.The cause is the retained coverage build, not the runner OS.
cargo llvm-cov reportcollects its objects by walkingtarget/llvm-cov-target/debugand passing every executable whose name matches a workspace package or target to llvm-cov (report.rsobject_files). The test step runscargo llvm-cov nextest --no-report, and--no-reportimplies--no-clean, so thecargo clean -p <members>cargo-llvm-cov would otherwise run first is skipped. Every version bump, Cargo.lock change or toolchain bump re-keys all test binaries under a new hash and leaves the old ones indeps/, where they are read forever. Those stale binaries carry coverage regions from the tree they were built from and have no counts in the current profile; llvm-cov keeps the first record it meets per function, so files that changed since pick up misses (and lose hits) unrelated to the code under test.The per-flag totals of recent
v4.2-devpushes track each runner's leftovers, not its OS:target/at job startrustflagBoth #4748 runs used rustc 1.98.1; the cargo-llvm-cov versions differed (0.9.0 vs 0.8.7) but the changelog between them has nothing relevant, and both versions walk the directory the same way. Per-OS codecov flags or pinning the job to one OS would not have fixed this.
What was done?
tests-rs-workspace.yml, right before the test step and gated like the other "tests will run" steps. It rebuilds cargo-llvm-cov's own name filter fromcargo metadata --no-depsand deletes the matching executables (test binaries, uplifted bins, examples, workspace cdylibs) indebug/,debug/depsanddebug/examples. Rlibs, rmeta, dep-info and the ~700 dependency crates stay, so only the workspace's own test, bin and example targets recompile and the retained build keeps its value.target/lcov-tree-hash. A PR run regenerates the lcov files but never rewrites the marker, so a later run of the marker's tree could have reused another tree's report through the tree-hash shortcut.runs-oncomment is unchanged and still accurate..codecov.ymlis untouched; the patch status target is not weakened.How Has This Been Tested?
actionlintand a YAML parse of the workflow pass.cargo test --no-runrebuilt only the two current binaries and the stale hashes did not come back. The missing-directory path exits 0 with a message.invoked.timestampfiles on fresh builds, so there is no timestamp-based way to keep only the current binaries; dropping all workspace executables is the simplest correct option. A zero-rebuild variant (cargo llvm-cov show-envpluscargo nextest list --message-format jsonto keep only current binaries) is possible if the recompile cost matters.target/(ubuntu-runner-1 is the best candidate) the new step should log several thousand dropped objects and therustflag should land near 83.5%. Expect the test step's build phase to grow by roughly one workspace test-target compile; it was 1 min 08 s on both runners before this change.Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit