Skip to content

fix(ci): gate changed-line coverage with diff-cover#6705

Closed
bladehan1 wants to merge 4 commits intotronprotocol:developfrom
bladehan1:fix/ci_coverage_line_gate
Closed

fix(ci): gate changed-line coverage with diff-cover#6705
bladehan1 wants to merge 4 commits intotronprotocol:developfrom
bladehan1:fix/ci_coverage_line_gate

Conversation

@bladehan1
Copy link
Copy Markdown
Collaborator

@bladehan1 bladehan1 commented Apr 24, 2026

Summary

Replace the coverage-changed-files gate (whole-file INSTRUCTION coverage) with a true changed-line LINE coverage gate powered by diff-cover, and make the gate robust against non-Java PRs and tool failures.

Commits included

Subject Purpose
ci(coverage): gate changed-line coverage Introduce diff-cover as the changed-code gate; switch madrapps/jacoco-report to summary-only to eliminate 403 errors on fork PRs.
ci(coverage): harden diff-cover step and handle non-Java PRs Drop an unreachable branch; add an NA sentinel so PRs with no src/main/java changes are reported as SKIPPED instead of failing.
ci(coverage): polish diff-cover step and clarify metric semantics Guard empty SRC_ROOTS; verify diff-cover actually produced its JSON; drop a no-signal log line; add a footnote clarifying the LINE vs INSTRUCTION mix in the metrics panel.

Net change: 1 file, +90 / -27.

Why

  1. Patch coverage semantics. The previous gate used madrapps/jacoco-report's coverage-changed-files output, which is the whole-file INSTRUCTION coverage of each changed file — not the coverage of the lines the PR actually changed. diff-cover provides the industry-standard "changed-line" measurement.
  2. Drop the 403 noise. jacoco-report's default PR comment needs pull-requests: write, which fork PRs do not get. Job logs kept filling up with 403 errors.
  3. Stop wrongly failing non-Java PRs. With just diff-cover, a PR that only touches workflows / docs / proto / test code produces total_num_lines=0, and the original jq call exited with 1. The NA sentinel now maps that state to SKIPPED in the enforce step.

Design notes for reviewers

  • CI wall-clock impact is ~0. The new coverage-base job runs independently of docker-build-debian11 (they share no needs:). Since docker-build-debian11 is already the slowest build (~16 min with testWithRocksDb), adding a parallel coverage-base of similar length does not extend the critical path. The gate job waits for both, so total PR wall-clock is unchanged.
  • Metric mix is intentional and footnoted. Changed-line Coverage uses LINE coverage (diff-cover), while PR Overall Coverage / Base Overall Coverage / Delta use INSTRUCTION coverage. madrapps/jacoco-report v1.7.2 is the latest release and hard-codes the coverage-overall output to INSTRUCTION; there is no coverage-type input yet — see the open madrapps/jacoco-report#82 on milestone 1.8. The METRICS panel and step summary now print an explicit note so readers do not accidentally compare the two counters.
  • SKIPPED scope. diff-cover --src-roots is limited to */src/main/java. PRs touching only src/test/java, build/generated (proto output), docs, or CI will be reported as SKIPPED. In practice proto-only PRs with non-trivial generated-code coverage shifts are rare in this repository; we accept the gap.
  • No unit tests for the CI script itself. The bash in pr-build.yml is exercised end-to-end on every PR via its own CI run rather than through a shell unit-test framework. The NA / SKIPPED path and the happy PASS path have both been verified end-to-end on prior fork PRs before this submission. Adding bats or similar is possible as a follow-up if desired, but is out of scope here.
  • Gate thresholds preserved. MIN_CHANGED=60 (for Changed-line LINE) and MAX_DROP=-0.1% (for Overall INSTRUCTION delta) are unchanged from the existing workflow.
  • Failure modes. The new exit 1 branches only fire in genuine error states: SRC_ROOTS is empty (unexpected workspace shape) or diff-cover.json is missing (tool crash). Neither can be triggered by ordinary PR content.

Follow-ups (not in this PR)

  • Migrate Overall to LINE once madrapps/jacoco-report 1.8 ships coverage-type, or by self-parsing the JaCoCo XML. Unifies the metrics panel on one counter.
  • Add proper shell unit tests (e.g. via bats) for the enforce step, so the three-state logic (NA / numeric / error) is covered without needing a real CI run.
  • Revisit comment-type: summary if reviewers prefer on-PR comment visibility; restoring comments requires granting pull-requests: write on the coverage-gate job.

Test plan

  • Workflow-only / non-Java PR → Changed-line Coverage: NA, Changed-line Gate: SKIPPED, gate passes. Verified end-to-end on a prior fork PR carrying the same commits.
  • Java PR with changed lines → Changed-line Coverage: X%, Changed-line Gate: PASS at X > 60. Verified end-to-end on a prior fork PR (25 changed lines @ 100%).
  • Java PR with changed lines at X <= 60FAIL. Logic reviewed; path not force-exercised.
  • First natural Java PR after merge → regress PASS / FAIL / mixed-change paths.

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.
- Drop the unreachable PR_XMLS empty-check; collect-xml already exits 1
  when jacoco reports are missing, and the previous `exit 0` would have
  left changed_line_coverage unset and triggered a misleading
  "Failed to parse changed-line coverage" in the enforcement step.
- Parse total_num_lines from diff-cover.json: when a PR has no changed
  Java source lines (workflow-only, docs-only, proto-only), emit the
  sentinel NA and treat it as SKIPPED in the enforcement step so the
  gate does not wrongly fail.
- Display NA without a trailing %% in the step summary and logs.

Addresses review comments from CodeRabbit and cubic on PR #23.
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant