ci(coverage): disable jacoco PR comments, relax overall delta threshold to 0.4%#22
Merged
ci(coverage): disable jacoco PR comments, relax overall delta threshold to 0.4%#22
Conversation
📝 WalkthroughWalkthroughThe workflow now computes "changed-line coverage" using diff-cover (Python 3.11), replaces the previous "changed files" coverage metric with this single changed-line metric for the coverage gate, updates base and PR aggregation to Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Py as Python/Runner
participant JC as JaCoCo
participant DC as diff-cover
participant Gate as Coverage Gate
participant Commenter as Commenter
PR->>GH: Trigger workflow
GH->>Py: Setup Python 3.11
GH->>JC: Aggregate JaCoCo XMLs (base & PR)
JC-->>GH: Produce JaCoCo XMLs
GH->>DC: Run diff-cover against JaCoCo XMLs
DC-->>GH: Emit diff-cover.json (total_percent_covered)
GH->>Gate: Provide CHANGED_LINE, MIN_CHANGED
Gate->>Gate: Validate numeric & compare thresholds
alt meets threshold
Gate-->>Commenter: Post summary comment (comment-type: summary)
else fails threshold
Gate-->>GH: Fail job with changed-line-specific message
end
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.
🧹 Nitpick comments (2)
.github/workflows/pr-build.yml (2)
391-391: Keep the threshold message in sync withMAX_DROP.The hardcoded
0.4%in the failure message will drift ifMAX_DROPis tuned again. Consider interpolating the variable.Proposed fix
- echo "Coverage gate failed: overall coverage dropped more than 0.4%." + echo "Coverage gate failed: overall coverage dropped more than ${MAX_DROP#-}%."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-build.yml at line 391, The failure message currently hardcodes "0.4%" which can go out of sync with the MAX_DROP setting; update the echo that prints the coverage gate failure so it interpolates the MAX_DROP variable (referencing MAX_DROP and the echo command) instead of a literal "0.4%" so the message always reflects the configured threshold.
361-364: Minor: status string reads awkwardly with a negative threshold.With
MAX_DROP=-0.4, the summary renders asPASS (>= -0.4%)/FAIL (< -0.4%), which conflates the delta with the threshold. Consider phrasing in terms of the delta for clarity.Proposed wording
- OVERALL_STATUS="PASS (>= ${MAX_DROP}%)" + OVERALL_STATUS="PASS (delta ${DELTA}% >= ${MAX_DROP}%)" if [ "$DELTA_OK" -ne 1 ]; then - OVERALL_STATUS="FAIL (< ${MAX_DROP}%)" + OVERALL_STATUS="FAIL (delta ${DELTA}% < ${MAX_DROP}%)" 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 361 - 364, The status message uses the threshold variable MAX_DROP in a way that reads awkwardly when it's negative; update the strings set to OVERALL_STATUS to phrase them in terms of the observed delta instead of the threshold. Replace "PASS (>= ${MAX_DROP}%)" and "FAIL (< ${MAX_DROP}%)" with clearer wording such as "PASS (delta >= ${MAX_DROP}%)" and "FAIL (delta < ${MAX_DROP}%)" (or "delta >=/ <" whatever style you prefer), ensuring the branch that checks DELTA_OK continues to set OVERALL_STATUS accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/pr-build.yml:
- Line 391: The failure message currently hardcodes "0.4%" which can go out of
sync with the MAX_DROP setting; update the echo that prints the coverage gate
failure so it interpolates the MAX_DROP variable (referencing MAX_DROP and the
echo command) instead of a literal "0.4%" so the message always reflects the
configured threshold.
- Around line 361-364: The status message uses the threshold variable MAX_DROP
in a way that reads awkwardly when it's negative; update the strings set to
OVERALL_STATUS to phrase them in terms of the observed delta instead of the
threshold. Replace "PASS (>= ${MAX_DROP}%)" and "FAIL (< ${MAX_DROP}%)" with
clearer wording such as "PASS (delta >= ${MAX_DROP}%)" and "FAIL (delta <
${MAX_DROP}%)" (or "delta >=/ <" whatever style you prefer), ensuring the branch
that checks DELTA_OK continues to set OVERALL_STATUS accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ff2d486-cf3d-476b-b392-960144f1732f
📒 Files selected for processing (1)
.github/workflows/pr-build.yml
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.
bladehan1
force-pushed
the
fix/ci_coverage
branch
from
a4fffea to
28f1819
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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)
370-371:⚠️ Potential issue | 🟠 MajorApply the intended
MAX_DROP=-0.4relaxation.The workflow still uses
MAX_DROP=-0.1and the failure message still says0.1%, so the PR objective is not actually implemented.Proposed fix
MIN_CHANGED=60 - MAX_DROP=-0.1 + MAX_DROP=-0.4if [ "$DELTA_OK" -ne 1 ]; then - echo "Coverage gate failed: overall coverage dropped more than 0.1%." + echo "Coverage gate failed: overall coverage dropped more than 0.4%." echo "base=${BASE_OVERALL}% pr=${PR_OVERALL}% delta=${DELTA}%" exit 1 fiAlso applies to: 441-443
🤖 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 370 - 371, Update the workflow to apply the intended relaxation by changing the MAX_DROP value from -0.1 to -0.4 wherever it appears (e.g., the MIN_CHANGED/MAX_DROP assignment block and the later occurrence around the other check), and update any corresponding failure messages that currently say "0.1%" to "0.4%" so they reflect the new threshold; look for the MAX_DROP identifier and the literal "0.1%" in the file and adjust them consistently.
🤖 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 404-408: The comparison currently uses a strict greater-than,
causing values exactly equal to MIN_CHANGED to fail; change the compare_float
invocation(s) to use ">=" instead of ">" where CHANGED_LINE_OK is computed (and
similarly at the other occurrence around CHANGED_LINE_STATUS) so that
CHANGED_LINE == MIN_CHANGED yields PASS; update the calls that produce
CHANGED_LINE_OK (the compare_float expression involving CHANGED_LINE and
MIN_CHANGED) in both places mentioned and keep the CHANGED_LINE_STATUS logic
unchanged.
- Around line 307-321: The workflow captures diff-cover's exit code into DIFF_RC
but continues to parse diff-cover.json; change the logic after running
diff-cover (the diff-cover invocation that sets DIFF_RC) to check if DIFF_RC is
non-zero and immediately fail (exit with DIFF_RC and an error message) before
attempting to read diff-cover.json/CHANGED_LINE_COVERAGE; apply the same fix to
the other diff-cover invocation block (the second diff-cover run that also sets
DIFF_RC) so the job stops on tool/report-generation failures instead of parsing
partial/stale JSON.
---
Outside diff comments:
In @.github/workflows/pr-build.yml:
- Around line 370-371: Update the workflow to apply the intended relaxation by
changing the MAX_DROP value from -0.1 to -0.4 wherever it appears (e.g., the
MIN_CHANGED/MAX_DROP assignment block and the later occurrence around the other
check), and update any corresponding failure messages that currently say "0.1%"
to "0.4%" so they reflect the new threshold; look for the MAX_DROP identifier
and the literal "0.1%" in the file and adjust them consistently.
🪄 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: 335d2e00-737e-4998-99c5-c93a3c048c1e
📒 Files selected for processing (1)
.github/workflows/pr-build.yml
Comment on lines
+307
to
+321
| 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.
Fail when diff-cover exits nonzero.
DIFF_RC is captured but only logged. With --fail-under=0, a nonzero exit should represent a tool/report-generation problem, so the gate should stop instead of parsing potentially stale or partial diff-cover.json.
Proposed fix
DIFF_RC=$?
set -e
+
+ if [ "$DIFF_RC" -ne 0 ]; then
+ echo "diff-cover failed with exit code: ${DIFF_RC}"
+ exit "$DIFF_RC"
+ fi
CHANGED_LINE_COVERAGE=$(jq -r '.total_percent_covered // empty' diff-cover.json)Also applies to: 334-334
🤖 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 307 - 321, The workflow captures
diff-cover's exit code into DIFF_RC but continues to parse diff-cover.json;
change the logic after running diff-cover (the diff-cover invocation that sets
DIFF_RC) to check if DIFF_RC is non-zero and immediately fail (exit with DIFF_RC
and an error message) before attempting to read
diff-cover.json/CHANGED_LINE_COVERAGE; apply the same fix to the other
diff-cover invocation block (the second diff-cover run that also sets DIFF_RC)
so the job stops on tool/report-generation failures instead of parsing
partial/stale JSON.
Comment on lines
+404
to
+408
| CHANGED_LINE_OK=$(compare_float "${CHANGED_LINE} > ${MIN_CHANGED}") | ||
| if [ "$CHANGED_LINE_OK" -eq 1 ]; then | ||
| CHANGED_LINE_STATUS="PASS (> ${MIN_CHANGED}%)" | ||
| else | ||
| CHANGED_LINE_STATUS="FAIL (<= ${MIN_CHANGED}%)" |
There was a problem hiding this comment.
Use inclusive comparison for the minimum changed-line threshold.
MIN_CHANGED=60 reads as “at least 60%,” but > makes exactly 60.0% fail. Use >= unless the strict boundary is intentional.
Proposed fix
- CHANGED_LINE_OK=$(compare_float "${CHANGED_LINE} > ${MIN_CHANGED}")
+ CHANGED_LINE_OK=$(compare_float "${CHANGED_LINE} >= ${MIN_CHANGED}")
if [ "$CHANGED_LINE_OK" -eq 1 ]; then
- CHANGED_LINE_STATUS="PASS (> ${MIN_CHANGED}%)"
+ CHANGED_LINE_STATUS="PASS (>= ${MIN_CHANGED}%)"
else
- CHANGED_LINE_STATUS="FAIL (<= ${MIN_CHANGED}%)"
+ CHANGED_LINE_STATUS="FAIL (< ${MIN_CHANGED}%)"
fi if [ "$CHANGED_LINE_OK" -ne 1 ]; then
- echo "Coverage gate failed: changed-line coverage must be > 60%."
+ echo "Coverage gate failed: changed-line coverage must be >= 60%."
echo "changed-line=${CHANGED_LINE}%"
exit 1
fiAlso applies to: 447-449
🤖 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 404 - 408, The comparison
currently uses a strict greater-than, causing values exactly equal to
MIN_CHANGED to fail; change the compare_float invocation(s) to use ">=" instead
of ">" where CHANGED_LINE_OK is computed (and similarly at the other occurrence
around CHANGED_LINE_STATUS) so that CHANGED_LINE == MIN_CHANGED yields PASS;
update the calls that produce CHANGED_LINE_OK (the compare_float expression
involving CHANGED_LINE and MIN_CHANGED) in both places mentioned and keep the
CHANGED_LINE_STATUS logic unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three targeted fixes to the
coverage-gateCI job inpr-build.yml:comment-type: summaryto bothjacoco-reportsteps — routes output to the workflow step summary instead of PR comments.MAX_DROPfrom-0.1%to-0.4%.Why
Issue 1 — 403 errors on PR comment posting
The
coverage-gatejob declares onlypermissions: contents: read. Themadrapps/jacoco-reportaction defaults tocomment-type: pr_comment, which calls the GitHub PR comment API and requirespull-requests: write. Every CI run was producing a 403 error in the logs, though it did not block the gate because the action's owncontinue-on-errordefault istrue.Setting
comment-type: summarymakes the action write to$GITHUB_STEP_SUMMARY(a local file write, no API call), so the 403 disappears entirely without needing to grant broader token permissions.Issue 2 —
coverage-changed-filesreports INSTRUCTION coverage, not LINE coverageThe value shown as Changed Files Coverage is bytecode instruction coverage, not source-line coverage. This is a fixed behavior of
madrapps/jacoco-report@v1.7.2— the action has nocoverage-typeparameter and internally reads only theINSTRUCTIONcounter from JaCoCo XML.Why INSTRUCTION ≠ LINE and why it matters:
INSTRUCTION,BRANCH,LINE,COMPLEXITY,METHOD,CLASS.if (x != null)may emit 3–5 instructions; a fluent chain on one line may emit 10+.toString, enum boilerplate) inflates the instruction count without adding meaningful semantic coverage.To switch to LINE coverage the action would need to be replaced with a custom script that reads the
LINEcounter (<counter type="LINE" covered="N" missed="M"/>) from each matching<sourcefile>node. The action itself does not expose this as a configuration option in any released version through v1.7.2.Issue 3 —
-0.1%delta threshold causes false failuresNormal variance in test execution timing, minor refactoring (renaming, extracting constants), or adding tests to previously uncovered edge-case paths can shift overall INSTRUCTION coverage by ±0.2%. A threshold of
-0.1%triggered spurious gate failures in these scenarios.-0.4%still catches real coverage regressions while eliminating the noise.Summary by CodeRabbit