From 282c4b1fdbea30044e8160fb2e161167d06da5d8 Mon Sep 17 00:00:00 2001 From: Joris Wouter Jonkers <74975850+ExtraToast@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:56:59 +0200 Subject: [PATCH] ci(report): a pull request says what it changed and what that did to coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff breakdown was spliced into the pull request body and the coverage was nowhere: four suites measure it on every run, and the numbers only ever reached an artifact and a floor. A floor says whether a number cleared a line, never whether this change moved it. One comment now carries the change table, the four suites' statements, branches, functions and lines against a baseline from main, and the coverage of the lines the pull request itself added. It is found again by a marker and updated in place, so a second push does not leave a trail. The body is the author's again. The four suites arrive in two formats. JaCoCo counts instructions and methods where istanbul counts statements and functions; neither pair is the same measurement, but they answer the same question per column, which is what lets one table hold both toolchains. Coverage paths are matched onto changed files by their longest common tail, because no report spells a path the repository's way and an ambiguous tail is dropped rather than guessed. It runs on workflow_run rather than inside Validate: a fork's pull request holds a read-only token and cannot comment, and forks and bots are most of this repository's volume. Nothing in it checks out head-ref code — it reads the artifacts and the pulls API, both data. The baseline is cached from merge-queue entries, which are the merged result about to become main, and written from this workflow so the cache lands on the default branch where pull requests can read it. An absent or malformed report is named in the comment rather than rendered as zero, and the job gates nothing. Closes #1326 --- .../{diff-stats.yml => pr-report-rules.yml} | 3 +- .github/scripts/pr_diff_stats.py | 335 --------- .github/scripts/pr_report.py | 660 ++++++++++++++++++ .github/workflows/pr-diff-stats.yml | 48 -- .github/workflows/pr-report.yml | 115 +++ .github/workflows/validate.yml | 19 + 6 files changed, 796 insertions(+), 384 deletions(-) rename .github/{diff-stats.yml => pr-report-rules.yml} (96%) delete mode 100755 .github/scripts/pr_diff_stats.py create mode 100644 .github/scripts/pr_report.py delete mode 100644 .github/workflows/pr-diff-stats.yml create mode 100644 .github/workflows/pr-report.yml diff --git a/.github/diff-stats.yml b/.github/pr-report-rules.yml similarity index 96% rename from .github/diff-stats.yml rename to .github/pr-report-rules.yml index 066b1fe34..b9a9d300a 100644 --- a/.github/diff-stats.yml +++ b/.github/pr-report-rules.yml @@ -1,4 +1,5 @@ -# Taxonomy for the diff breakdown. Ordered: first match wins. +# Taxonomy for the pull request report's Changes table (.github/scripts/pr_report.py). +# Ordered: first match wins. # `*` stops at a path separator, `**` spans them, a trailing `/**` also matches the dir. # `prod` and the test kinds roll up at the bottom of the block; `generated` is # excluded from every total. diff --git a/.github/scripts/pr_diff_stats.py b/.github/scripts/pr_diff_stats.py deleted file mode 100755 index ad0b62716..000000000 --- a/.github/scripts/pr_diff_stats.py +++ /dev/null @@ -1,335 +0,0 @@ -#!/usr/bin/env python3 -"""Render a per-service, per-category diff breakdown into a pull request body. - -Counts come from the pulls/{n}/files API, which is already a merge-base diff with -renames resolved, so no head checkout is needed. Reads GH_TOKEN, REPO, PR_NUMBER. -Pass --dry-run to render to stdout and write nothing. -""" - -from __future__ import annotations - -import json -import os -import re -import subprocess -import sys -from collections import defaultdict -from pathlib import Path - -START = "" -END = "" - -BAR_WIDTH = 26 -LABEL_WIDTH = 18 -INDENT = 2 -NUM_WIDTH = 7 -FILES_WIDTH = 5 -HEAD_WIDTH = INDENT + LABEL_WIDTH + 1 + BAR_WIDTH -TOTAL_WIDTH = HEAD_WIDTH + NUM_WIDTH * 2 + FILES_WIDTH - -ADD_GLYPH = "█" # full block -DEL_GLYPH = "░" # light shade - -BODY_LIMIT = 65000 - -SERVICE_ORDER = ["api", "frontend", "system-tests", "libs", "platform", "ci", "repo", "docs"] -CATEGORY_ORDER = [ - "prod", - "unit", - "integration", - "e2e", - "system", - "fixtures", - "infra", - "build", - "docs", - "generated", - "other", -] -CATEGORY_LABELS = { - "prod": "production", - "unit": "unit tests", - "integration": "integration tests", - "e2e": "e2e tests", - "system": "system tests", - "fixtures": "test fixtures", - "infra": "infrastructure", - "build": "build & config", - "docs": "docs", - "generated": "generated", - "other": "unclassified", -} - -PROD_CATEGORIES = {"prod"} -TEST_CATEGORIES = {"unit", "integration", "e2e", "system", "fixtures"} -EXCLUDED = {"generated"} - - -def glob_to_regex(pattern: str) -> re.Pattern[str]: - """Translate a path glob to a regex. `**` spans separators, `*` does not.""" - out: list[str] = [] - i, n = 0, len(pattern) - while i < n: - if pattern.startswith("**/", i): - out.append("(?:.*/)?") - i += 3 - elif pattern.startswith("/**", i) and i + 3 == n: - out.append("(?:/.*)?") - i += 3 - elif pattern.startswith("**", i): - out.append(".*") - i += 2 - elif pattern[i] == "*": - out.append("[^/]*") - i += 1 - elif pattern[i] == "?": - out.append("[^/]") - i += 1 - else: - out.append(re.escape(pattern[i])) - i += 1 - return re.compile("^" + "".join(out) + "$") - - -RULE_KEYS = ("glob", "service", "category") - - -def _scalar(value: str, where: str) -> str: - value = value.strip() - if value[:1] in ("'", '"'): - if len(value) < 2 or value[-1] != value[0]: - raise ValueError(f"{where}: unterminated quoted value") - return value[1:-1] - if "#" in value: - raise ValueError(f"{where}: quote any value containing '#'") - return value - - -def parse_rules(text: str, source: str = "") -> list[dict[str, str]]: - """Read a sequence of mappings with three scalar keys, raising on anything else. - - Not PyYAML: it is absent from the runner, and pip installing it would put a - network dependency inside a privileged workflow. - """ - entries: list[dict[str, str]] = [] - for lineno, raw in enumerate(text.splitlines(), start=1): - stripped = raw.strip() - if not stripped or stripped.startswith("#"): - continue - where = f"{source}:{lineno}" - if stripped.startswith("- "): - entries.append({}) - stripped = stripped[2:] - elif not entries: - raise ValueError(f"{where}: mapping before any '-' entry") - key, sep, value = stripped.partition(":") - key = key.strip() - if not sep or key not in RULE_KEYS: - raise ValueError(f"{where}: expected one of {RULE_KEYS}, got {key!r}") - if key in entries[-1]: - raise ValueError(f"{where}: duplicate key {key!r}") - entries[-1][key] = _scalar(value, where) - - for index, entry in enumerate(entries, start=1): - missing = [k for k in RULE_KEYS if k not in entry] - if missing: - raise ValueError(f"{source}: entry {index} is missing {missing}") - if not entries: - raise ValueError(f"{source}: no rules defined") - return entries - - -def load_rules(path: Path) -> list[tuple[re.Pattern[str], str, str]]: - entries = parse_rules(path.read_text(encoding="utf-8"), path.name) - return [(glob_to_regex(e["glob"]), e["service"], e["category"]) for e in entries] - - -def classify(path: str, rules) -> tuple[str, str]: - for pattern, service, category in rules: - if pattern.match(path): - return service, category - return "other", "other" - - -def fetch_files(repo: str, pr: str) -> list[dict]: - """One JSON object per line, so page boundaries do not need stitching.""" - proc = subprocess.run( - ["gh", "api", "--paginate", f"repos/{repo}/pulls/{pr}/files", "--jq", ".[]|@json"], - capture_output=True, - text=True, - check=True, - ) - return [json.loads(line) for line in proc.stdout.splitlines() if line.strip()] - - -def bar(add: int, dele: int, scale: int) -> str: - """Length encodes churn against `scale`, the split encodes added vs removed. - - A generated row can exceed `scale` and is clamped to full width; the split - comes from the row's own ratio so clamping cannot misreport it. - """ - total = add + dele - if scale <= 0 or total <= 0: - return "" - floor = 2 if add > 0 and dele > 0 else 1 # both glyphs need somewhere to go - width = min(BAR_WIDTH, max(floor, round(total / scale * BAR_WIDTH))) - added = round(width * add / total) - if add > 0 and added == 0: - added = 1 - if dele > 0 and added == width: - added = width - 1 - return ADD_GLYPH * added + DEL_GLYPH * (width - added) - - -def plural(count: int, noun: str) -> str: - return f"{count} {noun}" if count == 1 else f"{count} {noun}s" - - -def row(label: str, bar_text: str, add: int, dele: int, files: int, marker: str = "") -> str: - line = ( - " " * INDENT - + label.ljust(LABEL_WIDTH) - + " " - + bar_text.ljust(BAR_WIDTH) - + f"+{add}".rjust(NUM_WIDTH) - + f"-{dele}".rjust(NUM_WIDTH) - + str(files).rjust(FILES_WIDTH) - ) - return (line + marker).rstrip() - - -def render(files: list[dict], rules) -> str: - buckets: dict[tuple[str, str], dict[str, int]] = defaultdict(lambda: {"add": 0, "del": 0, "files": 0}) - for entry in files: - service, category = classify(entry["filename"], rules) - cell = buckets[(service, category)] - cell["add"] += entry.get("additions", 0) - cell["del"] += entry.get("deletions", 0) - cell["files"] += 1 - - if not buckets: - return "" - - def service_key(name: str) -> tuple[int, str]: - if name in SERVICE_ORDER: - return (SERVICE_ORDER.index(name), "") - return (len(SERVICE_ORDER) + (1 if name == "other" else 0), name) - - def category_key(name: str) -> tuple[int, str]: - return (CATEGORY_ORDER.index(name), "") if name in CATEGORY_ORDER else (len(CATEGORY_ORDER), name) - - # Hand-written rows set the scale so generated churn cannot dwarf them, but a - # dependency bump has none, so fall back to generated rather than draw nothing. - scale = max((c["add"] + c["del"] for (_, cat), c in buckets.items() if cat not in EXCLUDED), default=0) - if scale == 0: - scale = max((c["add"] + c["del"] for c in buckets.values()), default=0) - - lines: list[str] = [] - for service in sorted({s for s, _ in buckets}, key=service_key): - categories = sorted((c for s, c in buckets if s == service), key=category_key) - counted = [c for c in categories if c not in EXCLUDED] - head = { - "add": sum(buckets[(service, c)]["add"] for c in counted), - "del": sum(buckets[(service, c)]["del"] for c in counted), - "files": sum(buckets[(service, c)]["files"] for c in counted), - } - lines.append( - service.ljust(HEAD_WIDTH) - + f"+{head['add']}".rjust(NUM_WIDTH) - + f"-{head['del']}".rjust(NUM_WIDTH) - + str(head["files"]).rjust(FILES_WIDTH) - ) - for category in categories: - cell = buckets[(service, category)] - label = CATEGORY_LABELS.get(category, category) - marker = " ~" if category in EXCLUDED else "" - lines.append(row(label, bar(cell["add"], cell["del"], scale), cell["add"], cell["del"], cell["files"], marker)) - lines.append("") - - prod = {k: sum(c[k] for (_, cat), c in buckets.items() if cat in PROD_CATEGORIES) for k in ("add", "del")} - test = {k: sum(c[k] for (_, cat), c in buckets.items() if cat in TEST_CATEGORIES) for k in ("add", "del")} - kept = { - k: sum(c[k] for (_, cat), c in buckets.items() if cat not in EXCLUDED) - for k in ("add", "del", "files") - } - gen = { - k: sum(c[k] for (_, cat), c in buckets.items() if cat in EXCLUDED) - for k in ("add", "del", "files") - } - - def summary(label: str, add: int, dele: int, note: str = "") -> str: - line = label.ljust(HEAD_WIDTH) + f"+{add}".rjust(NUM_WIDTH) + f"-{dele}".rjust(NUM_WIDTH) - return (line + (" " + note if note else "")).rstrip() - - lines.append("─" * TOTAL_WIDTH) - # A dependency bump touches neither, and two zero rows say less than no rows. - if prod["add"] or prod["del"] or test["add"] or test["del"]: - lines.append(summary("production", prod["add"], prod["del"])) - ratio = f"{test['add'] / prod['add']:.2f} test lines per prod line" if prod["add"] else "" - lines.append(summary("tests", test["add"], test["del"], ratio)) - lines.append(summary("total (hand-written)", kept["add"], kept["del"], plural(kept["files"], "file"))) - if gen["files"]: - lines.append(summary("~ generated (excluded)", gen["add"], gen["del"], plural(gen["files"], "file"))) - - table = "\n".join(lines).rstrip() - return ( - f"{START}\n---\n**Diff breakdown** — `{ADD_GLYPH}` added `{DEL_GLYPH}` removed, " - f"scaled to the largest row.\n\n```text\n{table}\n```\n{END}" - ) - - -def splice(body: str, block: str) -> str: - pattern = re.compile(re.escape(START) + ".*?" + re.escape(END), re.DOTALL) - if pattern.search(body): - return pattern.sub(lambda _: block, body, count=1) - return (body.rstrip() + "\n\n" + block) if body.strip() else block - - -def main() -> int: - dry_run = "--dry-run" in sys.argv - repo, pr = os.environ.get("REPO"), os.environ.get("PR_NUMBER") - if not repo or not pr: - print("REPO and PR_NUMBER are required", file=sys.stderr) - return 1 - - rules = load_rules(Path(__file__).resolve().parents[1] / "diff-stats.yml") - files = fetch_files(repo, pr) - if not files: - print("No changed files; leaving the body alone.") - return 0 - - block = render(files, rules) - if dry_run: - print(block) - return 0 - - current = json.loads( - subprocess.run( - ["gh", "api", f"repos/{repo}/pulls/{pr}", "--jq", "{body:.body}"], - capture_output=True, - text=True, - check=True, - ).stdout - )["body"] or "" - - updated = splice(current, block) - if updated == current: - print("Diff breakdown already current.") - return 0 - if len(updated) > BODY_LIMIT: - print("Body would exceed the size limit; skipping.", file=sys.stderr) - return 0 - - subprocess.run( - ["gh", "api", "--method", "PATCH", f"repos/{repo}/pulls/{pr}", "--input", "-"], - input=json.dumps({"body": updated}), - text=True, - check=True, - capture_output=True, - ) - print(f"Updated the diff breakdown on {repo}#{pr}.") - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/.github/scripts/pr_report.py b/.github/scripts/pr_report.py new file mode 100644 index 000000000..3e234f2df --- /dev/null +++ b/.github/scripts/pr_report.py @@ -0,0 +1,660 @@ +#!/usr/bin/env python3 +"""Report a pull request's shape and coverage in one comment. + +Replaces the body splice that `pr_diff_stats.py` did: a comment can carry the +coverage tables too, and it stops a bot rewriting the author's description. + +Two modes. `render` builds the body and upserts the comment, found again by +MARKER so a second push updates one comment rather than leaving a trail. +`summarize` writes the normalised coverage of a run to one JSON file, which is +what a merge-queue run caches for pull requests to compare against. + +Change counts come from the pulls/{n}/files API: already a merge-base diff with +renames resolved, and its `patch` hunks are what patch coverage reads, so no +head checkout is needed anywhere in this script. + +Reads GH_TOKEN, REPO, PR_NUMBER, COVERAGE_DIR, BASELINE_PATH, BASE_REF. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +import xml.etree.ElementTree as ElementTree +from collections import defaultdict +from pathlib import Path + +MARKER = "" + +# Caps, so a large pull request does not bury the coverage under a list of lines. +MAX_FILES = 40 +MAX_LINES_PER_FILE = 15 +BAR_WIDTH = 10 +COMMENT_LIMIT = 65000 + +ADD_GLYPH = "█" +DEL_GLYPH = "░" + +METRICS = ("statements", "branches", "functions", "lines") +METRIC_HEADS = ("Statements", "Branches", "Functions", "Lines") + +# JaCoCo counts instructions and methods where istanbul counts statements and +# functions. Neither pair is the same measurement, but they answer the same +# question per column, which is what lets one table hold both toolchains. +JACOCO_COUNTERS = { + "INSTRUCTION": "statements", + "BRANCH": "branches", + "METHOD": "functions", + "LINE": "lines", +} + +SERVICE_ORDER = ["api", "frontend", "system-tests", "libs", "platform", "ci", "repo", "docs"] +CATEGORY_ORDER = [ + "prod", "unit", "integration", "e2e", "system", + "fixtures", "infra", "build", "docs", "generated", "other", +] +CATEGORY_LABELS = { + "prod": "production", + "unit": "unit tests", + "integration": "integration tests", + "e2e": "e2e tests", + "system": "system tests", + "fixtures": "test fixtures", + "infra": "infrastructure", + "build": "build & config", + "docs": "docs", + "generated": "generated", + "other": "unclassified", +} +PROD_CATEGORIES = {"prod"} +TEST_CATEGORIES = {"unit", "integration", "e2e", "system", "fixtures"} +EXCLUDED = {"generated"} + +# Each suite names the artifact it arrives in, because two of the four carry a +# file called coverage-summary.json and only the directory tells them apart. +SUITES = ( + ("api unit", "api-unit-test-reports", "jacoco", "jacocoTestReport.xml"), + ("api integration", "api-integration-coverage", "jacoco", "jacocoIntegrationTestReport.xml"), + ("frontend unit", "frontend-unit-coverage", "istanbul", "coverage-summary.json"), + ("frontend e2e", "frontend-e2e-coverage", "istanbul", "coverage-summary.json"), +) + + +# -------------------------------------------------------------------------- +# taxonomy — the rules file `pr_diff_stats.py` used, read the same way +# -------------------------------------------------------------------------- + +def glob_to_regex(pattern: str) -> re.Pattern[str]: + """Translate a path glob to a regex. `**` spans separators, `*` does not.""" + out: list[str] = [] + i, n = 0, len(pattern) + while i < n: + if pattern.startswith("**/", i): + out.append("(?:.*/)?") + i += 3 + elif pattern.startswith("/**", i) and i + 3 == n: + out.append("(?:/.*)?") + i += 3 + elif pattern.startswith("**", i): + out.append(".*") + i += 2 + elif pattern[i] == "*": + out.append("[^/]*") + i += 1 + elif pattern[i] == "?": + out.append("[^/]") + i += 1 + else: + out.append(re.escape(pattern[i])) + i += 1 + return re.compile("^" + "".join(out) + "$") + + +RULE_KEYS = ("glob", "service", "category") + + +def _scalar(value: str, where: str) -> str: + value = value.strip() + if value[:1] in ("'", '"'): + if len(value) < 2 or value[-1] != value[0]: + raise ValueError(f"{where}: unterminated quoted value") + return value[1:-1] + if "#" in value: + raise ValueError(f"{where}: quote any value containing '#'") + return value + + +def parse_rules(text: str, source: str = "") -> list[dict[str, str]]: + """Read a sequence of mappings with three scalar keys, raising on anything else. + + Not PyYAML: it is absent from the runner, and pip installing it would put a + network dependency inside a workflow that holds a write token. + """ + entries: list[dict[str, str]] = [] + for lineno, raw in enumerate(text.splitlines(), start=1): + stripped = raw.strip() + if not stripped or stripped.startswith("#"): + continue + where = f"{source}:{lineno}" + if stripped.startswith("- "): + entries.append({}) + stripped = stripped[2:] + elif not entries: + raise ValueError(f"{where}: mapping before any '-' entry") + key, sep, value = stripped.partition(":") + key = key.strip() + if not sep or key not in RULE_KEYS: + raise ValueError(f"{where}: expected one of {RULE_KEYS}, got {key!r}") + if key in entries[-1]: + raise ValueError(f"{where}: duplicate key {key!r}") + entries[-1][key] = _scalar(value, where) + + for index, entry in enumerate(entries, start=1): + missing = [k for k in RULE_KEYS if k not in entry] + if missing: + raise ValueError(f"{source}: entry {index} is missing {missing}") + if not entries: + raise ValueError(f"{source}: no rules defined") + return entries + + +def load_rules(path: Path) -> list[tuple[re.Pattern[str], str, str]]: + entries = parse_rules(path.read_text(encoding="utf-8"), path.name) + return [(glob_to_regex(e["glob"]), e["service"], e["category"]) for e in entries] + + +def classify(path: str, rules) -> tuple[str, str]: + for pattern, service, category in rules: + if pattern.match(path): + return service, category + return "other", "other" + + +# -------------------------------------------------------------------------- +# coverage — two report formats, one shape +# -------------------------------------------------------------------------- + +def _pct(covered: int, total: int) -> float | None: + """None rather than 0.0 for an unmeasured counter: they read very differently.""" + return None if total == 0 else covered / total * 100 + + +def parse_jacoco(xml_text: str) -> tuple[dict[str, float | None], dict[str, set[int]]]: + """A JaCoCo report's totals, and the lines it saw covered, keyed by source tail. + + Only the report-level counters are read for the totals: JaCoCo repeats each + counter at package, class and method level, so summing every `counter` + element would multiply the same numbers several times over. + """ + root = ElementTree.fromstring(xml_text) + totals: dict[str, float | None] = {metric: None for metric in METRICS} + for counter in root.findall("counter"): + metric = JACOCO_COUNTERS.get(counter.get("type", "")) + if metric is None: + continue + missed = int(counter.get("missed", "0")) + covered = int(counter.get("covered", "0")) + totals[metric] = _pct(covered, missed + covered) + + covered_lines: dict[str, set[int]] = defaultdict(set) + seen_lines: dict[str, set[int]] = defaultdict(set) + for package in root.iter("package"): + package_name = package.get("name", "") + for source in package.findall("sourcefile"): + tail = f"{package_name}/{source.get('name', '')}".lstrip("/") + for line in source.findall("line"): + number = int(line.get("nr", "0")) + seen_lines[tail].add(number) + # `ci` is covered instructions on that line; zero means the line + # exists in the bytecode and was never executed. + if int(line.get("ci", "0")) > 0: + covered_lines[tail].add(number) + return totals, _measured(seen_lines, covered_lines) + + +def parse_istanbul(json_text: str) -> dict[str, float | None]: + """The `total` entry of istanbul's json-summary reporter.""" + parsed = json.loads(json_text) + total = parsed.get("total") + if not isinstance(total, dict): + raise ValueError('coverage summary carries no usable "total" entry') + totals: dict[str, float | None] = {} + for metric in METRICS: + entry = total.get(metric) + if isinstance(entry, dict) and isinstance(entry.get("pct"), (int, float)): + totals[metric] = float(entry["pct"]) + else: + totals[metric] = None + return totals + + +def parse_lcov(text: str) -> dict[str, dict[str, set[int]]]: + """Every `DA:` record in an lcov report, as measured and covered line sets.""" + covered: dict[str, set[int]] = defaultdict(set) + seen: dict[str, set[int]] = defaultdict(set) + current: str | None = None + for raw in text.splitlines(): + line = raw.strip() + if line.startswith("SF:"): + current = line[3:].replace("\\", "/") + elif line.startswith("DA:") and current is not None: + number, _, hits = line[3:].partition(",") + try: + position = int(number) + count = int(hits.split(",")[0]) + except ValueError: + continue + seen[current].add(position) + if count > 0: + covered[current].add(position) + elif line == "end_of_record": + current = None + return _measured(seen, covered) + + +def _measured(seen, covered) -> dict[str, dict[str, set[int]]]: + return {path: {"seen": lines, "covered": covered.get(path, set())} for path, lines in seen.items()} + + +def find_one(root: Path, name: str) -> Path | None: + """The first file called `name` under `root`, or None. + + Searched rather than named outright: an artifact's internal layout is + whatever common ancestor the upload step happened to strip. + """ + if not root.is_dir(): + return None + matches = sorted(root.rglob(name)) + return matches[0] if matches else None + + +def read_suites(coverage_dir: Path) -> tuple[dict[str, dict[str, float | None]], dict[str, dict[str, set[int]]], list[str]]: + """Each suite's totals, the union of every line-level report, and what was missing.""" + totals: dict[str, dict[str, float | None]] = {} + measured: dict[str, dict[str, set[int]]] = {} + notes: list[str] = [] + + for label, artifact, kind, filename in SUITES: + root = coverage_dir / artifact + report = find_one(root, filename) + if report is None: + notes.append(f"No coverage reached this report for **{label}** (`{artifact}`).") + continue + try: + if kind == "jacoco": + suite_totals, suite_lines = parse_jacoco(report.read_text(encoding="utf-8")) + else: + suite_totals = parse_istanbul(report.read_text(encoding="utf-8")) + lcov = find_one(root, "lcov.info") + suite_lines = parse_lcov(lcov.read_text(encoding="utf-8")) if lcov else {} + except Exception as error: # a malformed report is reported, never silently zero + notes.append(f"Could not read **{label}** coverage: {error}.") + continue + totals[label] = suite_totals + for path, entry in suite_lines.items(): + existing = measured.setdefault(path, {"seen": set(), "covered": set()}) + existing["seen"] |= entry["seen"] + existing["covered"] |= entry["covered"] + + return totals, measured, notes + + +# -------------------------------------------------------------------------- +# patch coverage — the lines this pull request added, and whether they ran +# -------------------------------------------------------------------------- + +HUNK_HEADER = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@") + + +def added_lines(patch: str) -> list[int]: + """The new-file line numbers a `patch` hunk body adds.""" + lines: list[int] = [] + position = 0 + for raw in patch.splitlines(): + header = HUNK_HEADER.match(raw) + if header: + position = int(header.group(1)) + continue + if raw.startswith("+"): + lines.append(position) + position += 1 + elif raw.startswith("-") or raw.startswith("\\"): + continue + else: + position += 1 + return lines + + +def match_paths(measured: dict, changed: list[str]) -> dict[str, str]: + """Map each coverage path onto the changed file it names, by longest common tail. + + Every report spells a path its own way — JaCoCo by package, lcov by whatever + root the run had — and none of them is repository-relative. Matching on the + tail is what survives that without a per-toolchain prefix table. An ambiguous + tail is dropped rather than guessed. + """ + resolved: dict[str, str] = {} + for path in measured: + tail = path.lstrip("./") + candidates = [c for c in changed if c == tail or c.endswith("/" + tail)] + if len(candidates) != 1: + # Try the other direction: a coverage path carrying a deeper root + # than the repository one (an absolute path from the runner). + candidates = [c for c in changed if tail.endswith("/" + c) or tail == c] + if len(candidates) == 1: + resolved[path] = candidates[0] + return resolved + + +def patch_coverage(files: list[dict], measured: dict) -> tuple[int, int, list[tuple[str, list[int]]]]: + """How many added lines coverage measured, how many it saw run, and which it did not. + + A line counts only when the file it is in appears in a coverage report with a + record for that exact line: a line no report measures (a comment, a blank, a + file outside every `include`) is absent from the denominator rather than + counted as uncovered. + """ + resolved = match_paths(measured, [f["filename"] for f in files]) + by_repo_path: dict[str, dict[str, set[int]]] = {} + for coverage_path, repo_path in resolved.items(): + entry = by_repo_path.setdefault(repo_path, {"seen": set(), "covered": set()}) + entry["seen"] |= measured[coverage_path]["seen"] + entry["covered"] |= measured[coverage_path]["covered"] + + total = 0 + covered = 0 + uncovered: list[tuple[str, list[int]]] = [] + for entry in files: + name = entry["filename"] + record = by_repo_path.get(name) + if record is None or not entry.get("patch"): + continue + missing: list[int] = [] + for line in added_lines(entry["patch"]): + if line not in record["seen"]: + continue + total += 1 + if line in record["covered"]: + covered += 1 + else: + missing.append(line) + if missing: + uncovered.append((name, missing)) + uncovered.sort(key=lambda item: (-len(item[1]), item[0])) + return covered, total, uncovered + + +# -------------------------------------------------------------------------- +# rendering — one pure function over plain data +# -------------------------------------------------------------------------- + +def _cell(glyph: str, value: int, scale: int) -> str: + """Empty rather than a zero, so the eye goes to the rows that moved.""" + if value == 0: + return "" + blocks = max(1, round(value / scale * BAR_WIDTH)) if scale > 0 else 1 + return f"{glyph * blocks} {value}" + + +def _plural(count: int, noun: str) -> str: + return f"{count} {noun}" if count == 1 else f"{count} {noun}s" + + +def _pct_text(value: float | None) -> str: + return "—" if value is None else f"{value:.2f}%" + + +def _delta_text(current: float | None, base: float | None) -> str: + if current is None or base is None: + return "—" + difference = current - base + if abs(difference) < 0.005: + return "0.00" + return f"{difference:+.2f}" + + +def changes_section(files: list[dict], rules) -> str: + buckets: dict[tuple[str, str], dict[str, int]] = defaultdict(lambda: {"add": 0, "del": 0, "files": 0}) + for entry in files: + service, category = classify(entry["filename"], rules) + cell = buckets[(service, category)] + cell["add"] += entry.get("additions", 0) + cell["del"] += entry.get("deletions", 0) + cell["files"] += 1 + if not buckets: + return "## Changes\n\nNo files changed." + + def service_key(name: str) -> tuple[int, str]: + if name in SERVICE_ORDER: + return (SERVICE_ORDER.index(name), "") + return (len(SERVICE_ORDER) + (1 if name == "other" else 0), name) + + def category_key(name: str) -> tuple[int, str]: + return (CATEGORY_ORDER.index(name), "") if name in CATEGORY_ORDER else (len(CATEGORY_ORDER), name) + + # Hand-written rows set the scale so generated churn cannot dwarf them, but a + # dependency bump has none, so fall back to generated rather than draw nothing. + scale = max((c["add"] + c["del"] for (_, cat), c in buckets.items() if cat not in EXCLUDED), default=0) + if scale == 0: + scale = max((c["add"] + c["del"] for c in buckets.values()), default=1) + + rows = ["| Bucket | Files | Added | Removed |", "|---|---:|---:|---:|"] + for service in sorted({s for s, _ in buckets}, key=service_key): + for category in sorted((c for s, c in buckets if s == service), key=category_key): + cell = buckets[(service, category)] + label = f"{service} · {CATEGORY_LABELS.get(category, category)}" + mark = " ~" if category in EXCLUDED else "" + rows.append( + f"| {label}{mark} | {cell['files']} " + f"| {_cell(ADD_GLYPH, cell['add'], scale)} | {_cell(DEL_GLYPH, cell['del'], scale)} |" + ) + + kept = {k: sum(c[k] for (_, cat), c in buckets.items() if cat not in EXCLUDED) for k in ("add", "del", "files")} + generated = any(cat in EXCLUDED for _, cat in buckets) + total_label = f"**total**{' (generated excluded)' if generated else ''}" + # In the table rather than a sentence under it, so the column a reader is + # already scanning is where the total is. + rows.append(f"| {total_label} | {kept['files']} | +{kept['add']} | −{kept['del']} |") + + prod = sum(c["add"] for (_, cat), c in buckets.items() if cat in PROD_CATEGORIES) + tests = sum(c["add"] for (_, cat), c in buckets.items() if cat in TEST_CATEGORIES) + if prod == 0: + ratio = "**No production lines added.**" if tests else "**No production or test lines added.**" + else: + ratio = f"**{tests / prod:.2f} test lines per prod line.**" + + return "\n\n".join(["## Changes", "\n".join(rows), ratio]) + + +def coverage_section(totals, baseline, base_ref: str) -> str: + if not totals: + return "## Coverage\n\nNo coverage summary reached this report." + + head = "| | " + " | ".join(METRIC_HEADS) + " |" + rows = [head, "|---|---:|---:|---:|---:|"] + base_totals = (baseline or {}).get("suites", {}) + commit = (baseline or {}).get("commit") + base_label = f"{base_ref}" + (f" `{commit[:7]}`" if commit else "") + + for label, _, _, _ in SUITES: + suite = totals.get(label) + if suite is None: + continue + rows.append(f"| **{label}** | " + " | ".join(_pct_text(suite[m]) for m in METRICS) + " |") + base_suite = base_totals.get(label) + if base_suite is None: + rows.append(f"| {base_label} | " + " | ".join("—" for _ in METRICS) + " |") + else: + rows.append(f"| {base_label} | " + " | ".join(_pct_text(base_suite.get(m)) for m in METRICS) + " |") + rows.append("| Δ | " + " | ".join(_delta_text(suite[m], base_suite.get(m)) for m in METRICS) + " |") + + parts = ["## Coverage", "\n".join(rows)] + if baseline is None: + parts.append(f"No baseline is cached from `{base_ref}` yet, so there is nothing to compare against.") + return "\n\n".join(parts) + + +def patch_section(covered: int, total: int, uncovered) -> list[str]: + if total == 0: + return ["**Patch coverage:** this pull request changes no line that coverage measures."] + headline = ( + f"**Patch coverage: {covered / total * 100:.1f}%**, " + f"{covered} of {total} changed lines covered." + ) + if not uncovered: + return [headline] + + missed = sum(len(lines) for _, lines in uncovered) + shown = uncovered[:MAX_FILES] + listed = [] + for path, lines in shown: + head_lines = ", ".join(str(line) for line in lines[:MAX_LINES_PER_FILE]) + rest = len(lines) - MAX_LINES_PER_FILE + listed.append(f"`{path}` {head_lines}{f' and {rest} more' if rest > 0 else ''}") + hidden = len(uncovered) - len(shown) + if hidden > 0: + listed.append(f"and {_plural(hidden, 'file')} more") + + details = "\n".join([ + f"
{_plural(missed, 'uncovered line')} in this pull request", + "", + "\n".join(listed), + "", + "
", + ]) + return [headline, details] + + +def render(files, rules, totals, baseline, base_ref, patch, notes) -> str: + covered, total, uncovered = patch + sections = [ + changes_section(files, rules), + coverage_section(totals, baseline, base_ref), + *patch_section(covered, total, uncovered), + # One blockquote rather than one per note, so a run missing three + # artifacts does not read as three separate complaints. + "\n".join(f"> {note}" for note in notes), + MARKER, + ] + body = "\n\n".join(part for part in sections if part) + if len(body) > COMMENT_LIMIT: + # Drop the uncovered-line list first: it is the longest part and the + # least load-bearing of the three. + sections = [ + changes_section(files, rules), + coverage_section(totals, baseline, base_ref), + patch_section(covered, total, [])[0], + "> The list of uncovered lines was too long for one comment.", + MARKER, + ] + body = "\n\n".join(part for part in sections if part) + return body + + +# -------------------------------------------------------------------------- +# the impure edges +# -------------------------------------------------------------------------- + +def gh(args: list[str], stdin: str | None = None) -> str: + result = subprocess.run( + ["gh", *args], capture_output=True, text=True, input=stdin, check=True, + ) + return result.stdout + + +def fetch_files(repo: str, pr: str) -> list[dict]: + """One JSON object per line, so page boundaries do not need stitching.""" + out = gh(["api", "--paginate", f"repos/{repo}/pulls/{pr}/files", "--jq", ".[]|@json"]) + return [json.loads(line) for line in out.splitlines() if line.strip()] + + +def upsert_comment(repo: str, pr: str, body: str) -> str: + out = gh(["api", "--paginate", f"repos/{repo}/issues/{pr}/comments", "--jq", ".[]|@json"]) + for line in out.splitlines(): + if not line.strip(): + continue + comment = json.loads(line) + if MARKER in (comment.get("body") or ""): + if (comment.get("body") or "") == body: + return "unchanged" + gh(["api", "--method", "PATCH", f"repos/{repo}/issues/comments/{comment['id']}", + "--input", "-"], stdin=json.dumps({"body": body})) + return "updated" + gh(["api", f"repos/{repo}/issues/{pr}/comments", "--input", "-"], + stdin=json.dumps({"body": body})) + return "posted" + + +def read_baseline(path: str | None) -> dict | None: + if not path: + return None + file = Path(path) + if not file.is_file(): + return None + try: + parsed = json.loads(file.read_text(encoding="utf-8")) + except (OSError, ValueError): + return None + return parsed if isinstance(parsed.get("suites"), dict) else None + + +def command_summarize(coverage_dir: Path, out_path: Path, commit: str) -> int: + """Write this run's coverage as the baseline a later pull request compares against.""" + totals, _, notes = read_suites(coverage_dir) + for note in notes: + print(note, file=sys.stderr) + if not totals: + print("No coverage found; writing no baseline.", file=sys.stderr) + return 1 + out_path.parent.mkdir(parents=True, exist_ok=True) + out_path.write_text(json.dumps({"commit": commit, "suites": totals}, indent=2), encoding="utf-8") + print(f"Wrote a baseline for {len(totals)} suite(s) to {out_path}.") + return 0 + + +def command_render(repo: str, pr: str, coverage_dir: Path, baseline_path: str | None, + base_ref: str, dry_run: bool) -> int: + rules = load_rules(Path(__file__).resolve().parents[1] / "pr-report-rules.yml") + files = fetch_files(repo, pr) + totals, measured, notes = read_suites(coverage_dir) + baseline = read_baseline(baseline_path) + patch = patch_coverage(files, measured) + body = render(files, rules, totals, baseline, base_ref, patch, notes) + + if dry_run: + print(body) + return 0 + print(f"Comment {upsert_comment(repo, pr, body)} on {repo}#{pr}.") + return 0 + + +def main() -> int: + args = [a for a in sys.argv[1:] if not a.startswith("-")] + mode = args[0] if args else "render" + dry_run = "--dry-run" in sys.argv + coverage_dir = Path(os.environ.get("COVERAGE_DIR", "coverage-artifacts")) + + if mode == "summarize": + return command_summarize( + coverage_dir, + Path(os.environ.get("BASELINE_PATH", "coverage-baseline/baseline.json")), + os.environ.get("BASELINE_COMMIT", ""), + ) + + repo, pr = os.environ.get("REPO"), os.environ.get("PR_NUMBER") + if not repo or not pr: + print("REPO and PR_NUMBER are required", file=sys.stderr) + return 1 + return command_render( + repo, pr, coverage_dir, + os.environ.get("BASELINE_PATH"), + os.environ.get("BASE_REF", "main"), + dry_run, + ) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/workflows/pr-diff-stats.yml b/.github/workflows/pr-diff-stats.yml deleted file mode 100644 index e9bf2d630..000000000 --- a/.github/workflows/pr-diff-stats.yml +++ /dev/null @@ -1,48 +0,0 @@ -name: PR diff stats - -# Renders a per-service, per-category breakdown of the diff into the bottom of -# the pull request body. -# -# `pull_request_target` rather than `pull_request`: the latter hands out a -# read-only token for bot and fork pull requests, which is most of this -# repo's volume, and the body cannot be written with it. The elevated token is -# safe here only because the job never checks out or executes head-ref code — -# the diff is read from the pulls/{n}/files API, which also gives merge-base -# semantics and pre-resolved renames for free. Do not add a `ref:` to the -# checkout step below. - -on: - pull_request_target: - types: [opened, synchronize, reopened, edited] - -concurrency: - group: diff-stats-${{ github.event.pull_request.number }} - cancel-in-progress: true - -permissions: - contents: read - -jobs: - diff-stats: - name: Annotate PR body with diff breakdown - runs-on: ubuntu-latest - timeout-minutes: 5 - # Stops this job's own body edit from retriggering on `edited`. Dependency - # pull requests are included: separating a hand-written fix from lockfile - # churn is exactly what the generated split is for. Their token stays - # read/write because the base ref is not itself bot-authored. - if: github.event.sender.login != 'github-actions[bot]' - permissions: - contents: read - pull-requests: write - steps: - # No `ref:` — this must stay on the trusted base ref. See the note above. - - name: Check out the base ref - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - - name: Render and splice the diff breakdown - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - run: python3 .github/scripts/pr_diff_stats.py diff --git a/.github/workflows/pr-report.yml b/.github/workflows/pr-report.yml new file mode 100644 index 000000000..f1f60f5c5 --- /dev/null +++ b/.github/workflows/pr-report.yml @@ -0,0 +1,115 @@ +name: PR report + +# Reports a pull request's shape and its coverage in one comment, found again +# by a marker and updated in place. It gates nothing: the coverage floors in +# the Gradle build and the vitest config are the enforcement, and this only +# says what the numbers are. +# +# `workflow_run` rather than a job inside Validate: a fork's pull request runs +# Validate with a read-only token and cannot comment, and forks plus bots are +# most of this repository's volume. A `workflow_run` job runs in the base +# repository with a write token. That is only safe because nothing here checks +# out or executes head-ref code — it reads the artifacts Validate uploaded and +# the pulls API, both of which are data. Do not add a checkout of the head ref. +# +# The same workflow keeps the baseline current: a merge-queue entry is the +# merged result about to become main, so its coverage is main's next coverage. +# Caching it from here rather than from Validate is deliberate — a +# `workflow_run` job runs on the default branch, so the cache it writes is +# visible to every pull request, which one written from a +# `gh-readonly-queue/**` ref would not be. + +on: + workflow_run: + workflows: [ Validate ] + types: [ completed ] + +concurrency: + group: pr-report-${{ github.event.workflow_run.id }} + cancel-in-progress: true + +permissions: + contents: read + +env: + BASELINE_KEY: coverage-baseline-main + +jobs: + report: + name: Comment the pull request's shape and coverage + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + actions: read + pull-requests: write + steps: + # No `ref:` — this must stay on the trusted default branch. See above. + - name: Check out the default branch + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + # Every artifact is optional. A shard that failed uploads nothing, and a + # report that names the gap is worth more than a job that fails next to + # an already-failing suite. + - name: Download whatever coverage the run produced + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + RUN_ID: ${{ github.event.workflow_run.id }} + run: | + mkdir -p coverage-artifacts + for artifact in api-unit-test-reports api-integration-coverage \ + frontend-unit-coverage frontend-e2e-coverage pr-report-meta; do + gh run download "$RUN_ID" --repo "$GITHUB_REPOSITORY" \ + --name "$artifact" --dir "coverage-artifacts/$artifact" \ + || echo "no $artifact on run $RUN_ID" + done + + - name: Restore the coverage baseline cached from main + id: baseline + uses: actions/cache/restore@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 + with: + path: coverage-baseline + key: ${{ env.BASELINE_KEY }}-${{ github.event.workflow_run.head_sha }} + restore-keys: ${{ env.BASELINE_KEY }}- + + - name: Comment on the pull request + if: github.event.workflow_run.event == 'pull_request' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + COVERAGE_DIR: coverage-artifacts + BASELINE_PATH: coverage-baseline/baseline.json + BASE_REF: ${{ github.event.repository.default_branch }} + run: | + set -euo pipefail + number_file=coverage-artifacts/pr-report-meta/pr-number + if [ ! -f "$number_file" ]; then + echo "The run carried no pull request number; nothing to comment on." + exit 0 + fi + PR_NUMBER=$(tr -dc '0-9' < "$number_file") + export PR_NUMBER + [ -n "$PR_NUMBER" ] || { echo "Empty pull request number."; exit 0; } + python3 .github/scripts/pr_report.py render + + # Only a merge-queue entry writes the baseline, and only a green one: the + # numbers from a run whose suites failed are not what main will have. + - name: Cache this merge-queue entry's coverage as the baseline + if: | + github.event.workflow_run.event == 'merge_group' + && github.event.workflow_run.conclusion == 'success' + env: + COVERAGE_DIR: coverage-artifacts + BASELINE_PATH: coverage-baseline/baseline.json + BASELINE_COMMIT: ${{ github.event.workflow_run.head_sha }} + run: python3 .github/scripts/pr_report.py summarize + + - name: Save the baseline + if: | + github.event.workflow_run.event == 'merge_group' + && github.event.workflow_run.conclusion == 'success' + && hashFiles('coverage-baseline/baseline.json') != '' + uses: actions/cache/save@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 + with: + path: coverage-baseline + key: ${{ env.BASELINE_KEY }}-${{ github.event.workflow_run.head_sha }} diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 72d233bbb..b57dab552 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -151,6 +151,25 @@ jobs: echo "images=true" >> "$GITHUB_OUTPUT" fi + # The pull request number, for the report workflow that runs after this + # one. `workflow_run.pull_requests` is empty for a fork's pull request, + # and forks are most of this repository's volume, so it is carried here + # rather than looked up there. + - name: Record which pull request this run is for + if: github.event_name == 'pull_request' + run: | + mkdir -p pr-report-meta + echo "${{ github.event.pull_request.number }}" > pr-report-meta/pr-number + + - name: Upload the pull request number + if: github.event_name == 'pull_request' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: pr-report-meta + path: pr-report-meta/pr-number + retention-days: 1 + if-no-files-found: error + nix-flake-check: name: NixOS flake check needs: changes