nisse-review: a commit diff Gitea can't give falls back to the whole diff
From nisse-review's review of #156: only a failed listing fell back to the PR's whole diff; one commit's diff failing (a force-push dropped it) failed the run. Tests also pin down its other three points, which the code already handled: a command is taken in once, a second review at the same head has its own marker, and `review` beside a comment is one fresh review. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
This commit is contained in:
1 parent
fca4c41d95
commit
e107484042
2 files changed
+60
-7
No files matched your search
@@ -384,23 +384,24 @@ class Reviewer:
|
|||||||
|
|
||||||
def _changes(self, repo: str, pr: PullRequest, since: str) -> str:
|
def _changes(self, repo: str, pr: PullRequest, since: str) -> str:
|
||||||
"""The commits since ``since``, each with its diff; the whole PR's
|
"""The commits since ``since``, each with its diff; the whole PR's
|
||||||
diff if Gitea can't list them (``since`` gone, say)."""
|
diff if Gitea can't list them or give one's diff (``since`` gone
|
||||||
|
after a force-push, say)."""
|
||||||
try:
|
try:
|
||||||
commits = self._gitea.commits_between(repo, since, pr.head_sha)
|
commits = self._gitea.commits_between(repo, since, pr.head_sha)
|
||||||
|
diffs = "\n".join(
|
||||||
|
f"commit {line}\n{self._gitea.commit_diff(repo, line.split()[0])}"
|
||||||
|
for line in commits
|
||||||
|
)
|
||||||
except GiteaError as exc:
|
except GiteaError as exc:
|
||||||
self._oplog.emit("review.compare_failed", cause=str(exc)[:300])
|
self._oplog.emit("review.compare_failed", cause=str(exc)[:300])
|
||||||
commits = []
|
commits = []
|
||||||
if not commits:
|
if not commits:
|
||||||
return (
|
return (
|
||||||
"Gitea couldn't list the commits since then (a force-push?): "
|
"Gitea couldn't give the commits since then (a force-push?): "
|
||||||
"the whole PR's diff instead.\n"
|
"the whole PR's diff instead.\n"
|
||||||
+ self._cut(self._gitea.pull_request_diff(repo, pr.number))
|
+ self._cut(self._gitea.pull_request_diff(repo, pr.number))
|
||||||
+ "\n\n"
|
+ "\n\n"
|
||||||
)
|
)
|
||||||
diffs = "\n".join(
|
|
||||||
f"commit {line}\n{self._gitea.commit_diff(repo, line.split()[0])}"
|
|
||||||
for line in commits
|
|
||||||
)
|
|
||||||
return (
|
return (
|
||||||
"The commits since:\n"
|
"The commits since:\n"
|
||||||
+ "\n".join(commits)
|
+ "\n".join(commits)
|
||||||
|
|||||||
@@ -136,6 +136,22 @@ def test_without_the_commits_since_it_reads_the_whole_diff(tmp_path: Path) -> No
|
|||||||
assert "the whole PR's diff instead" in last_brief(w)
|
assert "the whole PR's diff instead" in last_brief(w)
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_commit_whose_diff_cannot_be_read_falls_back_too(tmp_path: Path) -> None:
|
||||||
|
# nisse-review on #156: only a failed listing fell back; a failed diff
|
||||||
|
# cost a run, in the force-push case the fallback is for.
|
||||||
|
class Gone(FakeGitea):
|
||||||
|
def commit_diff(self, repo: str, sha: str) -> str:
|
||||||
|
raise GiteaError("404: no such commit")
|
||||||
|
|
||||||
|
gitea = Gone(repos={REPO: [pr()]})
|
||||||
|
w = world(tmp_path, Scripted(*REVIEW, [call("post_review", summary="ok")]), gitea)
|
||||||
|
w["scheduler"].round()
|
||||||
|
push(w)
|
||||||
|
w["scheduler"].round()
|
||||||
|
assert "the whole PR's diff instead" in last_brief(w)
|
||||||
|
assert item(w).position == REVIEWED
|
||||||
|
|
||||||
|
|
||||||
def test_a_sign_off_is_said_once_while_it_stays_happy(tmp_path: Path) -> None:
|
def test_a_sign_off_is_said_once_while_it_stays_happy(tmp_path: Path) -> None:
|
||||||
gitea = FakeGitea(repos={REPO: [pr()]})
|
gitea = FakeGitea(repos={REPO: [pr()]})
|
||||||
model = Scripted(
|
model = Scripted(
|
||||||
@@ -218,7 +234,9 @@ def test_stop_quiets_it_until_review_asks_again(tmp_path: Path) -> None:
|
|||||||
push(w)
|
push(w)
|
||||||
assert w["scheduler"].round() is False
|
assert w["scheduler"].round() is False
|
||||||
assert item(w).position == STOPPED
|
assert item(w).position == STOPPED
|
||||||
assert (stop, "+1") in w["gitea"].reactions
|
assert w["gitea"].reactions == [(stop, "+1")]
|
||||||
|
w["scheduler"].round()
|
||||||
|
assert w["gitea"].reactions == [(stop, "+1")] # taken in: said once
|
||||||
w["gitea"].say("don", "Review.")
|
w["gitea"].say("don", "Review.")
|
||||||
assert w["scheduler"].round() is True
|
assert w["scheduler"].round() is True
|
||||||
assert "Review o/demo#7" in last_brief(w) # the whole PR, afresh
|
assert "Review o/demo#7" in last_brief(w) # the whole PR, afresh
|
||||||
@@ -357,3 +375,37 @@ def test_a_summary_answers_nothing_and_a_first_review_answers_no_points(
|
|||||||
w["scheduler"].round()
|
w["scheduler"].round()
|
||||||
assert "reply" not in model.offered[-1] and "post_summary" in model.offered[-1]
|
assert "reply" not in model.offered[-1] and "post_summary" in model.offered[-1]
|
||||||
assert item(w).position == CAPPED
|
assert item(w).position == CAPPED
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_second_review_at_the_same_head_has_its_own_marker(tmp_path: Path) -> None:
|
||||||
|
# nisse-review on #156 feared the first review's unnumbered marker would
|
||||||
|
# pass for a crashed second one at the same head.
|
||||||
|
head = pr().head_sha
|
||||||
|
w = reviewed(tmp_path)
|
||||||
|
reviewer = w["scheduler"]._reviewer
|
||||||
|
assert reviewer.posted(REPO, 7, head, 1)
|
||||||
|
assert not reviewer.posted(REPO, 7, head, 2) # `review` again: not yet
|
||||||
|
w["gitea"].posted.append(
|
||||||
|
{
|
||||||
|
"repo": REPO,
|
||||||
|
"pr": 7,
|
||||||
|
"body": f"again\n\n{marker(head, 2)}",
|
||||||
|
"commit": head,
|
||||||
|
"comments": [],
|
||||||
|
}
|
||||||
|
)
|
||||||
|
assert reviewer.posted(REPO, 7, head, 2)
|
||||||
|
assert marker(head, 1) not in marker(head, 2)
|
||||||
|
|
||||||
|
|
||||||
|
def test_review_with_a_comment_beside_it_is_one_fresh_review(tmp_path: Path) -> None:
|
||||||
|
# nisse-review on #156 feared a replies run, then a second, pointless
|
||||||
|
# whole-PR review: `review` makes the run a first one, which clears it.
|
||||||
|
w = reviewed(tmp_path, [call("post_review", summary="Fresh look.")])
|
||||||
|
w["gitea"].say("don", "review")
|
||||||
|
w["gitea"].say("nisse-dev", "Also, a question.")
|
||||||
|
assert w["scheduler"].round() is True
|
||||||
|
assert "Review o/demo#7" in last_brief(w) # a first review, not replies
|
||||||
|
known = pr_of(item(w))
|
||||||
|
assert (known.full, known.fresh, item(w).position) == (False, (), REVIEWED)
|
||||||
|
assert w["scheduler"].round() is False # nothing more due
|
||||||
Reference in new issue
Block a user