Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 9 additions & 66 deletions src/scripts/testbot/create_pr.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
from typing import Any

from src.scripts.testbot.guardrails import get_changed_test_files
from src.scripts.testbot.verify_coverage import (
render_markdown,
reports_from_json,
)

logging.basicConfig(
level=logging.INFO,
Expand Down Expand Up @@ -322,79 +326,18 @@ def _build_generator_summary_section(path: str) -> str:
def _build_coverage_section(path: str) -> str:
"""Render the coverage-gain section from verify_coverage.py's JSON.

The verifier writes one entry per picker target with the listed-line
hit count and per-range outcome. Missing / unreadable reports yield
an empty string so the PR opens unchanged. Below-threshold files get
a ⚠️ marker so reviewers can scan the gap at a glance — for example
``2/121 lines (2%)`` should jump out the way the roles.go PR did.
Missing / unreadable reports yield an empty string so the PR still
opens. Rendering itself lives in verify_coverage so the PR body and
the generator's self-check report cannot drift apart.
"""
if not path:
return ""
try:
reports = json.loads(Path(path).read_text(encoding="utf-8"))
payload = json.loads(Path(path).read_text(encoding="utf-8"))
except (OSError, json.JSONDecodeError) as exc:
logger.warning("Could not read coverage report %s: %s", path, exc)
return ""
if not isinstance(reports, list) or not reports:
return ""

lines: list[str] = ["## Coverage gain on listed uncovered ranges", ""]
for entry in reports:
if not isinstance(entry, dict):
continue
file_path = entry.get("file_path", "")
if not file_path:
continue
# Fail-soft on malformed values so a stray string in one field can't
# abort PR creation — we'd rather show 0 and let the reviewer see the
# gap than block the whole pipeline on a typo upstream.
try:
listed = int(entry.get("listed_lines", 0) or 0)
except (TypeError, ValueError):
listed = 0
try:
hit = int(entry.get("hit_lines", 0) or 0)
except (TypeError, ValueError):
hit = 0
try:
hit_fraction = float(entry.get("hit_fraction", 0.0) or 0.0)
except (TypeError, ValueError):
hit_fraction = 0.0
passed = bool(entry.get("passed", False))
lcov_seen = bool(entry.get("lcov_seen", True))
if not lcov_seen:
marker = "❔"
note = (
" — file not found in LCOV (test target may not have run; "
"the harness ran `bazel coverage` over //... after generation)"
)
else:
marker = "✅" if passed else "⚠️"
note = ""
lines.append(
f"{marker} **`{file_path}`** — "
f"{hit}/{listed} listed lines hit ({hit_fraction * 100:.0f}%)"
f"{note}"
)
# Detail per range so reviewers can spot which specific blocks
# the bot missed without leaving the PR view.
for r in entry.get("ranges", []) or []:
if not isinstance(r, dict):
continue
start = r.get("start")
end = r.get("end")
hit_lines = r.get("hit_lines", 0)
total_lines = r.get("total_lines", 0)
covered = bool(r.get("covered", False))
if start is None or end is None:
continue
span = f"line {start}" if start == end else f"lines {start}-{end}"
check = "✅" if covered else "❌"
lines.append(
f" - {check} {span} — {hit_lines}/{total_lines} hit"
)
lines.append("")
return "\n".join(lines).rstrip() + "\n"
return render_markdown(reports_from_json(payload))
Comment thread
jiaenren marked this conversation as resolved.


def _build_rationale_section(meta: dict[str, dict]) -> str:
Expand Down
45 changes: 36 additions & 9 deletions src/scripts/testbot/tests/test_create_pr.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import unittest
from unittest.mock import patch

from src.scripts.testbot.verify_coverage import MAX_REPORTED_RANGES
from src.scripts.testbot.create_pr import ( # noqa: E501
_build_generator_summary_section,
SLACK_API_URL,
Expand Down Expand Up @@ -1151,9 +1152,10 @@ def test_renders_pass_target_with_check_marker(self):
self.assertIn("8/10", section)
self.assertIn("80%", section)
self.assertIn("✅", section)
# Per-range detail should be in the body so reviewers can scan
# which blocks were missed without leaving the PR.
self.assertIn("lines 90-99", section)
# Covered ranges are not listed. A checklist of all-✅ bullets ran to
# ~300 lines on a 3-target PR and buried the summary lines above.
self.assertNotIn("lines 90-99", section)
self.assertNotIn("still uncovered", section)

def test_below_threshold_target_gets_warning_marker(self):
path = self._write_report([
Expand Down Expand Up @@ -1228,24 +1230,49 @@ def test_single_line_range_renders_singular_form(self):
{
"file_path": "src/lib/foo.py",
"listed_lines": 1,
"hit_lines": 1,
"hit_fraction": 1.0,
"passed": True,
"hit_lines": 0,
"hit_fraction": 0.0,
"passed": False,
"lcov_seen": True,
"ranges": [
{"start": 5, "end": 5,
"hit_lines": 1, "total_lines": 1, "covered": True},
"hit_lines": 0, "total_lines": 1, "covered": False},
],
"still_uncovered_ranges": [],
"still_uncovered_ranges": [[5, 5]],
},
])
try:
section = _build_coverage_section(path)
finally:
os.unlink(path)
self.assertIn("line 5", section)
self.assertIn("still uncovered: line 5", section)
self.assertNotIn("lines 5-5", section)

def test_still_uncovered_ranges_are_capped(self):
misses = [
{"start": n, "end": n, "hit_lines": 0, "total_lines": 1,
"covered": False}
for n in range(1, 15)
]
path = self._write_report([
{
"file_path": "src/lib/foo.py",
"listed_lines": 14,
"hit_lines": 0,
"hit_fraction": 0.0,
"passed": False,
"lcov_seen": True,
"ranges": misses,
"still_uncovered_ranges": [[n, n] for n in range(1, 15)],
},
])
try:
section = _build_coverage_section(path)
finally:
os.unlink(path)
self.assertIn(f"and {len(misses) - MAX_REPORTED_RANGES} more", section)
self.assertNotIn("line 14", section)


if __name__ == "__main__":
unittest.main()
53 changes: 45 additions & 8 deletions src/scripts/testbot/verify_coverage.py
Original file line number Diff line number Diff line change
Expand Up @@ -249,15 +249,52 @@ def build_reports(
return reports


def reports_from_json(payload: object) -> list[TargetReport]:
"""Rebuild reports from the JSON written by ``render_json``.

Fail-soft: malformed entries and fields are skipped or coerced to 0 so
a bad value upstream degrades the PR body instead of blocking the PR.
"""
if not isinstance(payload, list):
return []

def _int(value: object) -> int:
if not isinstance(value, (int, float, str)):
return 0
try:
return int(value)
except ValueError:
return 0

reports: list[TargetReport] = []
for entry in payload:
if not isinstance(entry, dict) or not entry.get("file_path"):
continue
ranges = []
for raw in entry.get("ranges") or []:
if not isinstance(raw, dict):
continue
start, end = raw.get("start"), raw.get("end")
if start is None or end is None:
continue
ranges.append(RangeResult(
_int(start), _int(end),
_int(raw.get("hit_lines")), _int(raw.get("total_lines")),
))
reports.append(TargetReport(
file_path=str(entry["file_path"]),
listed_lines=_int(entry.get("listed_lines")),
hit_lines=_int(entry.get("hit_lines")),
ranges=ranges,
lcov_seen=bool(entry.get("lcov_seen", True)),
Comment thread
jiaenren marked this conversation as resolved.
))
return reports


def render_markdown(reports: list[TargetReport]) -> str:
"""Render a Markdown coverage-gain section for the PR body.

Mirrors the picker-rationale block format already used by
``create_pr.py``: one heading and one row per target. Only the ranges
still missing coverage are named — a per-range checklist of mostly-✅
entries ran to dozens of lines and buried the number that matters.
The numbers come from the same JSON the LLM consumed during
iteration, so the PR description and the generator's view agree.
"""Render the Markdown coverage-gain section for the PR body.

One row per target, naming only the ranges still missing coverage.
"""
if not reports:
return ""
Expand Down
Loading