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
22 changes: 16 additions & 6 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@
)
from pr_review_rounds import (
ACK_MARKER,
CARRIED_PARTIAL,
CODERABBIT_LOGIN,
FILES_UNREAD,
KNOWN_REVIEWERS,
Expand Down Expand Up @@ -994,12 +995,21 @@ def report_verdict(pr: dict) -> int:
return 0
state, why = coverage(pr)
if state == "partial":
print(
f"status=COVERAGE_IS_PARTIAL the review covering the head states it read fewer files "
f"than the pull request changed ({why}), so part of the diff has no review at all. A "
"re-request is not the remedy it reads as. Splitting is the remedy where it applies, "
"and it does not apply to a promotion. Otherwise this is the maintainer's call"
)
if why.startswith(CARRIED_PARTIAL):
print(
f"status=COVERAGE_IS_PARTIAL the head reads as partial because {why}, so part of "
"the diff may have no review. A round on the head stating full coverage clears "
"this, unless a round on it already states a reading this cannot believe. "
"Otherwise this is the maintainer's call"
)
else:
print(
f"status=COVERAGE_IS_PARTIAL the review covering the head states it read fewer "
f"files than the pull request changed ({why}), so part of the diff has no review "
"at all. A re-request is not the remedy it reads as. Splitting is the remedy where "
"it applies, and it does not apply to a promotion. Otherwise this is the "
"maintainer's call"
)
return 42
if flagged := uncounted_verdict(pr):
print(
Expand Down
3 changes: 2 additions & 1 deletion scripts/pr_review_rounds.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
)
TRUSTED_ASSOCIATIONS = frozenset({"OWNER", "MEMBER", "COLLABORATOR"})
ACK_MARKER = "<!-- fleet-shape-ack: fingerprint={} -->"
CARRIED_PARTIAL = "an earlier round states partial coverage"


def reviewer_requested(pr: dict) -> bool:
Expand Down Expand Up @@ -437,7 +438,7 @@ def coverage(pr: dict) -> tuple[str, str]:
if worst
else "no round on the head states any"
)
return "partial", f"an earlier round states partial coverage, and {none}"
return "partial", f"{CARRIED_PARTIAL}, and {none}"
if worst:
return worst
why = ""
Expand Down
17 changes: 16 additions & 1 deletion tests/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -1989,12 +1989,27 @@ def test_a_head_round_stating_partial_coverage_exits_forty_two(self) -> None:
self.assertEqual(42, self.status(payload([review(body=part)], files=["a.py"])))
out = self.out.getvalue()
self.assertIn("coverage=PARTIAL ", out)
self.assertIn("status=COVERAGE_IS_PARTIAL", out)
self.assertIn("status=COVERAGE_IS_PARTIAL the review covering the head states it", out)
self.assertNotIn("may have no review", out)

def test_an_earlier_partial_carries_to_a_head_stating_none(self) -> None:
part = f"{OVERVIEW}\n<!-- fleet-review: reviewed=1 changed=3 findings=0 -->"
rounds = [review(oid=OLD, body=part, rid="A"), review(body=OVERVIEW, rid="B")]
self.assertEqual(42, self.status(payload(rounds, files=["a.py"])))
line = next(ln for ln in self.out.getvalue().splitlines() if ln.startswith("status="))
self.assertIn("because an earlier round states partial coverage", line)
self.assertNotIn("the review covering the head states", line)
self.assertIn("A round on the head stating full coverage clears this", line)
self.assertNotIn("A re-request is not the remedy", line)

def test_an_earlier_partial_carried_past_an_unbelievable_head_says_so(self) -> None:
part = f"{OVERVIEW}\n<!-- fleet-review: reviewed=1 changed=3 findings=0 -->"
odd = f"{OVERVIEW}\n<!-- fleet-review: reviewed=4 changed=3 findings=0 -->"
rounds = [review(oid=OLD, body=part, rid="A"), review(body=odd, rid="B")]
self.assertEqual(42, self.status(payload(rounds, files=["a.py"])))
line = next(ln for ln in self.out.getvalue().splitlines() if ln.startswith("status="))
self.assertIn("a round on the head states a reading this cannot believe", line)
self.assertNotIn("the review covering the head states", line)

def test_a_prose_partial_on_an_earlier_round_keeps_a_table_from_standing_in(self) -> None:
part = f"{OVERVIEW}\nCopilot reviewed 1 out of 2 changed files.\n"
Expand Down
Loading