diff --git a/adapters/facts.py b/adapters/facts.py index 252c3557..cb871da0 100644 --- a/adapters/facts.py +++ b/adapters/facts.py @@ -3,9 +3,8 @@ The pull request is read first and once more last (`adapters/pull.unchanged`): a push, a retarget, or any change GitHub records on it between makes the parts of two pull requests, and the snapshot is refused, to be read again at the -next pass. A head pushed away and back -between the two reads is not seen: the diff and the files are the pull request's, not pinned to its head commit, so they -may be of the commit between (the known limit; pinning them is still to be built). A part that cannot be read +next pass. The diff and the files are read by one compare pinned to the first read's head commit (issue 284), so a head +pushed away and back between the two reads still gives the parts of that commit. A part that cannot be read refuses the whole; a list that was not read whole says so in its `complete` flag, and the Kernel defers on it. One part is not read yet, and says so: `acceptance` is null, and whole only where no issue the pull request closes @@ -46,8 +45,8 @@ def snapshot(transport, repo: str, number: int, identities: dict, accounts: time own, title = pull.own(first, identities) branch = pull.base(transport, first) pusher, pushed_at = pull.last_push(transport, first) - content = pull.diff(transport, first) - paths = pull.changed_paths(transport, first) + content = pull.diff(transport, first, branch["head"]) + paths = pull.changed_paths(transport, first, branch["head"]) events, timeline_whole = timeline.events(transport, first) reviews, reviews_whole = timeline.reviews(transport, first, events, accounts, branch["head"]) writer = timeline.since_writer(events, accounts) diff --git a/adapters/pull.py b/adapters/pull.py index eef19ea1..7f78f5f3 100644 --- a/adapters/pull.py +++ b/adapters/pull.py @@ -15,7 +15,7 @@ from adapters.github import DIFF, Unanswered, raw, request -PAGE = 100 # files on one page of a pull request's list of files +COMPARED = 300 # the files a compare lists at most, as GitHub documents it, unverified; more is read short # What GitHub says of a file. A rename and a copy carry the name the file had; no other status does. STATUSES = {"added", "removed", "modified", "renamed", "copied", "changed", "unchanged"} MOVED = {"renamed", "copied"} @@ -189,21 +189,36 @@ def _counts(text: str): return files, added, deleted -def diff(transport, pull: dict) -> dict: - """`diff`, `content_id` and `complete.diff`, from the pull request's own diff against its merge base. +def _compare(pull: dict, base_head: str) -> str: + """The compare of the base branch's head with the pull request's head commit, as its first read gave it: the one + read the diff and the files are taken from, so both are of that commit, whatever is pushed while the snapshot is + read (issue 284's ruling).""" + head = str(pull["head"].get("sha", "")) + if not SHA.match(head): + raise Unanswered("the pull request's head commit cannot be read") + if not SHA.match(str(base_head)): + raise Unanswered("the base branch's head commit cannot be read") + return "/repos/%s/compare/%s...%s" % (pull["base"]["repo"]["full_name"], base_head, head) + + +def diff(transport, pull: dict, base_head: str) -> dict: + """`diff`, `content_id` and `complete.diff`, from the diff of the head commit against its merge base with the base + branch's head, `GET /repos/{repo}/compare/{base_head}...{head}`, pinned to the head of the pull request's first read + (issue 284). P-29 shows those bytes equal the pull request's own diff (`trials/P-29.base.json`), so the content id is + the one `adapters/timeline.py` gives a review's commit. The diff is checked against the counts of the same pull request's read, its files, added and deleted lines: a difference is a short read, `complete.diff` false. The content id is the sha256 of the bytes GitHub sent. A 406 `too_large` is GitHub's whole answer that it will not serve the diff: `truncated`, read whole, which holds the pull request as too large; its `content_id` is the head commit's, so any push is new content and nothing is read as - unchanged (decision 0002). Any other 406 is refused. No read is built from `patch`.""" + unchanged (decision 0002). P-29 grounds that answer for a pull request's diff; compare's is read the same, + unverified. A 404, a head no longer there, refuses the snapshot. Any other 406 is refused. No read is built from + `patch`.""" files, added, deleted = pull.get("changed_files"), pull.get("additions"), pull.get("deletions") if not all(isinstance(n, int) and not isinstance(n, bool) and n >= 0 for n in (files, added, deleted)): raise Unanswered("the pull request's counts cannot be read") - head = str(pull["head"].get("sha", "")) - if not SHA.match(head): - raise Unanswered("the pull request's head commit cannot be read") - path = "/repos/%s/pulls/%d" % (pull["base"]["repo"]["full_name"], pull["number"]) + path = _compare(pull, base_head) + head = str(pull["head"]["sha"]) answer = transport.send(request("GET", path, accept=DIFF)) size = {"files": files, "lines": added + deleted} if answer["status"] == 406: @@ -220,37 +235,38 @@ def diff(transport, pull: dict) -> dict: "complete": whole} -def changed_paths(transport, pull: dict) -> dict: - """`changed_paths` and `complete.changed_paths`: every file the pull request changes, page by page, each by its path - and, for a rename or a copy, the path it had before. Whole only when every page was read and the list holds exactly - as many files as the pull request's count, each once; GitHub stops listing at 3,000, and a page past the end is a - 200 with nothing in it, so a list that ends short of the count is read as short, never as the whole (P-29, decision - 0020).""" +def changed_paths(transport, pull: dict, base_head: str) -> dict: + """`changed_paths` and `complete.changed_paths`: every file the head commit changes against its merge base, from the + same compare as the diff, pinned to the head of the pull request's first read (issue 284), each by its path and, + for a rename or a copy, the path it had before. Whole only when the list holds exactly as many files as the pull + request's count, each once. A compare lists at most COMPARED files, as GitHub documents it (no trial records it), so + a change of more is read short by its count alone, with nothing asked, and the Kernel defers it for the Maintainer: + no answer GitHub gives for such a change can refuse the snapshot (issue 284's ruling; the pull request's own list + gave 3,000, P-29).""" want = pull.get("changed_files") if not isinstance(want, int) or isinstance(want, bool) or want < 0: raise Unanswered("the pull request's count of files cannot be read") - base = "/repos/%s/pulls/%d/files?per_page=%d" % (pull["base"]["repo"]["full_name"], pull["number"], PAGE) - paths, page = [], 1 - while True: - rows = _ok(transport.send(request("GET", base if page == 1 else "%s&page=%d" % (base, page))), "the files") - if not isinstance(rows, list): - raise Unanswered("a page of files is not a list") - for row in rows: - name = row.get("filename") if isinstance(row, dict) else None - if not isinstance(name, str) or not name: - return {"changed_paths": paths, "complete": False} - status = row.get("status") - moved = status in MOVED - before = row.get("previous_filename") if moved else None - # A status GitHub does not give, a move without its old name, or an old name on anything but a move, is a - # row that cannot be read: the list is short, never read as an ordinary change. - if status not in STATUSES or (moved and not (isinstance(before, str) and before)) \ - or (not moved and row.get("previous_filename") is not None): - return {"changed_paths": paths, "complete": False} - paths.append({"path": name, "previous": before}) - if len(rows) < PAGE or len(paths) >= want: - break - page += 1 + if want > COMPARED: + return {"changed_paths": [], "complete": False} + path = _compare(pull, base_head) + compared = _ok(transport.send(request("GET", path)), "the compare") + rows = compared.get("files") if isinstance(compared, dict) else None + if not isinstance(rows, list): + raise Unanswered("the compare's files are not a list") + paths = [] + for row in rows: + name = row.get("filename") if isinstance(row, dict) else None + if not isinstance(name, str) or not name: + return {"changed_paths": paths, "complete": False} + status = row.get("status") + moved = status in MOVED + before = row.get("previous_filename") if moved else None + # A status GitHub does not give, a move without its old name, or an old name on anything but a move, is a + # row that cannot be read: the list is short, never read as an ordinary change. + if status not in STATUSES or (moved and not (isinstance(before, str) and before)) \ + or (not moved and row.get("previous_filename") is not None): + return {"changed_paths": paths, "complete": False} + paths.append({"path": name, "previous": before}) names = [p["path"] for p in paths] whole = len(paths) == want and len(set(names)) == len(names) return {"changed_paths": paths, "complete": whole} diff --git a/docs/architecture/software-factory-design.md b/docs/architecture/software-factory-design.md index be96acab..80deec10 100644 --- a/docs/architecture/software-factory-design.md +++ b/docs/architecture/software-factory-design.md @@ -327,8 +327,8 @@ for contract tests, so one schema has three uses. | `mergeable` | Whether the head merges cleanly into the base: true, false, or null while GitHub has not worked it out. As GitHub last worked it out: a move of the base alone does not make GitHub work it out again (P-29) | | `diff_size` | Files and lines changed | | `head`, `base`, `merge_base` | Commit hashes: the head commit; the base branch's head, read at the pass, never the pull request's `base.sha`, which moves only when the head is pushed (P-29); and the merge base GitHub gives for the head against that branch, `compare`'s `merge_base_commit`. None is in `schemas/facts.schema.json` until something reads it | -| `content_id` | Hash of the pull request's own diff against its merge base, computed by the port | -| `changed_paths` | One record for each file in that diff, as many as `diff_size` counts files, each by its own path once, from the top of the repo, with `previous`, the path a renamed or copied file had before, null for any other change. A list of another length, a record not in that shape, or a path with an empty, `.` or `..` name, is unreadable, `paths_unreadable`, for the Maintainer. The Kernel matches each against the protected paths and the Lenses' inputs, and never shows one (§4.2, decision 0020) | +| `content_id` | Hash of the head commit's diff against its merge base, computed by the port. It is read by compare, `GET /repos/{repo}/compare/{base}...{head}`, at the head of the snapshot's first read and the base branch's head the pass read, so a head pushed away and back during the snapshot still gives that commit's content (issue 284). P-29 shows those bytes are the pull request's own diff. A `406` `too_large` reads as truncated, with the head commit's hash as its id: P-29 grounds that answer for the pull request's diff, not for compare's | +| `changed_paths` | One record for each file in that diff, as many as `diff_size` counts files, each by its own path once, from the top of the repo, with `previous`, the path a renamed or copied file had before, null for any other change. A list of another length, a record not in that shape, or a path with an empty, `.` or `..` name, is unreadable, `paths_unreadable`, for the Maintainer. The Kernel matches each against the protected paths and the Lenses' inputs, and never shows one (§4.2, decision 0020). Read from the same compare as `content_id`, which lists at most 300 files, as GitHub documents it (unverified): a change of more is read short by its count alone, with nothing asked, and the Kernel defers it for the Maintainer (issue 284) | | `label_events` | Every `labeled` and `unlabeled` event on the timeline: the label, who added or removed it (kind, name and write access), and when | | `reviews` | Who submitted it (`by`: kind, name and whether that actor has write access, as a thread's opening actor), state (`approved`, `changes_requested`, `commented` or `dismissed`, as GitHub holds it now), commit, `content_id`, `submitted_at`; and, from the timeline, `dismissed_by`: who dismissed it, of what kind and with what access, and `dismissed_from`: what it was before (`approved`, `changes_requested` or `commented`). Both are null unless it was dismissed | | `threads` | Thread id, path, line, when it was opened, resolved, outdated, opening actor and resolving actor, each with its kind and whether it has write access (the resolving actor null unless resolved); and the Finding's `category` and `severity`. For a required Lens's thread both come from that Lens's own Finding record, in its Run's `applied.json` (§3.7, §3.10), joined by `surface.key`, never from the comment text, which anyone with write access can edit. A person's thread is `human`, with no severity; an Advisory reviewer's, which no Finding record names, has neither | diff --git a/docs/decisions/0020-a-change-to-the-factorys-own-rules-or-a-lenss-inputs-is-reviewed-by-a-model-lens.md b/docs/decisions/0020-a-change-to-the-factorys-own-rules-or-a-lenss-inputs-is-reviewed-by-a-model-lens.md index 0c402c1e..8419411d 100644 --- a/docs/decisions/0020-a-change-to-the-factorys-own-rules-or-a-lenss-inputs-is-reviewed-by-a-model-lens.md +++ b/docs/decisions/0020-a-change-to-the-factorys-own-rules-or-a-lenss-inputs-is-reviewed-by-a-model-lens.md @@ -97,5 +97,6 @@ not weaken the Lens that judges it, and that story comes next. | The session hooks ask about `config/apps.json`, and list a Lens's input at a stop without asking about it | `tests/test_hooks.py::test_an_edit_elsewhere_is_not_asked_about`, `tests/test_hooks.py::test_a_lens_input_changed_is_listed_as_reported` | | In the simulator a Role, the dependency bot and the Maintainer change the list, the list is read short or leaves a path out, and nothing that changes it is approved, enqueued, left queued or merged without every model Lens's review of that head (I-45) | `tests/test_simulator.py::test_the_seeds_change_the_list_by_a_role_the_dependency_bot_and_the_maintainer` | | The port reads every path the pull request changes, both names of a rename among them, and says when the list was cut short. "Port: the paths a pull request changes, and both names of a rename" | `tests/test_adapters.py::test_every_file_is_read_and_the_list_is_whole_when_it_holds_the_count`, `tests/test_adapters.py::test_a_list_github_stops_short_of_the_count_is_never_the_whole`, `tests/test_adapters.py::test_a_rename_or_a_copy_gives_both_names_and_one_without_its_old_name_is_short`, `tests/test_adapters.py::test_a_renamed_file_is_read_with_its_old_name_from_a_fixture` | +| The paths are read from the compare of the base branch's head with the head of the snapshot's first read, never off the pull request, so they are that commit's; a compare lists at most 300 files, as GitHub documents it (unverified), so a change of more is read short by its count alone and deferred for the Maintainer. "Facts port: the snapshot's diff and files pinned to the head commit" | `tests/test_adapters.py::test_a_file_list_read_at_another_commit_is_not_asked_for`, `tests/test_adapters.py::test_a_list_github_stops_short_of_the_count_is_never_the_whole`, `tests/test_adapters.py::test_a_list_of_up_to_three_hundred_files_is_whole` | | `apply` refuses a Role's push to a protected path or a Lens's input that no model Lens has reviewed. "Harness: apply refuses a push to the list that no model Lens has reviewed" | waiting on `harness/` | | What GitHub holds is recorded: the `main` ruleset requires no review, as `config/rulesets.json` says, so a ruleset that requires the machine user's approval changes that file, and a live ruleset that differs from it is drift | `tests/test_tools.py::test_a_changed_rule_is_drift` | diff --git a/sim/grounding.py b/sim/grounding.py index 9045a796..c7c55389 100644 --- a/sim/grounding.py +++ b/sim/grounding.py @@ -126,6 +126,8 @@ ("a_pull_request_lists_the_paths_it_changes", "Every file a pull request changes is read once, as many as the diff " "counts, by its path from the top of the repo, with the path a renamed file had before; an unchanged copy is " "listed as an added file", ("P-43",), "trials/P-43.json", None), + # The port reads a compare, not this list (issue 284): a change of more files than a compare lists, 300 as GitHub + # documents it and no trial records, is read short by changed_files alone, which the fixture's 3,001 shows. ("a_path_list_can_leave_out_a_file", "A list of the files a pull request changes can leave one out while it says it " "was read in full; it then holds fewer records than the diff has files: GitHub's list stops at its cap with a 200 " "and an empty next page, and only changed_files shows it short", ("P-29",), "trials/P-29.truncated.json", None), diff --git a/tests/test_adapters.py b/tests/test_adapters.py index 639e9332..de63e7bc 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -536,17 +536,64 @@ def test_a_fork_is_read_as_no_one_and_its_activity_is_never_asked(self): self.assertIsNone(pull.last_pusher(github.Replay([]), fork)) +PINNED_BASE = "b" * 40 # a base branch's head the compare names; a replay matches the call it is given + + +def as_compare(exchange: dict, data: dict, base: str = PINNED_BASE) -> dict: + """A recorded answer to the pull request's own diff, or to a page of its files, replayed as the compare of `base` + with the head of `data`, as the port now asks it (issue 284): P-29 shows the diff's bytes equal the compare's + (`trials/P-29.base.json`), and a compare lists its files under `files`, as P-29's exchange 10 does.""" + found = copy.deepcopy(exchange) + call = found["request"]["call"] + found["request"]["call"] = "GET /repos/%s/compare/%s...%s" % (data["base"]["repo"]["full_name"], base, + data["head"]["sha"]) + if "/files" in call: + found["request"]["headers"] = dict(found["request"]["headers"], accept="application/vnd.github+json") + found["response"] = dict(found["response"], body={"files": found["response"]["body"]}) + return found + + +def compared(name: str, at: int, data: dict, base: str = PINNED_BASE) -> github.Replay: + return github.Replay([as_compare(exchanges(name)[at], data, base)]) + + class Diff(unittest.TestCase): + def test_a_diff_read_at_another_commit_cannot_give_the_snapshots_content(self): # issue 284 + # The diff is read by compare at the head of the pull request's first read, never off the pull request: a head + # pushed away and back between the reads still gives the content of the first read's head, and an answer for + # another commit is not one the port asks for. + data = body("P-29.base", 1) + at_head = pull.diff(compared("P-29.base", 2, data), data, PINNED_BASE) + self.assertEqual(at_head["content_id"], "sha256:" + body("P-29.base", 24)["before the base moved"]) + elsewhere = copy.deepcopy(data) + elsewhere["head"]["sha"] = "c" * 40 + with self.assertRaises(github.Unanswered): + pull.diff(github.Replay([as_compare(exchanges("P-29.base")[2], elsewhere)]), data, PINNED_BASE) + with self.assertRaises(github.Unanswered): # the pull request's own diff is never read + pull.diff(replay("P-29.base", 2), data, PINNED_BASE) + # A head no longer there answers 404, and a base head that cannot be read is refused: no snapshot. + gone = as_compare(exchanges("P-29.base")[2], data) + gone["response"] = dict(gone["response"], status=404, body={"message": "Not Found"}) + with self.assertRaises(github.Unanswered): + pull.diff(github.Replay([gone]), data, PINNED_BASE) + with self.assertRaises(github.Unanswered): + pull.diff(compared("P-29.base", 2, data, "main"), data, "main") + # A fork's head is compared in the base repo, which holds both commits, never in the fork (Muse on PR 284). + fork = copy.deepcopy(data) + fork["head"]["repo"] = dict(fork["head"]["repo"], full_name="someone/fork", id=1) + self.assertEqual(pull.diff(compared("P-29.base", 2, fork), fork, PINNED_BASE)["content_id"], at_head["content_id"]) + def test_the_content_is_the_hash_of_the_diff_as_github_served_it(self): # trials/P-29.base.json: the hash taken as each diff arrived is the one the port computes. hashes = body("P-29.base", 24) for at, answer, moment in ((1, 2, "before the base moved"), (12, 13, "after the plain push"), (21, 22, "after the force push")): - got = pull.diff(replay("P-29.base", answer), body("P-29.base", at)) + got = pull.diff(compared("P-29.base", answer, body("P-29.base", at)), body("P-29.base", at), PINNED_BASE) self.assertEqual(got["content_id"], "sha256:" + hashes[moment], moment) self.assertIs(got["complete"], True, moment) self.assertEqual(got["diff"]["truncated"], False) - self.assertEqual(pull.diff(replay("P-29.base", 2), body("P-29.base", 1))["diff"], + data = body("P-29.base", 1) + self.assertEqual(pull.diff(compared("P-29.base", 2, data), data, PINNED_BASE)["diff"], {"files": 1, "lines": 3, "truncated": False}) def test_the_content_is_hashed_over_the_bytes_github_sent(self): @@ -559,7 +606,7 @@ def test_the_content_is_hashed_over_the_bytes_github_sent(self): answer = github._answer(200, {"Content-Type": github.DIFF}, sent) exchange = dict(exchanges("P-29.base")[2], response=dict(exchanges("P-29.base")[2]["response"], body=answer["body"])) - got = pull.diff(github.Replay([exchange]), data) + got = pull.diff(github.Replay([as_compare(exchange, data)]), data, PINNED_BASE) self.assertEqual(got["content_id"], "sha256:" + hashlib.sha256(sent).hexdigest()) self.assertIs(got["complete"], True) ids.append(got["content_id"]) @@ -569,29 +616,29 @@ def test_a_diff_that_disagrees_with_the_counts_is_a_short_read(self): # Decision 0002: the diff is held to the same pull request's counts; a difference is a short read. data = body("P-29.base", 1) data["additions"] += 1 - self.assertIs(pull.diff(replay("P-29.base", 2), data)["complete"], False) + self.assertIs(pull.diff(compared("P-29.base", 2, data), data, PINNED_BASE)["complete"], False) data = body("P-29.base", 1) data["changed_files"] += 1 - self.assertIs(pull.diff(replay("P-29.base", 2), data)["complete"], False) + self.assertIs(pull.diff(compared("P-29.base", 2, data), data, PINNED_BASE)["complete"], False) def test_a_diff_github_will_not_serve_is_truncated_and_new_content_at_every_push(self): # trials/P-29.truncated.json: 3,001 files, a 406 too_large. Read whole, so the Kernel holds it as too large; # its content is the head commit's, so a push is never read as unchanged. data = body("P-29.truncated", 2) - got = pull.diff(replay("P-29.truncated", 4), data) + got = pull.diff(compared("P-29.truncated", 4, data), data, PINNED_BASE) self.assertEqual((got["diff"], got["complete"]), ({"files": 3001, "lines": 3001, "truncated": True}, True)) moved = copy.deepcopy(data) moved["head"]["sha"] = "0" * 40 - self.assertNotEqual(pull.diff(replay("P-29.truncated", 4), moved)["content_id"], got["content_id"]) + self.assertNotEqual(pull.diff(compared("P-29.truncated", 4, moved), moved, PINNED_BASE)["content_id"], got["content_id"]) # A 406 for anything but too_large, and a head that cannot be read, are refused. other = copy.deepcopy(exchanges("P-29.truncated")[4]) other["response"]["body"]["errors"][0]["code"] = "not_acceptable" with self.assertRaises(github.Unanswered): - pull.diff(github.Replay([other]), data) + pull.diff(github.Replay([as_compare(other, data)]), data, PINNED_BASE) no_head = copy.deepcopy(data) no_head["head"]["sha"] = "" with self.assertRaises(github.Unanswered): - pull.diff(replay("P-29.truncated", 4), no_head) + pull.diff(compared("P-29.truncated", 4, no_head), no_head, PINNED_BASE) def test_a_line_of_content_that_looks_like_a_header_is_counted_as_the_line_it_is(self): text = ("diff --git a/q.sql b/q.sql\nindex 1..2 100644\n--- a/q.sql\n+++ b/q.sql\n@@ -1,2 +1,2 @@\n" @@ -606,41 +653,60 @@ def test_an_answer_that_is_not_a_diff_is_refused(self): broken = copy.deepcopy(exchanges("P-29.base")[2]) broken["response"]["status"] = 500 with self.assertRaises(github.Unanswered): - pull.diff(github.Replay([broken]), body("P-29.base", 1)) + pull.diff(github.Replay([as_compare(broken, body("P-29.base", 1))]), body("P-29.base", 1), PINNED_BASE) + +def files_rows(count: int) -> list: + """`count` rows of a list of files, shaped as P-29.truncated's page 30 gives them.""" + row = exchanges("P-29.truncated")[10]["response"]["body"][0] + return [dict(row, filename="p29-many-files/f%04d.txt" % (i + 1)) for i in range(count)] -def files_pages(pages: int, last: list) -> list: - """Pages 1 to `pages` of pull request 27's files, shaped as P-29.truncated's page 30, then that page and `last`.""" - found = exchanges("P-29.truncated") - page30, page31 = found[10], found[11] - base = page30["request"]["call"].split()[1].split("&page=")[0] - out = [] - for n in range(1, pages): - rows = [dict(row, filename="p29-many-files/f%04d.txt" % ((n - 1) * 100 + i + 1)) - for i, row in enumerate(page30["response"]["body"])] - call = "GET %s" % (base if n == 1 else "%s&page=%d" % (base, n)) - out.append({"request": dict(page30["request"], call=call), "response": dict(page30["response"], body=rows)}) - return out + [page30, dict(page31, response=dict(page31["response"], body=last))] + +def compare_files(rows: list, data: dict) -> dict: + """A compare at the head of `data` that lists `rows` as its files.""" + found = as_compare(exchanges("P-29.truncated")[13], data) + found["response"] = dict(found["response"], body={"files": rows}) + return found class ChangedPaths(unittest.TestCase): def test_every_file_is_read_and_the_list_is_whole_when_it_holds_the_count(self): - got = pull.changed_paths(replay("P-29.truncated", 13), body("P-29.truncated", 12)) + got = pull.changed_paths(compared("P-29.truncated", 13, body("P-29.truncated", 12)), body("P-29.truncated", 12), PINNED_BASE) self.assertEqual(got, {"changed_paths": [{"path": "p29-many-lines-2.txt", "previous": None}, {"path": "p29-many-lines.txt", "previous": None}], "complete": True}) def test_a_list_github_stops_short_of_the_count_is_never_the_whole(self): - # trials/P-29.truncated.json: 3,001 files; page 30 is the last GitHub serves, and page 31 is a 200 with none. - got = pull.changed_paths(github.Replay(files_pages(30, [])), body("P-29.truncated", 2)) - self.assertEqual((len(got["changed_paths"]), got["complete"]), (3000, False)) + # trials/P-29.truncated.json: 3,001 files. A compare lists at most 300, so a change of more is read short by + # its count alone: nothing is asked (an empty replay answers nothing), so no answer can refuse the snapshot + # (issue 284's ruling). 301 rows, which GitHub would not send, are never read whole either. + data = body("P-29.truncated", 2) + for count in (3001, 301): + data["changed_files"] = count + got = pull.changed_paths(github.Replay([]), data, PINNED_BASE) + self.assertEqual(got, {"changed_paths": [], "complete": False}, count) - def test_a_list_at_an_exact_page_count_asks_for_no_page_past_it(self): + def test_a_list_of_up_to_three_hundred_files_is_whole(self): data = body("P-29.truncated", 2) - data["changed_files"] = 200 - pages = files_pages(3, [])[:2] - got = pull.changed_paths(github.Replay(pages), data) - self.assertEqual((len(got["changed_paths"]), got["complete"]), (200, True)) + for count in (200, 300): + data["changed_files"] = count + got = pull.changed_paths(github.Replay([compare_files(files_rows(count), data)]), data, PINNED_BASE) + self.assertEqual((len(got["changed_paths"]), got["complete"]), (count, True), count) + + def test_a_file_list_read_at_another_commit_is_not_asked_for(self): # issue 284 + data = body("P-29.truncated", 12) + elsewhere = copy.deepcopy(data) + elsewhere["head"]["sha"] = "c" * 40 + with self.assertRaises(github.Unanswered): + pull.changed_paths(github.Replay([as_compare(exchanges("P-29.truncated")[13], elsewhere)]), data, PINNED_BASE) + with self.assertRaises(github.Unanswered): # the pull request's own list is never read + pull.changed_paths(replay("P-29.truncated", 13), data, PINNED_BASE) + # A compare whose files are not a list, or are not there at all, is refused, never read as none (Muse on PR 284). + for answered in ({"files": "none"}, {}): + broken = as_compare(exchanges("P-29.truncated")[13], data) + broken["response"] = dict(broken["response"], body=answered) + with self.assertRaises(github.Unanswered): + pull.changed_paths(github.Replay([broken]), data, PINNED_BASE) def test_a_rename_or_a_copy_gives_both_names_and_one_without_its_old_name_is_short(self): data = body("P-29.truncated", 12) @@ -649,11 +715,11 @@ def test_a_rename_or_a_copy_gives_both_names_and_one_without_its_old_name_is_sho rows[1].update(status="copied", previous_filename="p29-many-lines-2.txt") answer = dict(exchanges("P-29.truncated")[13], response=dict(exchanges("P-29.truncated")[13]["response"], body=rows)) - got = pull.changed_paths(github.Replay([answer]), data) + got = pull.changed_paths(github.Replay([as_compare(answer, data)]), data, PINNED_BASE) self.assertEqual([p["previous"] for p in got["changed_paths"]], ["old/name.txt", "p29-many-lines-2.txt"]) self.assertIs(got["complete"], True) del rows[0]["previous_filename"] - self.assertIs(pull.changed_paths(github.Replay([answer]), data)["complete"], False) + self.assertIs(pull.changed_paths(github.Replay([as_compare(answer, data)]), data, PINNED_BASE)["complete"], False) def test_a_list_with_a_name_twice_or_none_is_short(self): data = body("P-29.truncated", 12) @@ -663,7 +729,7 @@ def test_a_list_with_a_name_twice_or_none_is_short(self): rows[0].update(change) answer = dict(exchanges("P-29.truncated")[13], response=dict(exchanges("P-29.truncated")[13]["response"], body=rows)) - self.assertIs(pull.changed_paths(github.Replay([answer]), data)["complete"], False, change) + self.assertIs(pull.changed_paths(github.Replay([as_compare(answer, data)]), data, PINNED_BASE)["complete"], False, change) def test_a_renamed_file_is_read_with_its_old_name_from_a_fixture(self): # trials/P-43.json (issue 250) records a rename: REST names its old path, as the synthetic case above does. @@ -1014,7 +1080,8 @@ def test_an_outcome_is_held_in_the_bounds_before_it_is_read_out(self): class Router: """A transport that answers a permission or a compare from a table it is given, and anything else from a replay. - No fixture holds those two reads yet: the permission's trial is #222's, and P-7 recorded no compare.""" + A compare not in the table is the replay's, as the snapshot's own compares of the diff and the files are (issue + 284), and one the replay does not hold either answers 404. The permission's trial is #222's.""" def __init__(self, replay, permissions=None, compares=None): self.replay, self.permissions, self.compares, self.asked = replay, dict(permissions or {}), dict(compares or {}), [] @@ -1030,9 +1097,13 @@ def send(self, req): return {"status": 200, "headers": {}, "body": {"permission": level}} if "/compare/" in path: commit = path.split("...")[1] - if commit not in self.compares: + if commit in self.compares: + return {"status": 200, "headers": {}, "body": self.compares[commit]} + # The snapshot's own compares, of the diff and the files at the head, are the replay's (issue 284). + try: + return self.replay.send(req) + except github.Unanswered: return {"status": 404, "headers": {}, "body": {"message": "Not Found"}} - return {"status": 200, "headers": {}, "body": self.compares[commit]} return self.replay.send(req) @@ -1722,8 +1793,11 @@ def sequence(self, reread: int) -> list: P-29.truncated, P-7 and P-32 for its files, reviews and checks, which no fixture holds for this one.""" found = exchanges("P-29.base") head = found[1]["response"]["body"]["head"]["sha"] - return [found[1], found[9], exchanges("P-28")[3], found[2], p26_files(), exchanges("P-28")[4], p26_reviews(), - p26_checks(head), closing_answer(canary(), 26, []), threads_answer(canary(), 26, []), found[reread]] + branch = found[9]["response"]["body"]["commit"]["sha"] + data = found[1]["response"]["body"] + return [found[1], found[9], exchanges("P-28")[3], as_compare(found[2], data, branch), + as_compare(p26_files(), data, branch), exchanges("P-28")[4], p26_reviews(), p26_checks(head), + closing_answer(canary(), 26, []), threads_answer(canary(), 26, []), found[reread]] def read(self, reread: int): router = Router(github.Replay(self.sequence(reread)), {"don-petry": "admin"}) @@ -1869,8 +1943,8 @@ def test_the_port_reads_each_grounded_rule_as_the_fake_assumes_it(self): # grounding, or a fixture the port reads otherwise, fails. from sim import grounding grounded = {code: fixture for code, _, _, fixture, _ in grounding.RULES if fixture} - before = pull.diff(replay("P-29.base", 2), body("P-29.base", 1))["content_id"] - after = pull.diff(replay("P-29.base", 13), body("P-29.base", 12))["content_id"] + before = pull.diff(compared("P-29.base", 2, body("P-29.base", 1)), body("P-29.base", 1), PINNED_BASE)["content_id"] + after = pull.diff(compared("P-29.base", 13, body("P-29.base", 12)), body("P-29.base", 12), PINNED_BASE)["content_id"] fork = [e for e in exchanges("P-27") if "/runs?" in e["request"].get("call", "") and e["response"]["status"] == 200] rows = [r for e in fork for r in e["response"]["body"].get("workflow_runs", []) if r["head_repository"]["full_name"] != r["repository"]["full_name"]] @@ -1880,9 +1954,9 @@ def test_the_port_reads_each_grounded_rule_as_the_fake_assumes_it(self): "a_push_is_new_content": before != after, "the_pusher_is_the_last_pusher": pull.last_pusher(replay("P-28", 7), body("P-28", 10)) == "don-petry" and pull.last_pusher(replay("P-28.app", 1), body("P-28.app", 0)) == "combsmith-sf-trial-author[bot]", - "a_short_diff_says_so": pull.diff(replay("P-29.truncated", 4), body("P-29.truncated", 2))["diff"]["truncated"], - "a_path_list_can_leave_out_a_file": pull.changed_paths(github.Replay(files_pages(30, [])), - body("P-29.truncated", 2))["complete"] is False, + "a_short_diff_says_so": pull.diff(compared("P-29.truncated", 4, body("P-29.truncated", 2)), body("P-29.truncated", 2), PINNED_BASE)["diff"]["truncated"], + "a_path_list_can_leave_out_a_file": pull.changed_paths(github.Replay([]), body("P-29.truncated", 2), + PINNED_BASE)["complete"] is False, "a_forks_run_never_counts": bool(recorded) and all(r["event"] == "pull_request" for r in recorded) and all(records._record(r, r["repository"]["full_name"], r.get("head_branch"), {r.get("path")}, (r["actor"]["id"], r["actor"]["type"])) == (None, None) for r in rows), diff --git a/tools/mutants.py b/tools/mutants.py index 3655bcf5..ad28d94d 100644 --- a/tools/mutants.py +++ b/tools/mutants.py @@ -1913,10 +1913,10 @@ def moved_before(rel: str, block: str, anchor: str) -> tuple: ' if self._left and self._left[0][0] == want:\n asked, answer = self._left.pop(0)\n', ' found = [i for i, (a, _) in enumerate(self._left) if a == want]\n if found:\n asked, answer = self._left.pop(found[0])\n', ('tests.test_adapters',)), ('a file row of unknown status is read', 'adapters/pull.py', - ' if status not in STATUSES or (moved and', - ' if (moved and', ('tests.test_adapters',)), + ' if status not in STATUSES or (moved and', + ' if (moved and', ('tests.test_adapters',)), ('an old name on anything but a move is read', 'adapters/pull.py', - ' \\\n or (not moved and row.get("previous_filename") is not None):\n', + ' \\\n or (not moved and row.get("previous_filename") is not None):\n', ':\n', ('tests.test_adapters',)), ('the read may be of another pull request', 'adapters/pull.py', ' if not isinstance(body, dict) or body.get("number") != number:\n', @@ -2161,12 +2161,30 @@ def moved_before(rel: str, block: str, anchor: str) -> tuple: ("a truncated diff's content never changes", 'adapters/pull.py', '"content_id": "sha256:" + hashlib.sha256(head.encode()).hexdigest(),', '"content_id": "sha256:" + hashlib.sha256(b"").hexdigest(),', ('tests.test_adapters',)), - ('only the first page of files is read', 'adapters/pull.py', - ' page += 1\n', - ' break\n', ('tests.test_adapters',)), + ('more files than a compare lists are read', 'adapters/pull.py', + ' if want > COMPARED:\n', + ' if False:\n', ('tests.test_adapters',)), + ('the diff and the files are read off the pull request', 'adapters/pull.py', + ' return "/repos/%s/compare/%s...%s" % (pull["base"]["repo"]["full_name"], base_head, head)\n', + ' return "/repos/%s/pulls/%d" % (pull["base"]["repo"]["full_name"], pull["number"])\n', ('tests.test_adapters',)), + ("a fork's head is compared in the fork", 'adapters/pull.py', + ' return "/repos/%s/compare/%s...%s" % (pull["base"]["repo"]["full_name"], base_head, head)\n', + ' return "/repos/%s/compare/%s...%s" % (pull["head"]["repo"]["full_name"], base_head, head)\n', ('tests.test_adapters',)), + ("a compare with no files is read as none", 'adapters/pull.py', + ' rows = compared.get("files") if isinstance(compared, dict) else None\n', + ' rows = compared.get("files", []) if isinstance(compared, dict) else None\n', ('tests.test_adapters',)), + ("the compare's base head may be anything", 'adapters/pull.py', + ' if not SHA.match(str(base_head)):\n', + ' if False:\n', ('tests.test_adapters',)), + ('the diff is compared with the base the pull request last named', 'adapters/facts.py', + ' content = pull.diff(transport, first, branch["head"])\n', + ' content = pull.diff(transport, first, first["base"]["sha"])\n', ('tests.test_adapters',)), + ('the files are compared with the base the pull request last named', 'adapters/facts.py', + ' paths = pull.changed_paths(transport, first, branch["head"])\n', + ' paths = pull.changed_paths(transport, first, first["base"]["sha"])\n', ('tests.test_adapters',)), ("a rename's old name is dropped", 'adapters/pull.py', - ' before = row.get("previous_filename") if moved else None\n', - ' before = None\n', ('tests.test_adapters',)), + ' before = row.get("previous_filename") if moved else None\n', + ' before = None\n', ('tests.test_adapters',)), ('a rename without its old name is whole', 'adapters/pull.py', '(moved and not (isinstance(before, str) and before))', '(False)', ('tests.test_adapters',)), @@ -2174,8 +2192,8 @@ def moved_before(rel: str, block: str, anchor: str) -> tuple: ' whole = len(paths) == want and len(set(names)) == len(names)\n', ' whole = True\n', ('tests.test_adapters',)), ('a list naming a file twice is whole', 'adapters/pull.py', - ' whole = len(paths) == want and len(set(names)) == len(names)\n', - ' whole = len(paths) == want\n', ('tests.test_adapters',)), + ' and len(set(names)) == len(names)\n', + '\n', ('tests.test_adapters',)), ('the network may be reached anywhere', 'tools/check_repo.py', ' if any(mod == net or mod.startswith(net + ".") for net in NETWORK) and pathlib.PurePosixPath(*rel.parts) != TRANSPORT:\n', ' if False:\n', ('tests.test_tools.Repo',)),