Skip to content

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

Open
bladehan1 wants to merge 1 commit intotronprotocol:developfrom
bladehan1:fix/ci_coverage_gate
Open

fix(ci): gate changed-line coverage with diff-cover#6706
bladehan1 wants to merge 1 commit intotronprotocol:developfrom
bladehan1:fix/ci_coverage_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.

Commit included

Subject Purpose
ci(coverage): gate changed-line coverage Introduce diff-cover as the changed-code gate; switch madrapps/jacoco-report to summary-only; handle non-Java PRs via an NA sentinel reported as SKIPPED; guard empty SRC_ROOTS; verify diff-cover.json was produced; clarify LINE vs INSTRUCTION semantics in the metrics panel.

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.

- Use diff-cover changed-line coverage as the changed-code gate; keep
  the overall coverage delta gate at -0.1% and JaCoCo reports as
  summary-only snapshots.
- Handle non-Java PRs: when diff-cover reports no changed Java lines
  (workflow-only, docs-only, proto-only), emit NA and treat it as
  SKIPPED in the enforcement step so the gate does not wrongly fail.
- Harden the diff-cover step: guard against empty SRC_ROOTS before
  invoking diff-cover, verify diff-cover.json was written, and surface
  exit codes in failure messages.
- Clarify metric semantics in the step summary: Changed-line is LINE
  coverage (diff-cover) while Overall / Delta are INSTRUCTION coverage
  (jacoco-report).
@github-actions github-actions Bot requested a review from halibobo1205 April 24, 2026 10:02
@bladehan1 bladehan1 requested a review from 317787106 April 24, 2026 10:07
@halibobo1205 halibobo1205 added this to the GreatVoyage-v4.8.2 milestone Apr 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants