fix(ci): gate changed-line coverage with diff-cover#26
Conversation
- Guard against empty SRC_ROOTS before invoking diff-cover; exits with a readable error instead of letting --fail-under be consumed as a src-root value. - Verify that diff-cover actually wrote diff-cover.json; surface the exit code in the failure message so a runner/binary crash is diagnosable without hunting through logs. - Drop the trailing "diff-cover exit code" log; --fail-under=0 means DIFF_RC is always 0 in the happy path, so the line carries no signal. - Clarify the metrics panel: Changed-line is LINE coverage (diff-cover) while Overall / Delta are INSTRUCTION coverage (jacoco-report). Add a footnote to both the console output and the step summary so readers do not silently compare the two counters.
📝 WalkthroughWalkthroughThe PR-build workflow is hardened to fail early if Java sources are absent and to validate that Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
bladehan1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-build.yml (1)
306-320:⚠️ Potential issue | 🟡 MinorConsider also failing on non-zero
DIFF_RC, not just missing JSON.With
--fail-under=0(line 310),diff-coverwill not exit non-zero on low coverage, so a non-zeroDIFF_RCindicates a real tool error (bad args, git/ref issue, etc.). Today, ifdiff-covererrors but still writes a (possibly partial)diff-cover.json, the step continues past line 319 and downstreamjqparses whatever landed on disk — masking the failure. The capturedDIFF_RCis only surfaced in the "missing JSON" branch.Suggest gating on both conditions so tool errors are never silently absorbed:
🛠️ Proposed fix
- if [ ! -f diff-cover.json ]; then - echo "diff-cover did not produce JSON report (exit=${DIFF_RC})." >&2 - exit 1 - fi + if [ "${DIFF_RC}" -ne 0 ] || [ ! -f diff-cover.json ]; then + echo "diff-cover failed (exit=${DIFF_RC}, json_present=$( [ -f diff-cover.json ] && echo yes || echo no ))." >&2 + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-build.yml around lines 306 - 320, The script currently only fails when diff-cover.json is missing but ignores a non-zero DIFF_RC from running diff-cover; update the post-run check to also fail when DIFF_RC is non-zero. After running the diff-cover command (the block that sets DIFF_RC from diff-cover $PR_XMLS ...), test if DIFF_RC != 0 OR if diff-cover.json is missing and exit with an error message that includes the DIFF_RC value; reference the existing DIFF_RC variable, the diff-cover invocation, and the diff-cover.json filename when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/pr-build.yml:
- Around line 306-320: The script currently only fails when diff-cover.json is
missing but ignores a non-zero DIFF_RC from running diff-cover; update the
post-run check to also fail when DIFF_RC is non-zero. After running the
diff-cover command (the block that sets DIFF_RC from diff-cover $PR_XMLS ...),
test if DIFF_RC != 0 OR if diff-cover.json is missing and exit with an error
message that includes the DIFF_RC value; reference the existing DIFF_RC
variable, the diff-cover invocation, and the diff-cover.json filename when
making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b0dce10-777c-4619-9bbd-a2131bbd213d
📒 Files selected for processing (1)
.github/workflows/pr-build.yml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
Replace the upstream
coverage-changed-filesgate (whole-file INSTRUCTION coverage) with a true changed-line LINE coverage gate powered bydiff-cover, plus several robustness fixes uncovered during self-review.Commits included
28f1819cf9diff-coveras the changed-code gate, switch jacoco-report to summary-only to eliminate 403s on fork PRsece6e0930fNAsentinel for PRs with nosrc/main/javachanges so the gate does not wrongly fail37248f0bc3SRC_ROOTS, verify diff-cover produced its JSON, drop a no-signal log line, add a footnote clarifying the LINE vs INSTRUCTION mixNet change: 1 file, +90 / -27.
Why
madrapps/jacoco-report'scoverage-changed-filesoutput, which is the whole-file INSTRUCTION coverage of each changed file — not the coverage of the lines the PR actually changed.diff-coverprovides the industry-standard "changed-line" measurement.jacoco-report's default PR comment needspull-requests: write, which fork PRs do not get. The log kept filling up with 403 errors.comment-type: summarysidesteps that.diff-cover, a PR that only touches workflows / docs / proto / test code producestotal_num_lines=0and the originaljqcall exited with 1. TheNAsentinel now maps that state toSKIPPEDin the enforce step.Design notes for reviewers
coverage-basejob runs independently ofdocker-build-debian11(they share noneeds:). Sincedocker-build-debian11was already the slowest build (~16 min withtestWithRocksDb), adding a parallel 16-mincoverage-basedoes not extend the critical path. The gate job waits for both, so total PR wall-clock is unchanged.Changed-line Coverageuses LINE coverage (diff-cover), whilePR Overall Coverage/Base Overall Coverage/Deltause INSTRUCTION coverage (jacoco-report v1.7.2 hard-codes this; there is nocoverage-typeinput yet — see the openmadrapps/jacoco-report#82on milestone1.8). The METRICS panel and step summary now print an explicit note so reviewers do not accidentally compare the two counters.diff-cover --src-rootsis limited to*/src/main/java. PRs touching onlysrc/test/java,build/generated(proto output), docs, or CI will be reported asSKIPPED. Proto-only PRs that cause non-trivial generated-code changes are rare in this repo; we accept the gap.pr-build.ymlis exercised end-to-end by every PR's own CI run rather than via a shell unit-test framework. This PR itself relies on theNA / SKIPPEDpath being observed on its own Coverage Gate run (proven previously on PR ci(coverage): harden diff-cover step and handle non-Java PRs #25 of the forked repo). Addingbatsor similar as a follow-up is possible if the team wants it, but it is out of scope here.MIN_CHANGED=60(for Changed-line LINE) andMAX_DROP=-0.1%(for Overall INSTRUCTION delta) are kept as in the previous workflow.Follow-ups(not in this PR)
Overallto LINE oncemadrapps/jacoco-report1.8 shipscoverage-type, or switch to self-parsing the JaCoCo XML. That unifies the metrics panel on one counter.bats) for the enforce step, so the three-state logic (NA/ numeric / error) is covered without needing a real CI run.comment-type: summaryif upstream prefers on-PR comment visibility; restoring it requires grantingpull-requests: writeon thecoverage-gatejob.Test plan
Changed-line Coverage: NA,Changed-line Gate: SKIPPED, gate passes. Verified on PR ci(coverage): harden diff-cover step and handle non-Java PRs #25 of the forked repo (sameece6e0930fcommit): seeCoverage Gatejob summary.Changed-line Coverage: X%,Changed-line Gate: PASSatX > 60. Verified on PR test: PR12 unit-test coverage on external account #24 of the forked repo (25 lines @ 100%, both files under test).X <= 60→FAIL. Logic reviewed; path not forced in CI yet.Summary by CodeRabbit
Summary by cubic
Switch the PR coverage gate to changed-line LINE coverage using
diff-cover. Adds guardrails and clearer metrics, skips non-Java PRs, and avoids fork PR permission noise.New Features
diff-cover(LINE coverage).madrapps/jacoco-report(INSTRUCTION) and add a note to avoid mixing metrics.comment-type: summaryto avoid 403s on fork PRs.Bug Fixes
src/main/javachanges; reportNAand mark as SKIPPED.SRC_ROOTSbefore runningdiff-cover.diff-cover.jsonexists; fail early with a clear message.Written for commit 37248f0. Summary will update on new commits.