From 007a9cb999fb0e60b1cb2542080ea2f2a68d5327 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 18 May 2026 15:25:14 -0400 Subject: [PATCH 1/4] Add coverage validation: enforce 90% main-source line coverage in CI Bazel already produced an LCOV combined report on the test-jre21 jobs (`bazel coverage --combined_report=lcov //cli/...`) and uploaded it as an artifact, but nothing read it -- a PR could land that dropped coverage arbitrarily. This wires up: * `tools/coverage_check.py` + `:coverage-check` py_binary -- parses the LCOV report, prints a per-file table sorted by coverage ascending and an overall summary, exits non-zero if main-source line coverage is below `$COVERAGE_THRESHOLD` (default 90). Test sources and files with `LF:0` (no instrumentation data) are filtered out so thin production-code coverage can't hide behind well-covered tests or placeholder records. * `tools/coverage_check_test.py` + `:coverage_check_test` py_test -- 19 unit tests against parse_lcov, filter_main_source, format_report, and main(), covering: per-file extraction, non-line LCOV fields skipped, malformed payloads tolerated, multi-prefix include union, zero-line records dropped, threshold boundary inclusive, missing LCOV file => exit 2, env-var threshold override. * `make coverage` / `make coverage-check` / `make coverage-test` -- one-line invocations. `make coverage` runs `bazel coverage` then the binary; `make coverage-check` runs only the binary against an existing LCOV; `make coverage-test` runs the py_test. * CI: the `Run bazel-diff tests` step now coverage-instruments `//cli/... //tools:coverage_check_test`, and a new `Enforce coverage threshold` step invokes the py_binary on every test-jre21 matrix entry. The threshold step is placed AFTER the artifact upload so a failed threshold check still preserves the LCOV report. The threshold is 90 per the issue. Coverage on this PR's test-jre21 matrix run is the source of truth for the current repo state. Implementation notes: - The enforcement logic is a `py_library` plus `py_binary` and `py_test` that depend on it via `imports = ["."]`, so the test imports the library by its module name without needing a workspace-rooted path. - The `BUILD_WORKING_DIRECTORY` chdir hook in coverage_check.main lets `bazel run //tools:coverage-check -- bazel-out/_coverage/...` resolve the relative LCOV path against the user's cwd instead of the runfiles directory. - Files with `LF:0` are dropped from the per-file table and the totals, not treated as 0% covered. Today bazel's Python coverage emits exactly that for `tools/coverage_check.py` (no `coverage_tool` configured on the Python toolchain) -- so the .py file's coverage is enforced via the py_test passing, not via LCOV numbers. Configuring the toolchain with `configure_coverage_tool = True` in MODULE.bazel would surface numerical Python coverage and the file would slot in automatically. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yaml | 6 +- Makefile | 13 ++ README.md | 31 +++++ tools/BUILD | 28 +++++ tools/coverage_check.py | 209 +++++++++++++++++++++++++++++++ tools/coverage_check_test.py | 234 +++++++++++++++++++++++++++++++++++ tools/readme_template.md | 31 +++++ 7 files changed, 551 insertions(+), 1 deletion(-) create mode 100644 tools/coverage_check.py create mode 100644 tools/coverage_check_test.py diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 911831b..fd41b37 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -38,7 +38,7 @@ jobs: - name: Run bazel-diff tests env: USE_BAZEL_VERSION: ${{ matrix.bazel }} - run: ~/go/bin/bazelisk coverage --combined_report=lcov //cli/... --enable_bzlmod=true --enable_workspace=false + run: ~/go/bin/bazelisk coverage --combined_report=lcov //cli/... //tools:coverage_check_test --enable_bzlmod=true --enable_workspace=false - name: Upload coverage report uses: actions/upload-artifact@v4 if: always() @@ -46,6 +46,10 @@ jobs: name: coverage-report-jre21-${{ matrix.os }}-bazel-${{ matrix.bazel }} path: bazel-out/_coverage/_coverage_report.dat if-no-files-found: warn + - name: Enforce coverage threshold (>= 90% main-source line coverage) + env: + COVERAGE_THRESHOLD: '90' + run: ~/go/bin/bazelisk run //tools:coverage-check -- bazel-out/_coverage/_coverage_report.dat - name: Upload test logs uses: actions/upload-artifact@v4 if: always() diff --git a/Makefile b/Makefile index 2240c90..7307cf5 100644 --- a/Makefile +++ b/Makefile @@ -21,3 +21,16 @@ format: .PHONY: generate-readme generate-readme: bazel run //tools:generate-readme + +.PHONY: coverage +coverage: + bazel coverage --combined_report=lcov //cli/... //tools:coverage_check_test + bazel run //tools:coverage-check -- bazel-out/_coverage/_coverage_report.dat + +.PHONY: coverage-check +coverage-check: + bazel run //tools:coverage-check -- bazel-out/_coverage/_coverage_report.dat + +.PHONY: coverage-test +coverage-test: + bazel test //tools:coverage_check_test diff --git a/README.md b/README.md index 485b7ad..f7c1eb4 100644 --- a/README.md +++ b/README.md @@ -532,6 +532,37 @@ To run the tests simply run bazel test //... ``` +## Code coverage + +CI enforces a minimum **90% line coverage** on production sources (`cli/src/main/...`). To +run the same check locally: + +```terminal +make coverage +``` + +This invokes `bazel coverage --combined_report=lcov //cli/... //tools:coverage_check_test` +and then runs `//tools:coverage-check` against the resulting LCOV report. The check is a +Python `py_binary` ([`tools/coverage_check.py`](tools/coverage_check.py)) that prints a +per-file table sorted by coverage (worst first), the overall percentage, and exits +non-zero if main-source coverage is below the threshold. + +If you've already produced a coverage report and just want to re-check the threshold, +`make coverage-check` runs only the binary against `bazel-out/_coverage/_coverage_report.dat`. + +The enforcement logic itself is tested under `//tools:coverage_check_test` — run it +directly with `make coverage-test` (or `bazel test //tools:coverage_check_test`). + +To experiment with a different threshold (e.g. while ratcheting up), set +`COVERAGE_THRESHOLD`: + +```terminal +COVERAGE_THRESHOLD=80 make coverage +``` + +The CI matrix runs the same check on every Linux/macOS test job, so a PR cannot +land if it drops main-source line coverage below the threshold. + ## Versioning We use [SemVer](http://semver.org/) for versioning. For the versions available, diff --git a/tools/BUILD b/tools/BUILD index 00fd024..13e0538 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -1,3 +1,5 @@ +load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") + genrule( name = "cli_help_output", outs = [ @@ -24,3 +26,29 @@ py_binary( ], python_version = "PY3", ) + +# Coverage threshold enforcement: a py_library so both the runnable binary and the +# unit-test target can share the same source. Running the unit tests via +# `bazel coverage` instruments the library's source and contributes its coverage to +# the combined LCOV report, so the enforcement code itself participates in the gate. +py_library( + name = "coverage_check_lib", + srcs = ["coverage_check.py"], + imports = ["."], + visibility = ["//visibility:private"], +) + +py_binary( + name = "coverage-check", + srcs = ["coverage_check.py"], + main = "coverage_check.py", + python_version = "PY3", + visibility = ["//visibility:public"], +) + +py_test( + name = "coverage_check_test", + srcs = ["coverage_check_test.py"], + python_version = "PY3", + deps = [":coverage_check_lib"], +) diff --git a/tools/coverage_check.py b/tools/coverage_check.py new file mode 100644 index 0000000..2fed70b --- /dev/null +++ b/tools/coverage_check.py @@ -0,0 +1,209 @@ +"""Enforce a minimum line-coverage threshold on a Bazel LCOV report. + +Usage: + coverage_check.py [--threshold N] [--include PREFIX,...] [LCOV_FILE] + +Defaults: + LCOV_FILE bazel-out/_coverage/_coverage_report.dat + --threshold 90 (or $COVERAGE_THRESHOLD) + --include cli/src/main/,tools/coverage_check.py + +Only production-source files (whose path matches one of the include prefixes) +contribute to the numerator and denominator. Bazel's Kotlin and Python +instrumentation report test sources too; including them would let thin +production-code coverage hide behind well-covered tests, so they are stripped. + +Exit codes: + 0 overall coverage meets or exceeds the threshold + 1 overall coverage is below the threshold + 2 the LCOV report is missing, unreadable, or has no production-source lines +""" + +import argparse +import os +import sys +from dataclasses import dataclass +from typing import Iterable, List + + +@dataclass(frozen=True) +class FileCoverage: + """Per-file line coverage extracted from one LCOV `SF` record.""" + + path: str + lines_found: int + lines_hit: int + + @property + def pct(self) -> float: + return (self.lines_hit / self.lines_found * 100.0) if self.lines_found else 0.0 + + +def parse_lcov(text: str) -> List[FileCoverage]: + """Parse an LCOV `.info`/`.dat` blob into per-file coverage records. + + Only `SF`, `LF`, `LH`, and `end_of_record` lines are read; everything else + (`FN`, `FNDA`, `DA`, `BRF`, etc.) is ignored because the threshold is + line-coverage-based. + + Malformed integer payloads on `LF`/`LH` fall back to 0 rather than raising, + so a single bad record can't take down the whole check. + """ + records: List[FileCoverage] = [] + sf = None + lf = 0 + lh = 0 + for raw in text.splitlines(): + if raw.startswith("SF:"): + sf = raw[3:] + lf = 0 + lh = 0 + elif raw.startswith("LF:") and sf is not None: + try: + lf = int(raw[3:]) + except ValueError: + lf = 0 + elif raw.startswith("LH:") and sf is not None: + try: + lh = int(raw[3:]) + except ValueError: + lh = 0 + elif raw == "end_of_record" and sf is not None: + records.append(FileCoverage(sf, lf, lh)) + sf = None + return records + + +def filter_main_source( + records: Iterable[FileCoverage], include_prefixes: List[str] +) -> List[FileCoverage]: + """Keep records whose path begins with one of `include_prefixes` and that + have at least one instrumented line. + + Files with `LF:0` are dropped because LCOV has no measurable content for + them. This happens in two ways: (a) the file genuinely has no + instrumentable lines (rare), and (b) the language's instrumentation tool + didn't run for that file. Bazel's Python coverage in particular only + gathers data when the toolchain is configured with `coverage_tool` set, + so without that step a `py_test` produces an `LF:0`/`LH:0` placeholder + record for its sources. Treating that as "0% covered" would be misleading + and tank the threshold for files that are actually well-tested. + """ + if not include_prefixes: + return [] + return [ + r + for r in records + if r.lines_found > 0 + and any(r.path.startswith(p) for p in include_prefixes) + ] + + +def format_report( + records: List[FileCoverage], total_lh: int, total_lf: int, threshold: float +) -> str: + """Render the per-file table + overall summary as a multi-line string.""" + lines = [] + lines.append(f"{'COV%':>8} {'LINES (hit/total)':<17} FILE") + lines.append(f"{'-' * 8:>8} {'-' * 17:<17} ----") + for r in sorted(records, key=lambda r: (r.pct, r.path)): + lines.append( + f"{r.pct:>7.2f}% {r.lines_hit:>5} / {r.lines_found:<7} {r.path}" + ) + overall = (total_lh / total_lf * 100.0) if total_lf else 0.0 + lines.append("") + lines.append( + f"Overall main-source line coverage: {overall:.2f}% ({total_lh} / {total_lf})" + ) + lines.append(f"Threshold: {threshold:.2f}%") + return "\n".join(lines) + + +def _chdir_to_workspace_if_invoked_via_bazel_run() -> None: + """`bazel run //tools:coverage-check -- bazel-out/_coverage/...` would otherwise + leave us in the runfiles directory and fail to find the LCOV file. When Bazel + sets `BUILD_WORKING_DIRECTORY` (the user's cwd at the time they ran the binary) + chdir to it so relative paths resolve the way the user expects. + """ + cwd = os.environ.get("BUILD_WORKING_DIRECTORY") + if cwd and os.path.isdir(cwd): + os.chdir(cwd) + + +def main(argv: List[str] | None = None) -> int: + _chdir_to_workspace_if_invoked_via_bazel_run() + + parser = argparse.ArgumentParser( + description="Enforce a minimum line-coverage threshold on a Bazel LCOV report.", + ) + parser.add_argument( + "lcov", + nargs="?", + default="bazel-out/_coverage/_coverage_report.dat", + help="Path to the LCOV report (default: %(default)s).", + ) + parser.add_argument( + "--threshold", + "-t", + type=float, + default=float(os.environ.get("COVERAGE_THRESHOLD", "90")), + help=( + "Minimum overall line coverage percentage. " + "Default 90 (override with --threshold or $COVERAGE_THRESHOLD)." + ), + ) + parser.add_argument( + "--include", + default=os.environ.get( + "COVERAGE_INCLUDE", "cli/src/main/,tools/coverage_check.py" + ), + help=( + "Comma-separated path prefixes counted as production code " + "(default: cli/src/main/,tools/coverage_check.py)." + ), + ) + args = parser.parse_args(argv) + + if not os.path.isfile(args.lcov): + print(f"error: LCOV report not found at '{args.lcov}'.", file=sys.stderr) + print( + "Hint: run 'bazel coverage --combined_report=lcov //cli/... //tools/...' first,", + file=sys.stderr, + ) + print(" or pass an explicit path as the first argument.", file=sys.stderr) + return 2 + + with open(args.lcov, "r", encoding="utf-8", errors="replace") as f: + all_records = parse_lcov(f.read()) + + include_prefixes = [p.strip() for p in args.include.split(",") if p.strip()] + records = filter_main_source(all_records, include_prefixes) + + total_lf = sum(r.lines_found for r in records) + total_lh = sum(r.lines_hit for r in records) + if total_lf == 0: + print( + f"error: no instrumented production-source lines found in {args.lcov}.", + file=sys.stderr, + ) + print(f" include prefixes: {include_prefixes!r}", file=sys.stderr) + return 2 + + print(format_report(records, total_lh, total_lf, args.threshold)) + + overall = total_lh / total_lf * 100.0 + # Allow a tiny epsilon so 89.99999... that displays as 90.00 still passes a 90 + # threshold. Use the raw value, not the rounded one, otherwise 89.99 would slip + # through. + if overall + 1e-9 < args.threshold: + print( + f"FAIL: coverage {overall:.2f}% is below threshold {args.threshold:.2f}%.", + file=sys.stderr, + ) + return 1 + print("PASS") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/coverage_check_test.py b/tools/coverage_check_test.py new file mode 100644 index 0000000..db9fd28 --- /dev/null +++ b/tools/coverage_check_test.py @@ -0,0 +1,234 @@ +"""Unit tests for tools/coverage_check.py. + +Run via Bazel: + bazel test //tools:coverage_check_test +""" + +import io +import os +import sys +import tempfile +import unittest +from contextlib import redirect_stderr, redirect_stdout + +from coverage_check import ( + FileCoverage, + filter_main_source, + format_report, + main, + parse_lcov, +) + + +# Mixed LCOV blob covering all the cases the check has to handle: +# - one main-source file at 90% (9/10) +# - one main-source file at 0% (0/5) +# - one TEST source that should be stripped entirely +SIMPLE_LCOV = """\ +SF:cli/src/main/kotlin/com/bazel_diff/A.kt +LF:10 +LH:9 +end_of_record +SF:cli/src/main/kotlin/com/bazel_diff/B.kt +LF:5 +LH:0 +end_of_record +SF:cli/src/test/kotlin/com/bazel_diff/ATest.kt +LF:20 +LH:20 +end_of_record +""" + + +class ParseLcovTest(unittest.TestCase): + def test_extracts_per_file_records(self): + records = parse_lcov(SIMPLE_LCOV) + self.assertEqual(len(records), 3) + self.assertEqual(records[0].path, "cli/src/main/kotlin/com/bazel_diff/A.kt") + self.assertEqual(records[0].lines_found, 10) + self.assertEqual(records[0].lines_hit, 9) + self.assertAlmostEqual(records[0].pct, 90.0) + + def test_ignores_non_line_lcov_fields(self): + # FN/FNDA/DA/BRF lines should be skipped without disturbing the LF/LH read. + text = ( + "SF:foo.kt\n" + "FN:1,foo\n" + "FNDA:0,foo\n" + "FNF:1\n" + "FNH:0\n" + "DA:1,0\n" + "DA:2,1\n" + "LF:2\n" + "LH:1\n" + "BRF:0\n" + "BRH:0\n" + "end_of_record\n" + ) + records = parse_lcov(text) + self.assertEqual(len(records), 1) + self.assertEqual(records[0].lines_found, 2) + self.assertEqual(records[0].lines_hit, 1) + + def test_empty_input_returns_no_records(self): + self.assertEqual(parse_lcov(""), []) + + def test_record_without_terminator_is_skipped(self): + # If end_of_record is missing, we drop the record rather than emit a half- + # parsed FileCoverage. Otherwise a truncated LCOV blob would silently inflate + # the line count. + self.assertEqual(parse_lcov("SF:foo.kt\nLF:10\nLH:5\n"), []) + + def test_malformed_lf_falls_back_to_zero(self): + # A garbage LF payload must not raise; the file is still recorded but with + # zero instrumentable lines, contributing nothing to the totals. + records = parse_lcov("SF:x\nLF:not_a_number\nLH:0\nend_of_record\n") + self.assertEqual(len(records), 1) + self.assertEqual(records[0].lines_found, 0) + + def test_pct_zero_when_no_lines_found(self): + self.assertEqual(FileCoverage("x", 0, 0).pct, 0.0) + + +class FilterMainSourceTest(unittest.TestCase): + def test_keeps_only_matching_prefixes(self): + records = parse_lcov(SIMPLE_LCOV) + filtered = filter_main_source(records, ["cli/src/main/"]) + self.assertEqual( + [r.path for r in filtered], + [ + "cli/src/main/kotlin/com/bazel_diff/A.kt", + "cli/src/main/kotlin/com/bazel_diff/B.kt", + ], + ) + + def test_multiple_prefixes_are_unioned(self): + records = [ + FileCoverage("cli/src/main/a.kt", 1, 1), + FileCoverage("tools/coverage_check.py", 1, 1), + FileCoverage("cli/src/test/a.kt", 1, 1), + ] + filtered = filter_main_source( + records, ["cli/src/main/", "tools/coverage_check.py"] + ) + self.assertEqual( + [r.path for r in filtered], + ["cli/src/main/a.kt", "tools/coverage_check.py"], + ) + + def test_empty_include_returns_empty(self): + records = parse_lcov(SIMPLE_LCOV) + self.assertEqual(filter_main_source(records, []), []) + + def test_drops_zero_line_records(self): + # Bazel's Python coverage emits `LF:0`/`LH:0` for the .py files of a + # py_test target when no coverage tool is configured; we treat those + # as "no data" rather than "0% covered" to avoid tanking the threshold + # on files that the toolchain just doesn't have instrumentation for. + records = [ + FileCoverage("cli/src/main/has_data.kt", 10, 5), + FileCoverage("cli/src/main/no_data.kt", 0, 0), + FileCoverage("tools/coverage_check.py", 0, 0), + ] + filtered = filter_main_source( + records, ["cli/src/main/", "tools/coverage_check.py"] + ) + self.assertEqual([r.path for r in filtered], ["cli/src/main/has_data.kt"]) + + +class FormatReportTest(unittest.TestCase): + def test_includes_overall_summary(self): + records = [FileCoverage("a.kt", 10, 9), FileCoverage("b.kt", 10, 1)] + out = format_report(records, total_lh=10, total_lf=20, threshold=90.0) + self.assertIn("Overall main-source line coverage: 50.00% (10 / 20)", out) + self.assertIn("Threshold: 90.00%", out) + + def test_sorts_worst_covered_first(self): + records = [FileCoverage("a.kt", 10, 9), FileCoverage("b.kt", 10, 1)] + out = format_report(records, total_lh=10, total_lf=20, threshold=90.0) + a_idx = out.index("a.kt") + b_idx = out.index("b.kt") + self.assertLess(b_idx, a_idx, "Worst-covered file should sort first") + + def test_empty_records_still_prints_summary(self): + # No records => zeroed-out summary, but no crash. The main() path treats + # this as exit-2 separately; format_report itself stays well-defined. + out = format_report([], total_lh=0, total_lf=0, threshold=90.0) + self.assertIn("Overall main-source line coverage: 0.00% (0 / 0)", out) + + +class MainTest(unittest.TestCase): + def _write_lcov(self, content: str) -> str: + fd, path = tempfile.mkstemp(suffix=".dat") + with os.fdopen(fd, "w") as f: + f.write(content) + self.addCleanup(os.remove, path) + return path + + def _run_main(self, argv): + """Invoke main() with stdout/stderr captured. Returns (exit_code, stdout, stderr).""" + out, err = io.StringIO(), io.StringIO() + with redirect_stdout(out), redirect_stderr(err): + rc = main(argv) + return rc, out.getvalue(), err.getvalue() + + def test_passes_when_above_threshold(self): + path = self._write_lcov(SIMPLE_LCOV) + # SIMPLE_LCOV main-source lines: 9 hit / 15 total = 60.00% + rc, stdout, _ = self._run_main([path, "--threshold", "50"]) + self.assertEqual(rc, 0) + self.assertIn("PASS", stdout) + self.assertIn("60.00%", stdout) + + def test_fails_when_below_threshold(self): + path = self._write_lcov(SIMPLE_LCOV) + rc, _, stderr = self._run_main([path, "--threshold", "90"]) + self.assertEqual(rc, 1) + self.assertIn("FAIL", stderr) + + def test_threshold_boundary_is_inclusive(self): + # 9/15 = 60.00% exactly; threshold=60 must PASS, threshold=60.01 must FAIL. + path = self._write_lcov(SIMPLE_LCOV) + self.assertEqual(self._run_main([path, "--threshold", "60"])[0], 0) + self.assertEqual(self._run_main([path, "--threshold", "60.01"])[0], 1) + + def test_missing_lcov_file_exits_2(self): + rc, _, stderr = self._run_main(["/does/not/exist.dat"]) + self.assertEqual(rc, 2) + self.assertIn("not found", stderr) + + def test_no_main_source_lines_exits_2(self): + # Only a test source -- nothing matches the default include prefix. + path = self._write_lcov( + "SF:cli/src/test/kotlin/com/bazel_diff/ATest.kt\n" + "LF:5\nLH:5\nend_of_record\n" + ) + rc, _, stderr = self._run_main([path, "--include", "cli/src/main/"]) + self.assertEqual(rc, 2) + self.assertIn("no instrumented production-source lines", stderr) + + def test_default_include_covers_self_when_instrumented(self): + # The default include prefix list contains tools/coverage_check.py so that + # the script's own py_test coverage counts towards the threshold when + # Python instrumentation is configured. We simulate the instrumented case + # with non-zero LF/LH; the zero-line case (no Python coverage tool) is + # covered by FilterMainSourceTest.test_drops_zero_line_records. + path = self._write_lcov( + "SF:tools/coverage_check.py\nLF:10\nLH:10\nend_of_record\n" + ) + rc, stdout, _ = self._run_main([path]) + self.assertEqual(rc, 0) + self.assertIn("tools/coverage_check.py", stdout) + + def test_environment_threshold_override(self): + # $COVERAGE_THRESHOLD provides the default when --threshold is not passed. + path = self._write_lcov(SIMPLE_LCOV) + os.environ["COVERAGE_THRESHOLD"] = "55" + try: + self.assertEqual(self._run_main([path])[0], 0) + finally: + del os.environ["COVERAGE_THRESHOLD"] + + +if __name__ == "__main__": + unittest.main() diff --git a/tools/readme_template.md b/tools/readme_template.md index 8c69f36..cc63831 100644 --- a/tools/readme_template.md +++ b/tools/readme_template.md @@ -271,6 +271,37 @@ To run the tests simply run bazel test //... ``` +## Code coverage + +CI enforces a minimum **90% line coverage** on production sources (`cli/src/main/...`). To +run the same check locally: + +```terminal +make coverage +``` + +This invokes `bazel coverage --combined_report=lcov //cli/... //tools:coverage_check_test` +and then runs `//tools:coverage-check` against the resulting LCOV report. The check is a +Python `py_binary` ([`tools/coverage_check.py`](tools/coverage_check.py)) that prints a +per-file table sorted by coverage (worst first), the overall percentage, and exits +non-zero if main-source coverage is below the threshold. + +If you've already produced a coverage report and just want to re-check the threshold, +`make coverage-check` runs only the binary against `bazel-out/_coverage/_coverage_report.dat`. + +The enforcement logic itself is tested under `//tools:coverage_check_test` — run it +directly with `make coverage-test` (or `bazel test //tools:coverage_check_test`). + +To experiment with a different threshold (e.g. while ratcheting up), set +`COVERAGE_THRESHOLD`: + +```terminal +COVERAGE_THRESHOLD=80 make coverage +``` + +The CI matrix runs the same check on every Linux/macOS test job, so a PR cannot +land if it drops main-source line coverage below the threshold. + ## Versioning We use [SemVer](http://semver.org/) for versioning. For the versions available, From 554a65c23a3a12083cd15b6b8e42e48badaa4e29 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 18 May 2026 19:23:05 -0400 Subject: [PATCH 2/4] Add unit tests for small low-coverage files to clear the 90% gate CI on PR #356 reported main-source coverage at 88.76% (1429 / 1610) -- 1.24 points short of the 90% threshold introduced in the previous commit. These five focused unit tests close the gap by covering the smallest files that were near-zero on coverage, all of which were thin enough to test in isolation without mocking out infrastructure: BazelTargetTypeTest enum constants exhaustively touched (0/6 -> 6/6) VersionProviderTest happy-path resource load (1/9 -> 6/9) BazelDiffTest run() throws + isVerbose() truth (2/6 -> 6/6) StderrLoggerTest all 4 overloads x verbose true+false (8/12 -> 12/12) BazelTargetTest three subclass shapes + every type discriminator branch (21/29 -> 28/29) Each test file is registered as a `kt_jvm_test` in cli/BUILD; they all run in <1s and join the existing `:cli-test-lib`. Notes: - Main.kt (0/4) is not addressed here: its only behaviour is `exitProcess(...)`, which kills the JVM and so cannot be exercised from in-process unit tests without a SecurityManager hack or a refactor to extract a testable `run(args): Int`. The E2E suite already invokes the built jar as a subprocess, but JaCoCo doesn't instrument across that boundary, so Main.kt stays at 0% for now. The threshold gate accommodates this: 6 + 5 + 4 + 4 + 7 = 26 lines added, projecting CI to (1429+26)/1610 = 90.37%, a hair past the bar. - VersionProvider's `throw IllegalArgumentException(...)` branch stays uncovered (3 unreachable lines) because the production code resolves the classloader from `this::class.java`, with no injection seam to swap it for a loader that doesn't contain `cli/version`. Exercising it would require either reflection-mutating the classloader or refactoring the resource lookup to take a `ClassLoader` parameter -- both out of scope here. - BazelTarget's last uncovered line is the `else -> BazelTargetType.UNKNOWN` branch, only reachable when `Build.Target.Discriminator` adds a new enum value the production code doesn't recognise. Triggering that today would require a hand-forged proto with a reserved discriminator number, which the protobuf builder rejects. Co-Authored-By: Claude Opus 4.7 (1M context) --- cli/BUILD | 30 ++++++++ .../com/bazel_diff/bazel/BazelTargetTest.kt | 75 +++++++++++++++++++ .../bazel_diff/bazel/BazelTargetTypeTest.kt | 32 ++++++++ .../com/bazel_diff/cli/BazelDiffTest.kt | 42 +++++++++++ .../com/bazel_diff/cli/VersionProviderTest.kt | 19 +++++ .../com/bazel_diff/log/StderrLoggerTest.kt | 67 +++++++++++++++++ 6 files changed, 265 insertions(+) create mode 100644 cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTest.kt create mode 100644 cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTypeTest.kt create mode 100644 cli/src/test/kotlin/com/bazel_diff/cli/BazelDiffTest.kt create mode 100644 cli/src/test/kotlin/com/bazel_diff/cli/VersionProviderTest.kt create mode 100644 cli/src/test/kotlin/com/bazel_diff/log/StderrLoggerTest.kt diff --git a/cli/BUILD b/cli/BUILD index 0bdb52b..627aa49 100644 --- a/cli/BUILD +++ b/cli/BUILD @@ -108,6 +108,36 @@ kt_jvm_test( runtime_deps = [":cli-test-lib"], ) +kt_jvm_test( + name = "BazelTargetTest", + test_class = "com.bazel_diff.bazel.BazelTargetTest", + runtime_deps = [":cli-test-lib"], +) + +kt_jvm_test( + name = "BazelTargetTypeTest", + test_class = "com.bazel_diff.bazel.BazelTargetTypeTest", + runtime_deps = [":cli-test-lib"], +) + +kt_jvm_test( + name = "BazelDiffTest", + test_class = "com.bazel_diff.cli.BazelDiffTest", + runtime_deps = [":cli-test-lib"], +) + +kt_jvm_test( + name = "VersionProviderTest", + test_class = "com.bazel_diff.cli.VersionProviderTest", + runtime_deps = [":cli-test-lib"], +) + +kt_jvm_test( + name = "StderrLoggerTest", + test_class = "com.bazel_diff.log.StderrLoggerTest", + runtime_deps = [":cli-test-lib"], +) + kt_jvm_test( name = "BazelModServiceTest", test_class = "com.bazel_diff.bazel.BazelModServiceTest", diff --git a/cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTest.kt b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTest.kt new file mode 100644 index 0000000..abb3b6d --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTest.kt @@ -0,0 +1,75 @@ +package com.bazel_diff.bazel + +import assertk.assertThat +import assertk.assertions.isEqualTo +import com.google.devtools.build.lib.query2.proto.proto2api.Build +import org.junit.Test + +class BazelTargetTest { + // Helper that builds a Rule-shaped target with a chosen discriminator so we can + // exercise the `type` getter's branches without needing six separate hand-built + // protos. `BazelTarget.Rule` only asserts `target.hasRule()` in its init, which + // is satisfied by setting any Rule on the builder, so we can drive any + // discriminator through it. + private fun ruleTargetWith(d: Build.Target.Discriminator): Build.Target = + Build.Target.newBuilder() + .setType(d) + .setRule(Build.Rule.newBuilder().setRuleClass("dummy").setName("//:dummy")) + .build() + + @Test + fun sourceFileTarget_exposesSourceFileName_andSourceFileType() { + val target = Build.Target.newBuilder() + .setType(Build.Target.Discriminator.SOURCE_FILE) + .setSourceFile( + Build.SourceFile.newBuilder() + .setName("//pkg:foo.txt") + .addSubinclude("//other:bzl.bzl")) + .build() + val bt = BazelTarget.SourceFile(target) + assertThat(bt.name).isEqualTo("//pkg:foo.txt") + assertThat(bt.sourceFileName).isEqualTo("//pkg:foo.txt") + assertThat(bt.subincludeList).isEqualTo(listOf("//other:bzl.bzl")) + assertThat(bt.type).isEqualTo(BazelTargetType.SOURCE_FILE) + } + + @Test + fun ruleTarget_exposesRuleName_andRuleType() { + val target = Build.Target.newBuilder() + .setType(Build.Target.Discriminator.RULE) + .setRule(Build.Rule.newBuilder().setRuleClass("java_library").setName("//:foo")) + .build() + val bt = BazelTarget.Rule(target) + assertThat(bt.name).isEqualTo("//:foo") + assertThat(bt.rule.name).isEqualTo("//:foo") + assertThat(bt.type).isEqualTo(BazelTargetType.RULE) + } + + @Test + fun generatedFileTarget_exposesNamesAndGeneratedFileType() { + val target = Build.Target.newBuilder() + .setType(Build.Target.Discriminator.GENERATED_FILE) + .setGeneratedFile( + Build.GeneratedFile.newBuilder() + .setName("//:gen.out") + .setGeneratingRule("//:gen")) + .build() + val bt = BazelTarget.GeneratedFile(target) + assertThat(bt.name).isEqualTo("//:gen.out") + assertThat(bt.generatedFileName).isEqualTo("//:gen.out") + assertThat(bt.generatingRuleName).isEqualTo("//:gen") + assertThat(bt.type).isEqualTo(BazelTargetType.GENERATED_FILE) + } + + @Test + fun packageGroupDiscriminator_mapsToPackageGroupType() { + val bt = BazelTarget.Rule(ruleTargetWith(Build.Target.Discriminator.PACKAGE_GROUP)) + assertThat(bt.type).isEqualTo(BazelTargetType.PACKAGE_GROUP) + } + + @Test + fun environmentGroupDiscriminator_mapsToEnvironmentGroupType() { + val bt = BazelTarget.Rule(ruleTargetWith(Build.Target.Discriminator.ENVIRONMENT_GROUP)) + assertThat(bt.type).isEqualTo(BazelTargetType.ENVIRONMENT_GROUP) + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTypeTest.kt b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTypeTest.kt new file mode 100644 index 0000000..f9b0d5b --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/bazel/BazelTargetTypeTest.kt @@ -0,0 +1,32 @@ +package com.bazel_diff.bazel + +import assertk.assertThat +import assertk.assertions.containsExactlyInAnyOrder +import assertk.assertions.hasSize +import assertk.assertions.isEqualTo +import org.junit.Test + +class BazelTargetTypeTest { + @Test + fun enumDeclaresExpectedValues() { + // Reading every constant forces the JVM to initialise the whole enum, which + // is what gives this otherwise-trivial declaration any line coverage. + val all = BazelTargetType.entries + assertThat(all).hasSize(6) + assertThat(all).containsExactlyInAnyOrder( + BazelTargetType.RULE, + BazelTargetType.SOURCE_FILE, + BazelTargetType.GENERATED_FILE, + BazelTargetType.PACKAGE_GROUP, + BazelTargetType.ENVIRONMENT_GROUP, + BazelTargetType.UNKNOWN, + ) + } + + @Test + fun valueOfRoundTrip() { + for (t in BazelTargetType.entries) { + assertThat(BazelTargetType.valueOf(t.name)).isEqualTo(t) + } + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/cli/BazelDiffTest.kt b/cli/src/test/kotlin/com/bazel_diff/cli/BazelDiffTest.kt new file mode 100644 index 0000000..a3680d5 --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/cli/BazelDiffTest.kt @@ -0,0 +1,42 @@ +package com.bazel_diff.cli + +import assertk.assertThat +import assertk.assertions.contains +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import org.junit.Assert.assertThrows +import org.junit.Test +import picocli.CommandLine + +class BazelDiffTest { + @Test + fun runThrowsWhenInvokedWithoutSubcommand() { + // BazelDiff is a parent command -- it must be invoked with `generate-hashes` or + // `get-impacted-targets`. Calling `run()` directly mirrors picocli's behaviour + // when the user passes no subcommand and should surface a ParameterException + // that picocli will translate to a usage error. + val diff = BazelDiff() + diff.spec = CommandLine(diff).commandSpec + val ex = assertThrows(CommandLine.ParameterException::class.java) { diff.run() } + assertThat(ex.message!!).contains("Missing required subcommand") + } + + @Test + fun isVerboseFalseByDefault() { + assertThat(BazelDiff().isVerbose()).isFalse() + } + + @Test + fun isVerboseTrueWhenVerboseFlagSet() { + val diff = BazelDiff() + diff.verbose = true + assertThat(diff.isVerbose()).isTrue() + } + + @Test + fun isVerboseTrueWhenDebugFlagSet() { + val diff = BazelDiff() + diff.debug = true + assertThat(diff.isVerbose()).isTrue() + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/cli/VersionProviderTest.kt b/cli/src/test/kotlin/com/bazel_diff/cli/VersionProviderTest.kt new file mode 100644 index 0000000..4eaa7ba --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/cli/VersionProviderTest.kt @@ -0,0 +1,19 @@ +package com.bazel_diff.cli + +import assertk.assertThat +import assertk.assertions.hasSize +import assertk.assertions.isNotEmpty +import org.junit.Test + +class VersionProviderTest { + @Test + fun loadsBundledVersionResource() { + // tools/version is generated by the //cli:version_file genrule and packaged as a + // classpath resource at `cli/version`. This test exercises the happy path: the + // resource is present, the BufferedReader reads it, and the trimmed contents come + // back as the single-element version array picocli expects. + val versions = VersionProvider().getVersion() + assertThat(versions).hasSize(1) + assertThat(versions[0]).isNotEmpty() + } +} diff --git a/cli/src/test/kotlin/com/bazel_diff/log/StderrLoggerTest.kt b/cli/src/test/kotlin/com/bazel_diff/log/StderrLoggerTest.kt new file mode 100644 index 0000000..11ee3a9 --- /dev/null +++ b/cli/src/test/kotlin/com/bazel_diff/log/StderrLoggerTest.kt @@ -0,0 +1,67 @@ +package com.bazel_diff.log + +import assertk.assertThat +import assertk.assertions.contains +import assertk.assertions.isEmpty +import java.io.ByteArrayOutputStream +import java.io.PrintStream +import org.junit.After +import org.junit.Before +import org.junit.Test + +class StderrLoggerTest { + private val originalErr = System.err + private lateinit var captured: ByteArrayOutputStream + + @Before + fun setUp() { + captured = ByteArrayOutputStream() + System.setErr(PrintStream(captured)) + } + + @After + fun tearDown() { + System.setErr(originalErr) + } + + @Test + fun errorMessageIsAlwaysWritten() { + // `e(block)` does not gate on verbose; errors always reach stderr so a CLI user + // sees the failure reason even with logging otherwise muted. + StderrLogger(verbose = false).e { "boom" } + assertThat(captured.toString()).contains("[Error] boom") + } + + @Test + fun errorWithThrowableWritesMessageAndStackTrace() { + StderrLogger(verbose = false).e(RuntimeException("kaboom")) { "boom" } + val out = captured.toString() + assertThat(out).contains("[Error] boom") + // Stack trace surfaces the exception message verbatim. + assertThat(out).contains("kaboom") + } + + @Test + fun warningIsSuppressedWhenNotVerbose() { + StderrLogger(verbose = false).w { "ignored" } + assertThat(captured.toString()).isEmpty() + } + + @Test + fun warningIsEmittedWhenVerbose() { + StderrLogger(verbose = true).w { "shown" } + assertThat(captured.toString()).contains("[Warning] shown") + } + + @Test + fun infoIsSuppressedWhenNotVerbose() { + StderrLogger(verbose = false).i { "ignored" } + assertThat(captured.toString()).isEmpty() + } + + @Test + fun infoIsEmittedWhenVerbose() { + StderrLogger(verbose = true).i { "shown" } + assertThat(captured.toString()).contains("[Info] shown") + } +} From a21843e2199dfa4ce243908fd0b0787e6a6ee01f Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 18 May 2026 19:44:57 -0400 Subject: [PATCH 3/4] ci: propagate USE_BAZEL_VERSION to coverage threshold step The threshold step invokes `bazelisk run //tools:coverage-check`, which on macos-latest x bazel 9.x failed with: error: LCOV report not found at 'bazel-out/_coverage/_coverage_report.dat'. Hint: run 'bazel coverage --combined_report=lcov //cli/... //tools/...' first Root cause: USE_BAZEL_VERSION was set on the `Run bazel-diff tests` step but not propagated to the threshold step, so bazelisk fell back to the 8.5.1 pin in .bazelversion and started a fresh server. That second server re-symlinked bazel-out into its own output base, and the LCOV report produced by the 9.x server became unreachable through the bazel-out/_coverage/... path. Ubuntu 8.x/9.x didn't trip because the runners kept the same bazel server warm between steps -- but the dual-version dance is a real mismatch, so the macOS flake is the canonical bug. Setting USE_BAZEL_VERSION on the threshold step keeps both invocations on the same server. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index fd41b37..2bdc730 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -48,6 +48,11 @@ jobs: if-no-files-found: warn - name: Enforce coverage threshold (>= 90% main-source line coverage) env: + # Must match the `Run bazel-diff tests` step above, otherwise bazelisk + # downloads the default bazel from .bazelversion and starts a fresh + # server whose output base loses track of the coverage report symlink + # (seen flaking on macos-latest x bazel 9.x). + USE_BAZEL_VERSION: ${{ matrix.bazel }} COVERAGE_THRESHOLD: '90' run: ~/go/bin/bazelisk run //tools:coverage-check -- bazel-out/_coverage/_coverage_report.dat - name: Upload test logs From ac90a70a28e4013b4154dc95482f4cb731ab1c95 Mon Sep 17 00:00:00 2001 From: Maxwell Elliott Date: Mon, 18 May 2026 21:30:42 -0400 Subject: [PATCH 4/4] coverage: optional genhtml export + agent skills for inspection/improvement Two additive changes to the coverage gate that landed in the prior commits. 1. `tools/coverage_check.py --html DIR` (and `make coverage-html`) When passed `--html DIR`, the threshold check additionally shells out to `genhtml` and writes an annotated per-file HTML report there. The threshold gate still runs and still drives the exit code -- HTML is an extra artifact, not a replacement -- and it's emitted even when the gate fails so the below-threshold case (when the report is most useful for interactive investigation) is covered. Fails fast with a clear install hint if `genhtml` is missing from PATH (`brew install lcov` / `apt-get install lcov`); fails with the subprocess exit code if genhtml itself errors. Five new py_test cases (`HtmlReportTest`) mock `shutil.which` and `subprocess.run` so the tests don't require lcov to be installed in the test environment: - correct genhtml argv (LCOV file, --output-directory, --quiet) - clear error when genhtml is absent (exit 2, install-hint surfaced) - subprocess error code surfaced (exit 2, genhtml exit code reported) - HTML emitted even when the threshold check fails (exit 1 + HTML path printed) Output is added to .gitignore (`coverage-html/`). Total py_test count: 19 -> 24. 2. `.claude/skills/{coverage-status,improve-coverage}/SKILL.md` Two repo-local skill files so agents (Claude Code, etc.) know how to: - inspect current coverage state (`coverage-status`: what's the overall %, which files are below threshold, how to read the sorted table, how to produce an HTML report) - raise coverage to clear the gate (`improve-coverage`: how to pick high-leverage files, how to write a new kt_jvm_test, what won't work and why -- Main.kt's exitProcess, throw branches with no DI seam, unreachable enum else-branches) The improve-coverage skill cross-references the worked example from #356 itself (88.76% -> 90.37% by adding five small unit tests). .gitignore extended to keep .claude/settings.local.json and .claude/worktrees/ out of git while letting .claude/skills/ in -- precise patterns rather than a blanket `.claude/` ignore. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/skills/coverage-status/SKILL.md | 62 +++++++++++++++ .claude/skills/improve-coverage/SKILL.md | 63 +++++++++++++++ .gitignore | 7 ++ Makefile | 6 ++ README.md | 6 ++ tools/coverage_check.py | 52 +++++++++++++ tools/coverage_check_test.py | 98 ++++++++++++++++++++++++ tools/readme_template.md | 6 ++ 8 files changed, 300 insertions(+) create mode 100644 .claude/skills/coverage-status/SKILL.md create mode 100644 .claude/skills/improve-coverage/SKILL.md diff --git a/.claude/skills/coverage-status/SKILL.md b/.claude/skills/coverage-status/SKILL.md new file mode 100644 index 0000000..9c28fd6 --- /dev/null +++ b/.claude/skills/coverage-status/SKILL.md @@ -0,0 +1,62 @@ +--- +name: coverage-status +description: Use to check the current main-source line coverage of the bazel-diff repo, find files below the 90% threshold, see how far the repo is from the gate, or produce an interactive HTML coverage report. Triggers on questions like "what's the test coverage", "is the coverage gate passing", "which files have the worst coverage", "show me a coverage report", or any other inspection of the LCOV output from bazel coverage. +--- + +# Checking coverage status + +bazel-diff has a 90% main-source line-coverage gate enforced in CI ([.github/workflows/ci.yaml](../../../.github/workflows/ci.yaml)). The enforcement code lives in [tools/coverage_check.py](../../../tools/coverage_check.py); the gate runs on every `test-jre21` matrix entry as `bazelisk run //tools:coverage-check -- bazel-out/_coverage/_coverage_report.dat`. + +## Fastest path: just check the current state + +```bash +make coverage +``` + +This runs `bazel coverage --combined_report=lcov //cli/... //tools:coverage_check_test`, then `bazel run //tools:coverage-check` against the combined report. It prints a per-file table sorted worst-first, the overall percentage, and PASS/FAIL against the threshold. Exits non-zero when below. + +If a coverage report already exists in `bazel-out/_coverage/_coverage_report.dat` (e.g. from a prior `bazel coverage` invocation) and you only want to re-check the threshold without re-running the test suite: + +```bash +make coverage-check +``` + +For an annotated HTML report (lines highlighted as covered/uncovered, per-file drilldown), use: + +```bash +make coverage-html +``` + +Output lands at `coverage-html/index.html`. Requires `genhtml` from the `lcov` package (`brew install lcov` on macOS, `apt-get install lcov` on Debian/Ubuntu). The threshold gate still runs alongside HTML generation — the HTML is produced even when the gate fails, since the below-threshold case is exactly when the report is most useful. + +## Interpreting the output + +The per-file table looks like: + +``` + COV% LINES (hit/total) FILE +-------- ----------------- ---- + 0.00% 0 / 4 cli/src/main/kotlin/com/bazel_diff/Main.kt + 62.67% 47 / 75 cli/src/main/kotlin/com/bazel_diff/bazel/BazelModService.kt + ... + 100.00% 23 / 23 cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt + +Overall main-source line coverage: 90.37% (1455 / 1610) +Threshold: 90.00% +PASS +``` + +- **Sort order is ascending** — worst-covered files appear first, so the gap is at the top. +- **Test sources (`/test/`) are excluded** — Bazel's Kotlin instrumentation reports them too, but they don't count toward the threshold (otherwise well-covered tests would hide thin production-code coverage). +- **Files with `LF:0` are dropped** — that's an "no instrumentation data" signal (e.g. Bazel's Python coverage when no toolchain coverage tool is configured), and treating it as 0% would tank the threshold. +- **Threshold is inclusive at the boundary** — exactly 90.00% passes; 89.99% fails. + +## Looking at CI's number, not local + +Local coverage may be lower than CI's because the slow `//cli:E2ETest` is often skipped or fails on dev machines (JDK-env sandbox issues). The number CI sees comes from the `test-jre21` matrix; download the `coverage-report-jre21-*` artifact from a recent run for an exact snapshot, or just read the "Overall main-source line coverage:" line in the workflow log. + +## Configuration + +- **Threshold**: `COVERAGE_THRESHOLD` env var (default 90). Set in [.github/workflows/ci.yaml](../../../.github/workflows/ci.yaml) for CI, or on the command line to experiment: `COVERAGE_THRESHOLD=85 make coverage`. +- **Include prefixes**: `--include` flag or `$COVERAGE_INCLUDE` env var (default `cli/src/main/,tools/coverage_check.py`). Only files starting with one of these prefixes count. +- **Self-coverage of the checker**: `tools/coverage_check.py` is itself in the default include list, but Bazel's Python coverage emits `LF:0` for it today because the Python toolchain in `MODULE.bazel` has no `coverage_tool` configured. The 24-case py_test (`bazel test //tools:coverage_check_test`) is the actual proof the checker is exercised. diff --git a/.claude/skills/improve-coverage/SKILL.md b/.claude/skills/improve-coverage/SKILL.md new file mode 100644 index 0000000..fe51a88 --- /dev/null +++ b/.claude/skills/improve-coverage/SKILL.md @@ -0,0 +1,63 @@ +--- +name: improve-coverage +description: Use when you need to raise main-source line coverage in the bazel-diff repo, write tests for an under-covered Kotlin file, fix a CI failure on the 90% coverage gate, or pick the highest-leverage files to test next. Triggers on requests like "the coverage gate is failing, fix it", "write tests for X", "we need more coverage", or "what should I test to get to 90%". +--- + +# Improving coverage to clear the 90% gate + +bazel-diff enforces a 90% main-source line-coverage gate on every PR (see [coverage-status](../coverage-status/SKILL.md) for the inspection side). When the gate fails or you want to raise the bar, the workflow is: pick the worst-covered files, write small focused unit tests, re-run the gate locally before pushing. + +## 1. Pick the right files to target + +Run `make coverage` (or check the latest CI artifact) and look at the top of the sorted table. Prioritise files by **uncovered-lines-per-test-effort**, not by lowest percentage: + +- **Highest-leverage**: small files at 0% (e.g. enum classes, value objects, single-method utilities) — one short unit test usually moves the needle without much code. +- **Highest absolute gain**: large files with moderate coverage (e.g. `BazelQueryService.kt` at 303 lines / 93%) — closing a small percentage gap covers many lines. +- **Lowest leverage**: tiny files at 50–80% where the remaining branches are error paths needing fault injection or refactors. + +A worked example: the PR that added the gate ([#356](https://github.com/Tinder/bazel-diff/pull/356)) raised coverage from 88.76% → 90.37% by adding five small files of tests — `BazelTargetTypeTest`, `VersionProviderTest`, `BazelDiffTest`, `StderrLoggerTest`, `BazelTargetTest` — for a total of 26 newly-covered lines. + +## 2. Write the test + +Existing tests follow a consistent shape: + +- Live under `cli/src/test/kotlin/...` mirroring the main source path. +- Use **JUnit 4** (`@Test`, `@Before`, `@After`, `org.junit.Assert.assertThrows`). +- Use **assertk** for assertions (`assertk.assertThat`, with explicit imports for each assertion like `assertk.assertions.isEqualTo`). Forgetting `import assertk.assertions.contains` on a `String.contains` assertion produces a confusing receiver-mismatch error — import every assertion you use. +- Use **mockito-kotlin** when mocking is required (existing examples: `cli/src/test/kotlin/com/bazel_diff/bazel/BazelClientTest.kt`). +- Use **koin** for DI-test setup (existing pattern: `cli/src/test/kotlin/com/bazel_diff/interactor/CalculateImpactedTargetsInteractorIssue335Test.kt`). + +Tiny files (enums, value objects, small command classes) usually only need a few targeted tests. Looking at the bytes via `assertThat(BazelTargetType.entries).hasSize(N).containsExactlyInAnyOrder(...)` is enough to cover an enum's declaration lines. + +## 3. Register the test target in cli/BUILD + +Every test needs its own `kt_jvm_test` entry: + +```python +kt_jvm_test( + name = "BazelTargetTypeTest", + test_class = "com.bazel_diff.bazel.BazelTargetTypeTest", + runtime_deps = [":cli-test-lib"], +) +``` + +The `:cli-test-lib` glob picks up the new test source automatically; the explicit `kt_jvm_test` rule is what makes it executable via `bazel test //cli:`. + +## 4. Verify locally before pushing + +```bash +bazel test //cli: # one-off run of the new test +make coverage # full gate +``` + +The local number may be lower than CI's because `//cli:E2ETest` often fails or is excluded on dev machines (JDK-env sandbox issues). If you've added tests for a file that's also exercised by E2E (e.g. `BazelQueryService.kt`), the CI delta will be smaller than the local delta — count only the lines that weren't already covered by E2E. + +## 5. Things that don't work / aren't worth attempting + +- **`Main.kt`** — calls `exitProcess(...)` which kills the JVM; can't be tested in-process without a SecurityManager hack or refactor. Stays at 0%; the threshold tolerates it. +- **`throw IllegalArgumentException(...)` branches in resource-loading code** like `VersionProvider.kt` — the production code resolves the classloader from `this::class.java`, with no injection seam to swap it for one missing the resource. Refactor or skip. +- **`else -> BazelTargetType.UNKNOWN` branches** — only reachable when Bazel's `Build.Target.Discriminator` adds a new enum value the production code doesn't recognise. Triggering today would require a hand-forged proto with a reserved discriminator number, which the protobuf builder rejects. + +## When the gate fails on a flake, not on a coverage drop + +If the CI threshold step fails with `error: LCOV report not found at 'bazel-out/_coverage/_coverage_report.dat'`, that's an infrastructure issue, not a coverage regression. The fix that landed in PR #356 was to propagate `USE_BAZEL_VERSION` to the threshold step so `bazelisk run` doesn't start a different bazel server. If you see a similar mismatch resurface, check that env propagation first. diff --git a/.gitignore b/.gitignore index 1e6cb1b..9f0d593 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,10 @@ user.bazelrc .DS_Store !.bazelversion archives/ +# Coverage HTML report (genhtml output from `make coverage-html`). +coverage-html/ +# Claude Code local state (per-user settings + scratch worktrees). +# Skills under .claude/skills/ ARE checked in -- they're shared team docs -- +# so this is a precise ignore list, not a blanket .claude/ ignore. +.claude/settings.local.json +.claude/worktrees/ diff --git a/Makefile b/Makefile index 7307cf5..12a3c7c 100644 --- a/Makefile +++ b/Makefile @@ -34,3 +34,9 @@ coverage-check: .PHONY: coverage-test coverage-test: bazel test //tools:coverage_check_test + +.PHONY: coverage-html +coverage-html: + bazel coverage --combined_report=lcov //cli/... //tools:coverage_check_test + bazel run //tools:coverage-check -- bazel-out/_coverage/_coverage_report.dat --html coverage-html + @echo "Open coverage-html/index.html in a browser to inspect." diff --git a/README.md b/README.md index f7c1eb4..c790a0a 100644 --- a/README.md +++ b/README.md @@ -553,6 +553,12 @@ If you've already produced a coverage report and just want to re-check the thres The enforcement logic itself is tested under `//tools:coverage_check_test` — run it directly with `make coverage-test` (or `bazel test //tools:coverage_check_test`). +For an interactive HTML report (annotated source with covered/uncovered lines +highlighted), use `make coverage-html`. This requires the `lcov` package +(`brew install lcov` on macOS, `apt-get install lcov` on Debian/Ubuntu) and writes +the report to `coverage-html/index.html`. The threshold gate still runs and still +sets the exit code — HTML is an additional artifact, not a replacement. + To experiment with a different threshold (e.g. while ratcheting up), set `COVERAGE_THRESHOLD`: diff --git a/tools/coverage_check.py b/tools/coverage_check.py index 2fed70b..0ec5df2 100644 --- a/tools/coverage_check.py +++ b/tools/coverage_check.py @@ -21,6 +21,8 @@ import argparse import os +import shutil +import subprocess import sys from dataclasses import dataclass from typing import Iterable, List @@ -119,6 +121,30 @@ def format_report( return "\n".join(lines) +def generate_html_report(lcov_path: str, output_dir: str) -> str: + """Render an annotated HTML coverage report from `lcov_path` into `output_dir`. + + Returns the absolute path to the generated `index.html`. Raises FileNotFoundError + if `genhtml` is not on PATH (the LCOV `lcov` package; install via + `brew install lcov` on macOS or `apt-get install lcov` on Debian/Ubuntu) and + `subprocess.CalledProcessError` if genhtml itself fails. + + The directory is created if it doesn't exist; existing files are overwritten + by genhtml's normal behaviour (it always re-emits the full report). + """ + if shutil.which("genhtml") is None: + raise FileNotFoundError( + "genhtml not found on PATH. Install lcov (brew install lcov / " + "apt-get install lcov) and retry." + ) + os.makedirs(output_dir, exist_ok=True) + subprocess.run( + ["genhtml", lcov_path, "--output-directory", output_dir, "--quiet"], + check=True, + ) + return os.path.abspath(os.path.join(output_dir, "index.html")) + + def _chdir_to_workspace_if_invoked_via_bazel_run() -> None: """`bazel run //tools:coverage-check -- bazel-out/_coverage/...` would otherwise leave us in the runfiles directory and fail to find the LCOV file. When Bazel @@ -162,6 +188,16 @@ def main(argv: List[str] | None = None) -> int: "(default: cli/src/main/,tools/coverage_check.py)." ), ) + parser.add_argument( + "--html", + metavar="DIR", + help=( + "Also write an annotated HTML coverage report to DIR (requires " + "`genhtml`: brew install lcov / apt-get install lcov). The " + "threshold check still runs and still gates the exit code; --html " + "only adds an additional artifact for interactive inspection." + ), + ) args = parser.parse_args(argv) if not os.path.isfile(args.lcov): @@ -191,6 +227,22 @@ def main(argv: List[str] | None = None) -> int: print(format_report(records, total_lh, total_lf, args.threshold)) + # Generate the HTML report before the threshold gate so an interactive + # investigation has the report to look at even when the gate fails -- the + # below-threshold case is exactly when the report is most useful. + if args.html: + try: + index_path = generate_html_report(args.lcov, args.html) + print(f"HTML report: {index_path}") + except FileNotFoundError as e: + # Fail loudly: the user explicitly asked for HTML and we couldn't + # produce it. Better to bail than to silently skip and confuse them. + print(f"error: {e}", file=sys.stderr) + return 2 + except subprocess.CalledProcessError as e: + print(f"error: genhtml exited with code {e.returncode}.", file=sys.stderr) + return 2 + overall = total_lh / total_lf * 100.0 # Allow a tiny epsilon so 89.99999... that displays as 90.00 still passes a 90 # threshold. Use the raw value, not the rounded one, otherwise 89.99 would slip diff --git a/tools/coverage_check_test.py b/tools/coverage_check_test.py index db9fd28..fdfdb73 100644 --- a/tools/coverage_check_test.py +++ b/tools/coverage_check_test.py @@ -6,11 +6,14 @@ import io import os +import subprocess import sys import tempfile import unittest from contextlib import redirect_stderr, redirect_stdout +from unittest.mock import patch +import coverage_check from coverage_check import ( FileCoverage, filter_main_source, @@ -230,5 +233,100 @@ def test_environment_threshold_override(self): del os.environ["COVERAGE_THRESHOLD"] +class HtmlReportTest(unittest.TestCase): + """Cover the --html flag without requiring genhtml to actually be installed.""" + + def _write_lcov(self, content: str) -> str: + fd, path = tempfile.mkstemp(suffix=".dat") + with os.fdopen(fd, "w") as f: + f.write(content) + self.addCleanup(os.remove, path) + return path + + def _run_main(self, argv): + out, err = io.StringIO(), io.StringIO() + with redirect_stdout(out), redirect_stderr(err): + rc = main(argv) + return rc, out.getvalue(), err.getvalue() + + def test_html_flag_invokes_genhtml_with_correct_args(self): + # Mock both `which` (so the function doesn't reject the call) and `run` + # (so we don't actually invoke a subprocess). The test asserts the + # contract: genhtml is invoked with the LCOV file, --output-directory, + # and --quiet, then the absolute index.html path is printed. + path = self._write_lcov(SIMPLE_LCOV) + out_dir = tempfile.mkdtemp() + self.addCleanup(lambda: os.rmdir(out_dir) if os.path.isdir(out_dir) else None) + + with patch.object(coverage_check.shutil, "which", return_value="/usr/bin/genhtml") as mock_which, \ + patch.object(coverage_check.subprocess, "run") as mock_run: + mock_run.return_value = subprocess.CompletedProcess(args=[], returncode=0) + rc, stdout, _ = self._run_main( + [path, "--threshold", "50", "--html", out_dir] + ) + + self.assertEqual(rc, 0) + mock_which.assert_called_once_with("genhtml") + mock_run.assert_called_once_with( + ["genhtml", path, "--output-directory", out_dir, "--quiet"], + check=True, + ) + # The absolute index.html path is surfaced so the user can open it + # directly from their terminal. + self.assertIn(os.path.abspath(os.path.join(out_dir, "index.html")), stdout) + + def test_html_flag_errors_clearly_when_genhtml_missing(self): + # When genhtml is absent we exit 2 with a clear install-hint message, + # even if the threshold itself would have passed -- the user + # explicitly asked for HTML output and we couldn't produce it. + path = self._write_lcov(SIMPLE_LCOV) + out_dir = tempfile.mkdtemp() + self.addCleanup(lambda: os.rmdir(out_dir) if os.path.isdir(out_dir) else None) + + with patch.object(coverage_check.shutil, "which", return_value=None): + rc, _, stderr = self._run_main( + [path, "--threshold", "50", "--html", out_dir] + ) + + self.assertEqual(rc, 2) + self.assertIn("genhtml not found", stderr) + self.assertIn("brew install lcov", stderr) + + def test_html_flag_errors_when_genhtml_returns_nonzero(self): + path = self._write_lcov(SIMPLE_LCOV) + out_dir = tempfile.mkdtemp() + self.addCleanup(lambda: os.rmdir(out_dir) if os.path.isdir(out_dir) else None) + + with patch.object(coverage_check.shutil, "which", return_value="/usr/bin/genhtml"), \ + patch.object(coverage_check.subprocess, "run", + side_effect=subprocess.CalledProcessError(returncode=3, cmd=["genhtml"])): + rc, _, stderr = self._run_main( + [path, "--threshold", "50", "--html", out_dir] + ) + + self.assertEqual(rc, 2) + self.assertIn("genhtml exited with code 3", stderr) + + def test_html_generated_even_when_threshold_fails(self): + # The interactive-investigation case: someone below threshold needs the + # HTML report to see WHERE the gaps are. We render the HTML and THEN + # return exit 1 for the threshold failure. + path = self._write_lcov(SIMPLE_LCOV) + out_dir = tempfile.mkdtemp() + self.addCleanup(lambda: os.rmdir(out_dir) if os.path.isdir(out_dir) else None) + + with patch.object(coverage_check.shutil, "which", return_value="/usr/bin/genhtml"), \ + patch.object(coverage_check.subprocess, "run") as mock_run: + mock_run.return_value = subprocess.CompletedProcess(args=[], returncode=0) + rc, stdout, stderr = self._run_main( + [path, "--threshold", "95", "--html", out_dir] + ) + + self.assertEqual(rc, 1) + mock_run.assert_called_once() # HTML still produced + self.assertIn("HTML report:", stdout) + self.assertIn("FAIL", stderr) + + if __name__ == "__main__": unittest.main() diff --git a/tools/readme_template.md b/tools/readme_template.md index cc63831..52d9414 100644 --- a/tools/readme_template.md +++ b/tools/readme_template.md @@ -292,6 +292,12 @@ If you've already produced a coverage report and just want to re-check the thres The enforcement logic itself is tested under `//tools:coverage_check_test` — run it directly with `make coverage-test` (or `bazel test //tools:coverage_check_test`). +For an interactive HTML report (annotated source with covered/uncovered lines +highlighted), use `make coverage-html`. This requires the `lcov` package +(`brew install lcov` on macOS, `apt-get install lcov` on Debian/Ubuntu) and writes +the report to `coverage-html/index.html`. The threshold gate still runs and still +sets the exit code — HTML is an additional artifact, not a replacement. + To experiment with a different threshold (e.g. while ratcheting up), set `COVERAGE_THRESHOLD`: