test: validate changed-line coverage gate#23
Conversation
Use diff-cover changed-line coverage as the changed-code gate, keep the overall coverage delta gate at -0.1%, and keep Jacoco reports as summary-only snapshots.
📝 WalkthroughWalkthroughThe pull request enhances the PR coverage validation by introducing a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/pr-build.yml (2)
376-409: Redundant guards aroundCHANGED_LINEvalidation.
is_number's regex already rejects empty strings andNaN, so the[ -z … ]and= "NaN"tests on line 399 are redundant. Collapsing to a singleis_numbercheck keeps the validation consistent with the base/pr overall checks on line 390.♻️ Minor simplification
- if [ -z "$CHANGED_LINE" ] || [ "$CHANGED_LINE" = "NaN" ] || ! is_number "$CHANGED_LINE"; then + if ! is_number "$CHANGED_LINE"; then echo "Failed to parse changed-line coverage: changed-line='${CHANGED_LINE}'." 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 376 - 409, The CHANGED_LINE validation contains redundant checks ([ -z "$CHANGED_LINE" ] and = "NaN") because is_number() already rejects empty strings and NaN; update the validation to mirror the BASE_OVERALL/PR_OVERALL block by removing those extra guards and only calling is_number "$CHANGED_LINE" (and keep the same error message/exit behavior), so change the block that sets CHANGED_LINE_OK to first ensure is_number returns true for CHANGED_LINE and then call compare_float and set CHANGED_LINE_STATUS accordingly.
287-315: Recommended cleanup: reuse prior outputs and quote expansions.Small hygiene improvements that reduce duplication and make the step robust to unusual path characters:
PR_XMLSis already computed bycollect-xml— reusesteps.collect-xml.outputs.pr_xmls(comma-separated) rather than re-runningfind. If you keepfind, switch to-print0/xargs -0ormapfileto handle whitespace in paths safely.SRC_ROOTSis similarly expanded unquoted; safer to build an array.- The explicit
git fetchis fine as defense-in-depth even withfetch-depth: 0, but a brief comment noting that would help future readers.🤖 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 287 - 315, Replace the ad-hoc re-find of PR XMLs and unquoted path expansions with the previously computed step output and safe array handling: use the workflow output steps.collect-xml.outputs.pr_xmls (split on commas into a shell array) instead of re-running find for PR_XMLS, build SRC_ROOTS as a quoted array (avoid word-splitting) or use find -print0 / mapfile to populate it safely, and pass those quoted/array values into the diff-cover invocation to handle whitespace in paths; keep the existing git fetch but add a short comment noting it complements fetch-depth: 0 for robustness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-build.yml:
- Around line 298-302: The branch that checks PR_XMLS and exits 0 is misleading
or unreachable because the earlier collect-xml step already exits 1 when no
reports exist; remove this block or change it to set a sentinel output (e.g.,
set the changed_line_coverage output to "N/A" or "skipped") instead of exiting,
so the enforcement step can detect the sentinel and honestly skip parsing;
update the PR_XMLS check in the workflow to either be deleted or to assign
changed_line_coverage="N/A" (and document that enforcement should treat that
sentinel as skip) — refer to the PR_XMLS variable, the collect-xml step
behavior, and the changed_line_coverage/enforcement parsing logic to ensure
consistency.
- Around line 298-321: The diff-cover run can produce a report with no changed
lines (total_num_lines == 0) which makes jq return empty for
.total_percent_covered and causes an erroneous exit; update the workflow to
parse both .total_num_lines and .total_percent_covered from diff-cover.json (use
CHANGED_LINE_COUNT=$(jq -r '.total_num_lines // 0' diff-cover.json) and
CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered // empty'
diff-cover.json)), and if CHANGED_LINE_COUNT is 0 set CHANGED_LINE_COVERAGE="NA"
(or another sentinel) instead of exiting; also update the gate-enforcement logic
that consumes CHANGED_LINE_COVERAGE to treat "NA" as a pass rather than a
failure so workflow-only/non-Java PRs do not fail the gate.
---
Nitpick comments:
In @.github/workflows/pr-build.yml:
- Around line 376-409: The CHANGED_LINE validation contains redundant checks ([
-z "$CHANGED_LINE" ] and = "NaN") because is_number() already rejects empty
strings and NaN; update the validation to mirror the BASE_OVERALL/PR_OVERALL
block by removing those extra guards and only calling is_number "$CHANGED_LINE"
(and keep the same error message/exit behavior), so change the block that sets
CHANGED_LINE_OK to first ensure is_number returns true for CHANGED_LINE and then
call compare_float and set CHANGED_LINE_STATUS accordingly.
- Around line 287-315: Replace the ad-hoc re-find of PR XMLs and unquoted path
expansions with the previously computed step output and safe array handling: use
the workflow output steps.collect-xml.outputs.pr_xmls (split on commas into a
shell array) instead of re-running find for PR_XMLS, build SRC_ROOTS as a quoted
array (avoid word-splitting) or use find -print0 / mapfile to populate it
safely, and pass those quoted/array values into the diff-cover invocation to
handle whitespace in paths; keep the existing git fetch but add a short comment
noting it complements fetch-depth: 0 for robustness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3839bedf-7612-47f8-81f6-43ed4715e552
📒 Files selected for processing (1)
.github/workflows/pr-build.yml
| PR_XMLS=$(find coverage/pr -name "jacocoTestReport.xml" | sort) | ||
| if [ -z "$PR_XMLS" ]; then | ||
| echo "No PR jacoco XML reports found, skipping diff-cover." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Misleading "skip" branch — either dead or causes a later hard failure.
The earlier collect-xml step already exit 1s when PR XMLs are missing (lines 275-278), so this branch is unreachable on the normal path. Worse, if it ever did execute, exit 0 bypasses setting changed_line_coverage, and the enforcement step will then fail at lines 399-402 with a confusing "Failed to parse changed-line coverage" message rather than the "skipping diff-cover" intent suggested here.
Consider removing the block, or making it honestly skip by setting a sentinel output that the enforcement step treats as N/A.
🤖 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 298 - 302, The branch that
checks PR_XMLS and exits 0 is misleading or unreachable because the earlier
collect-xml step already exits 1 when no reports exist; remove this block or
change it to set a sentinel output (e.g., set the changed_line_coverage output
to "N/A" or "skipped") instead of exiting, so the enforcement step can detect
the sentinel and honestly skip parsing; update the PR_XMLS check in the workflow
to either be deleted or to assign changed_line_coverage="N/A" (and document that
enforcement should treat that sentinel as skip) — refer to the PR_XMLS variable,
the collect-xml step behavior, and the changed_line_coverage/enforcement parsing
logic to ensure consistency.
| PR_XMLS=$(find coverage/pr -name "jacocoTestReport.xml" | sort) | ||
| if [ -z "$PR_XMLS" ]; then | ||
| echo "No PR jacoco XML reports found, skipping diff-cover." | ||
| exit 0 | ||
| fi | ||
|
|
||
| SRC_ROOTS=$(find . -type d -path '*/src/main/java' \ | ||
| -not -path './coverage/*' -not -path './.git/*' | sort) | ||
|
|
||
| set +e | ||
| diff-cover $PR_XMLS \ | ||
| --compare-branch="origin/${BASE_REF}" \ | ||
| --src-roots $SRC_ROOTS \ | ||
| --fail-under=0 \ | ||
| --json-report=diff-cover.json \ | ||
| --markdown-report=diff-cover.md | ||
| DIFF_RC=$? | ||
| set -e | ||
|
|
||
| CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered // empty' diff-cover.json) | ||
| if [ -z "$CHANGED_LINE_COVERAGE" ]; then | ||
| echo "Unable to parse changed-line coverage from diff-cover.json." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
diff-cover 9.2.0 json report total_percent_covered total_num_lines when no changed lines in src roots
💡 Result:
When there are no changed lines in the source roots specified by --src-roots in diff-cover 9.2.0 (released 2024-09-08), the tool detects no relevant diff lines within those roots. The JSON report will have "total_num_lines": 0 and "total_percent_covered": null (or omitted, as percentage is undefined for zero lines). The "src_stats" object will be empty {}, "total_num_violations": 0, with metadata like "report_name": "XML" and "diff_name" describing the comparison (e.g., "origin/master...HEAD, staged and unstaged changes"). This follows from diff-cover's design to report only on changed lines in the git diff that match coverage data via src-roots for JaCoCo XML reports. If no diff lines match, totals reflect zero lines analyzed. Console output would show "No lines with coverage information in this diff" or similar, but JSON captures the empty stats.
Citations:
- 1: diff cover json report don't contain the total lines' number Bachmann1234/diff_cover#170
- 2: https://github.com/Bachmann1234/diff-cover
- 3: No line coverage instead of 0% Bachmann1234/diff_cover#132
- 4: No lines with coverage information in this diff when with multiple sub_project Bachmann1234/diff_cover#114
- 5: https://manpages.ubuntu.com/manpages/noble/man1/diff-cover.1.html
Gate fails for PRs with no changed Java source lines.
When a PR touches only non-Java files (or no src/main/java source at all, like a workflow-only change), diff-cover finds no covered/uncovered lines in the configured source roots. The resulting JSON report has total_percent_covered: null (or omitted) and total_num_lines: 0.
Current code (line 320) treats this as a parsing failure:
CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered // empty' diff-cover.json)
if [ -z "$CHANGED_LINE_COVERAGE" ]; then
echo "Unable to parse changed-line coverage from diff-cover.json."
exit 1
fi
Since total_percent_covered is null, jq yields empty, causing exit 1. This blocks legitimate PRs that don't modify production Java code (e.g., this PR itself, which only edits the workflow file).
Fix: Detect the "no changed lines" case and short-circuit to pass the changed-line gate:
Suggested change
- CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered // empty' diff-cover.json)
- if [ -z "$CHANGED_LINE_COVERAGE" ]; then
- echo "Unable to parse changed-line coverage from diff-cover.json."
- exit 1
- fi
- echo "changed_line_coverage=${CHANGED_LINE_COVERAGE}" >> "$GITHUB_OUTPUT"
+ TOTAL_NUM_LINES=$(jq -r '.total_num_lines // 0' diff-cover.json)
+ if [ "${TOTAL_NUM_LINES}" = "0" ]; then
+ echo "No changed Java source lines detected; skipping changed-line gate."
+ echo "changed_line_coverage=NA" >> "$GITHUB_OUTPUT"
+ else
+ CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered // empty' diff-cover.json)
+ if [ -z "$CHANGED_LINE_COVERAGE" ]; then
+ echo "Unable to parse changed-line coverage from diff-cover.json."
+ exit 1
+ fi
+ echo "changed_line_coverage=${CHANGED_LINE_COVERAGE}" >> "$GITHUB_OUTPUT"
+ fiThe gate-enforcement step (line 404) would also need to treat NA as a pass rather than fail.
🤖 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 298 - 321, The diff-cover run
can produce a report with no changed lines (total_num_lines == 0) which makes jq
return empty for .total_percent_covered and causes an erroneous exit; update the
workflow to parse both .total_num_lines and .total_percent_covered from
diff-cover.json (use CHANGED_LINE_COUNT=$(jq -r '.total_num_lines // 0'
diff-cover.json) and CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered //
empty' diff-cover.json)), and if CHANGED_LINE_COUNT is 0 set
CHANGED_LINE_COVERAGE="NA" (or another sentinel) instead of exiting; also update
the gate-enforcement logic that consumes CHANGED_LINE_COVERAGE to treat "NA" as
a pass rather than a failure so workflow-only/non-Java PRs do not fail the gate.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/pr-build.yml">
<violation number="1" location=".github/workflows/pr-build.yml:301">
P2: The `exit 0` here skips setting the `changed_line_coverage` output. If this branch ever executes, the downstream enforcement step will see an empty `CHANGED_LINE_RAW` and fail with "Failed to parse changed-line coverage" — a confusing error that masks the real cause. Either remove this unreachable block or set a sentinel output (e.g., `changed_line_coverage=NA`) and handle it in the enforcement step.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| PR_XMLS=$(find coverage/pr -name "jacocoTestReport.xml" | sort) | ||
| if [ -z "$PR_XMLS" ]; then | ||
| echo "No PR jacoco XML reports found, skipping diff-cover." | ||
| exit 0 |
There was a problem hiding this comment.
P2: The exit 0 here skips setting the changed_line_coverage output. If this branch ever executes, the downstream enforcement step will see an empty CHANGED_LINE_RAW and fail with "Failed to parse changed-line coverage" — a confusing error that masks the real cause. Either remove this unreachable block or set a sentinel output (e.g., changed_line_coverage=NA) and handle it in the enforcement step.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/pr-build.yml, line 301:
<comment>The `exit 0` here skips setting the `changed_line_coverage` output. If this branch ever executes, the downstream enforcement step will see an empty `CHANGED_LINE_RAW` and fail with "Failed to parse changed-line coverage" — a confusing error that masks the real cause. Either remove this unreachable block or set a sentinel output (e.g., `changed_line_coverage=NA`) and handle it in the enforcement step.</comment>
<file context>
@@ -279,6 +279,60 @@ jobs:
+ PR_XMLS=$(find coverage/pr -name "jacocoTestReport.xml" | sort)
+ if [ -z "$PR_XMLS" ]; then
+ echo "No PR jacoco XML reports found, skipping diff-cover."
+ exit 0
+ fi
+
</file context>
2 participants
Test PR for validating that PR Build uses diff-cover changed-line coverage as the changed-code gate.\n\nExpected workflow signals:\n- Changed-line coverage (diff-cover) step runs\n- diff-cover JSON output is parsed into changed_line_coverage\n- Coverage Gate summary shows Changed-line Gate\n- old changed-files gate is not used
Summary by CodeRabbit
Summary by cubic
Switch PR Build to use
diff-coverchanged-line coverage as the changed-code gate. Keeps the overall coverage delta gate (-0.1%) and posts JaCoCo reports as summary-only.Refactors
diff-coveragainst PR JaCoCo XMLs and compare to the base branch; add a "Changed-line Coverage" section to the step summary.changed_line_coveragefor gating and logs.madrapps/jacoco-report.Dependencies
diff-cover@9.2.0(viaactions/setup-python@v5with Python 3.11).Written for commit 28f1819. Summary will update on new commits.