diff --git a/scripts/notebook_tools/check_pr_exercises.py b/scripts/notebook_tools/check_pr_exercises.py index 37a4e7d1b7..7fcb8b6271 100644 --- a/scripts/notebook_tools/check_pr_exercises.py +++ b/scripts/notebook_tools/check_pr_exercises.py @@ -205,6 +205,7 @@ def check_notebooks( base_ref: str = "", head_ref: str = "", pr_body: str = "", + base_path_of: dict[str, str] | None = None, ) -> CheckResult: """Classify + count each path, bucketing by advisory status. @@ -219,6 +220,12 @@ def check_notebooks( exemption markers from ``pr_body``. The default (``base_ref=""``) skips this check (preserves backward compatibility for callers that don't pass a base ref). + + #19251 -- ``base_path_of`` maps a ``path`` to the location it had at the + base, for a notebook the PR **renamed**: the credited diff then reads + ``base:previousFilename`` against ``head:path`` instead of the same path on + both sides (which would find nothing at the base and read as a full loss). + Absent from the map, ``path`` is used for both sides -- the pre-rename case. """ result = CheckResult() # Pré-calcul des exemptions une seule fois (cf. #18740). @@ -283,7 +290,16 @@ def check_notebooks( head_examples = count_credited_examples(_read_nb(head_blob)) else: head_examples = count_credited_examples(_read_nb(path)) - base_path = _read_git_blob(base_ref, str(path).replace("\\", "/")) + # #19251 -- un carnet RENOMME par la PR n'existe pas au meme + # chemin dans la base : sans la correspondance, `base:path` + # rendrait un blob absent et le diff lirait une perte totale. + # La carte est clee en POSIX (le chemin GraphQL) : on normalise + # la cle du `Path` avant de la consulter, sinon sous Windows + # (`str(path)` rend des `\`) la correspondance raterait en + # silence et le renommage redeviendrait non mesure. + posix_path = str(path).replace("\\", "/") + base_side = (base_path_of or {}).get(posix_path, posix_path) + base_path = _read_git_blob(base_ref, base_side) base_examples = count_credited_examples(_read_nb(base_path)) diff = diff_examples(base_examples, head_examples) nb_name = path.name diff --git a/scripts/notebook_tools/credited_examples_sweep.py b/scripts/notebook_tools/credited_examples_sweep.py index ee723722f1..1eef6f327c 100644 --- a/scripts/notebook_tools/credited_examples_sweep.py +++ b/scripts/notebook_tools/credited_examples_sweep.py @@ -15,7 +15,7 @@ coup : il liste les PRs MERGEES de la fenetre et rejoue, pour chacune, le diff avec SA base et SON body -- c'est l'option 1 de #19101. -## Deux sources de verite, chacune du bon cote (review 04:53Z) +## Trois sources de verite, chacune du bon cote (review 04:53Z, #19251) - **`changeType` n'existe pas dans `gh pr list --json files`** (mesure gh 2.83.2 : cette forme ne rend que `{additions, deletions, path}`). Le @@ -24,6 +24,14 @@ carnet supprime (#19040 a supprime GameTheory-18d) fait sinon planter le balayage en `FileNotFoundError` : le repli « tout MODIFIED » de la premiere version classait tout en modifie. +- **le chemin de base d'un RENAMED n'existe PAS cote GraphQL** (#19251, + mesure : le serveur repond `Field 'previousFilename' doesn't exist on type + 'PullRequestChangedFile'` ; ses champs sont `additions, changeType, + deletions, path, viewerViewedState`). Il vit cote **REST**, champ + `previous_filename` de `pulls/{n}/files` -- lu par `_previous_filenames`, + et **seulement** quand la PR porte un renommage. Avec ce chemin, le + renommage est **mesure** comme un MODIFIED ; sans lui, il reste nomme NON + MESURE. - **le cote « apres » est la tete de la PR, pas l'arbre de travail** : `check_notebooks` recoit `head_ref=headRefOid`, et le commit est amene localement s'il manque (PR squash-mergee : l'objet n'est pas dans `main`). @@ -129,6 +137,38 @@ def merged_prs(repo: str, since: dt.datetime, run=_gh_json) -> list[dict]: return rows +def _previous_filenames(repo: str, number: int, run=_gh_json) -> dict[str, str]: + """`{chemin_de_tete: chemin_de_base}` des renommages, par REST. + + `previousFilename` n'existe **pas** sur le type GraphQL + `PullRequestChangedFile` (#19251, mesure : le serveur repond `Field + 'previousFilename' doesn't exist on type 'PullRequestChangedFile'` ; ses + champs sont `additions, changeType, deletions, path, viewerViewedState`). + Le chemin de base d'un renommage vit cote REST, champ `previous_filename` + de `pulls/{n}/files` -- d'ou cette seconde source, appelee **seulement** + quand la PR porte un renommage. Pagement explicite (`per_page`/`page`) : + `per_page` en `-f` ferait basculer `gh api` en POST. + """ + owner, name = repo.split("/", 1) + out: dict[str, str] = {} + page = 1 + while True: + payload = run([ + "api", (f"repos/{owner}/{name}/pulls/{number}/files" + f"?per_page=100&page={page}"), + ]) + if not isinstance(payload, list) or not payload: + return out + for f in payload: + head = f.get("filename") or "" + prev = f.get("previous_filename") or "" + if head and prev: + out[head] = prev + if len(payload) < 100: + return out + page += 1 + + def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: """Fichiers de la PR avec `changeType`, par GraphQL, pagine au curseur. @@ -136,6 +176,13 @@ def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: du changement n'existe que cote GraphQL. Pages de 100, boucle sur `pageInfo.endCursor` -- une PR de carnet peut deplacer plus de 100 fichiers (mesure : #19040 en deplace 2876+214 lignes sur plusieurs carnets). + + #19251 -- le chemin de base d'un `RENAMED` n'est pas dans GraphQL : quand la + PR porte au moins un renommage, une seconde passe REST (`_previous_filenames`) + remplit `previousFilename` sur ces seuls noeuds, pour que le renommage soit + mesurable. Si cette passe echoue, les renommages restent sans chemin de base + (`""`) et seront nommes NON MESURES par `sweep` -- jamais mesures contre le + mauvais chemin. """ owner, name = repo.split("/", 1) nodes: list[dict] = [] @@ -151,14 +198,25 @@ def pr_files(repo: str, number: int, run=_gh_json) -> list[dict]: nodes.extend(conn.get("nodes") or []) page = conn.get("pageInfo") or {} if not page.get("hasNextPage"): - return nodes + break cursor = page.get("endCursor") + if any(n.get("changeType") == "RENAMED" for n in nodes): + try: + previous = _previous_filenames(repo, number, run) + except RuntimeError: + previous = {} + for node in nodes: + if node.get("changeType") == "RENAMED": + node["previousFilename"] = previous.get(node.get("path") or "", "") + return nodes -def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str]]: - """`(modifies, ajoutes, renommes)` parmi les `.ipynb` de la PR. +def _ipynb_by_change( + files: list[dict], +) -> tuple[list[str], list[str], list[tuple[str, str]], list[str]]: + """`(modifies, ajoutes, renommes_mesurables, renommes_non_mesurables)`. - Deux exclusions et une separation, toutes structurelles, toutes lues de + Trois exclusions et une separation, toutes structurelles, toutes lues de `changeType` (GraphQL) : - `DELETED` : supprime, plus rien a compter (meme regle que le @@ -167,17 +225,27 @@ def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str] l'ouverture du carnet levait `FileNotFoundError` sur GameTheory-18d (supprime par #19040) ; - `ADDED` : neuf, donc **rien a perdre** par construction ; - - `RENAMED` : la version de base existe **sous un autre chemin**, que - cette source n'expose pas (`previousFilename` absent du jeu GraphQL - demande ici). On ne peut donc pas la comparer -- et contrairement a - `ADDED`, un renommage **peut** perdre des exemples. NON MESURE. + - `RENAMED` : la version de base existe **sous un autre chemin**. Depuis + #19251 ce chemin est rempli par `pr_files` (passe REST, + `previous_filename`) : quand il est present, le renommage est + **mesurable** et rendu comme la paire `(chemin_de_tete, chemin_de_base)`, + pour que le diff credite soit calcule entre `base:previous` et + `head:path` -- un `git mv` suivi d'une edition peut perdre des exemples + comme un `MODIFIED`. + + `previousFilename` peut manquer (passe REST en echec, ou renommage hors du + champ REST) : ces renommages-la restent **non mesurables** et sont rendus a + part, pour etre **nommes** NON MESURES plutot que comptes comme zero perte. Pourquoi separer plutot que compter en erreur : `credited_diff_errors > 0` **bloque** la pose du label (#18761). Une erreur structurelle sur un carnet empechait donc la mesure reelle des carnets modifies de la meme PR -- un faux positif d'erreur produisait un faux zero de pertes. """ - modified, added, renamed = [], [], [] + modified: list[str] = [] + added: list[str] = [] + renamed_measured: list[tuple[str, str]] = [] + renamed_unmeasured: list[str] = [] for f in (files or []): path = f.get("path") or "" if not path.endswith(".ipynb"): @@ -188,10 +256,14 @@ def _ipynb_by_change(files: list[dict]) -> tuple[list[str], list[str], list[str] if change == "ADDED": added.append(path) elif change == "RENAMED": - renamed.append(path) + previous = f.get("previousFilename") or "" + if previous: + renamed_measured.append((path, previous)) + else: + renamed_unmeasured.append(path) else: modified.append(path) - return modified, added, renamed + return modified, added, renamed_measured, renamed_unmeasured def ipynb_paths(files: list[dict]) -> list[str]: @@ -253,18 +325,27 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, except RuntimeError as exc: errors.append(f"#{number}: fichiers illisibles ({exc})") continue - paths, added, renamed = _ipynb_by_change(files) + modified, added, renamed_measured, renamed_unmeasured = \ + _ipynb_by_change(files) + # #19251 -- un renommage dont `previousFilename` est connu est mesure + # comme un MODIFIED : sa tete se lit au nouveau chemin, sa base a + # l'ancien. `base_path_of` porte cette correspondance ; un renommage + # sans `previousFilename` reste NON MESURE et seulement nomme. + base_path_of = {head: prev for head, prev in renamed_measured} + paths = modified + [head for head, _ in renamed_measured] + renamed = len(renamed_measured) + len(renamed_unmeasured) if not paths and not added and not renamed: continue if not paths: # PR sans carnet mesurable : rien a perdre (ajouts) ou rien de - # comparable (renommages). On la NOMME plutot que de la faire - # disparaitre du rapport. + # comparable (renommages sans chemin de base). On la NOMME plutot + # que de la faire disparaitre du rapport. rows.append({ "number": number, "notebooks": 0, "added_notebooks": len(added), - "renamed_notebooks": len(renamed), + "renamed_notebooks": renamed, + "renamed_measured": len(renamed_measured), "credited_lost_unexempted": 0, "credited_diff_errors": 0, "would_label": False, @@ -298,7 +379,8 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, continue try: result = check([Path(p) for p in paths], base_ref=base, - head_ref=head_ref, pr_body=body) + head_ref=head_ref, pr_body=body, + base_path_of=base_path_of) except (OSError, ValueError) as exc: # #19215 (review 5411248369, voie 2) : un carnet illisible depuis # l'arbre du jour ne doit pas emporter les AUTRES PR de la @@ -317,7 +399,8 @@ def sweep(repo: str, repo_dir: Path, hours: int, now: dt.datetime, "number": number, "notebooks": len(paths), "added_notebooks": len(added), - "renamed_notebooks": len(renamed), + "renamed_notebooks": renamed, + "renamed_measured": len(renamed_measured), "credited_lost_unexempted": lost, "credited_diff_errors": diff_errors, # #18761 : le label ne se pose QUE si tous les diffs ont reussi. @@ -370,11 +453,16 @@ def main(argv: list[str] | None = None) -> int: mark = "LABEL" if row["would_label"] else " - " added = row.get("added_notebooks", 0) renamed = row.get("renamed_notebooks", 0) + renamed_measured = row.get("renamed_measured", 0) + renamed_unmeasured = renamed - renamed_measured suffix = "" if added: suffix += f" (+{added} neuf(s), rien a perdre)" - if renamed: - suffix += f" ({renamed} renomme(s), NON MESURE(S) : base a un autre chemin)" + if renamed_measured: + suffix += f" ({renamed_measured} renomme(s) mesure(s) via previousFilename)" + if renamed_unmeasured: + suffix += (f" ({renamed_unmeasured} renomme(s) NON MESURE(S) : " + "previousFilename absent)") print(f" [{mark}] #{row['number']}: {row['notebooks']} carnet(s) modifie(s), " f"pertes non exemptees={row['credited_lost_unexempted']}, " f"erreurs de diff={row['credited_diff_errors']}{suffix}") diff --git a/scripts/tests/test_credited_examples_sweep.py b/scripts/tests/test_credited_examples_sweep.py index b5e3c668e5..49fce44609 100644 --- a/scripts/tests/test_credited_examples_sweep.py +++ b/scripts/tests/test_credited_examples_sweep.py @@ -75,9 +75,17 @@ def _pr(number, base="a" * 40, head="b" * 40): } -def _nb(path, change="MODIFIED"): - """Un noeud GraphQL `pullRequest.files.nodes { path changeType }`.""" - return {"path": path, "changeType": change} +def _nb(path, change="MODIFIED", previous=None): + """Un noeud GraphQL `pullRequest.files.nodes { path changeType previousFilename }`. + + `previous` n'est pose que quand il est fourni : un noeud sans la cle simule + la reponse degradee (renommage sous seuil de similarite) que #19251 doit + continuer de nommer NON MESURE, pas de mesurer contre le mauvais chemin. + """ + node = {"path": path, "changeType": change} + if previous is not None: + node["previousFilename"] = previous + return node def _files_of(mapping): @@ -98,8 +106,8 @@ def _sweep(prs, *, files=None, body=None, base_of=None, ensure=None, check=None) fetch_body=body or (lambda repo, number: ""), base_of=base_of or (lambda repo_dir, b, h: "deadbeef"), ensure=ensure or (lambda repo_dir, sha: True), - check=check or (lambda paths, base_ref="", head_ref="", pr_body="": - _FakeResult()), + check=check or (lambda paths, base_ref="", head_ref="", pr_body="", + base_path_of=None: _FakeResult()), ) @@ -162,6 +170,62 @@ def run(argv): assert "cursor=CUR1" not in calls[0] +class TestTheRenameBasePathComesFromRest: + """#19251 : `previousFilename` n'existe PAS cote GraphQL. + + Mesure : le serveur repond `Field 'previousFilename' doesn't exist on type + 'PullRequestChangedFile'` (ses champs : additions, changeType, deletions, + path, viewerViewedState). Le chemin de base d'un renommage est lu par une + seconde passe REST (`pulls/{n}/files`, champ `previous_filename`), et + SEULEMENT quand la PR porte un renommage. + """ + + @staticmethod + def _graphql(nodes): + return {"data": {"repository": {"pullRequest": {"files": { + "nodes": nodes, "pageInfo": {"hasNextPage": False}}}}}} + + def test_a_renamed_node_gets_its_base_path_from_rest(self): + calls = [] + + def run(argv): + calls.append(list(argv)) + if "graphql" in argv: + return self._graphql( + [{"path": "new.ipynb", "changeType": "RENAMED"}]) + return [{"filename": "new.ipynb", "previous_filename": "old.ipynb", + "status": "renamed"}] + + nodes = _mod.pr_files("o/r", 7, run=run) + assert nodes[0]["previousFilename"] == "old.ipynb" + assert any("pulls/7/files" in arg for arg in calls[-1]), \ + f"la passe REST doit viser pulls//files : {calls[-1]}" + + def test_the_rest_pass_is_skipped_when_no_file_is_renamed(self): + calls = [] + + def run(argv): + calls.append(list(argv)) + return self._graphql([{"path": "x.ipynb", "changeType": "MODIFIED"}]) + + nodes = _mod.pr_files("o/r", 7, run=run) + assert nodes[0].get("previousFilename", "") == "" + assert len(calls) == 1, "sans renommage, une seule source est interrogee" + + def test_a_failing_rest_pass_leaves_the_rename_unmeasured_not_wrong(self): + """L'echec de la passe REST ne doit ni planter, ni faire mesurer le + renommage contre le mauvais chemin : sans chemin de base, il reste NON + MESURE (rendu `""`, que `_ipynb_by_change` range a part).""" + def run(argv): + if "graphql" in argv: + return self._graphql( + [{"path": "new.ipynb", "changeType": "RENAMED"}]) + raise RuntimeError("gh failed (1): rate limited") + + nodes = _mod.pr_files("o/r", 7, run=run) + assert nodes[0]["previousFilename"] == "" + + class TestFiltering: def test_only_ipynb_paths_are_kept(self): assert _mod.ipynb_paths([_nb(IPY), _nb(MD)]) == [IPY] @@ -190,14 +254,17 @@ def test_added_notebooks_are_not_measured(self): Ce n'est PAS une erreur de mesure (#18761) -- et la compter comme telle bloquait le label pour les carnets MODIFIES de la meme PR. """ - modified, added, renamed = _mod._ipynb_by_change([_nb(IPY, "ADDED")]) - assert modified == [] and added == [IPY] and renamed == [] + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( + [_nb(IPY, "ADDED")]) + assert modified == [] and added == [IPY] + assert renamed_m == [] and renamed_u == [] def test_added_and_modified_split_in_the_same_pr(self): other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" - modified, added, renamed = _mod._ipynb_by_change( + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( [_nb(other, "ADDED"), _nb(IPY, "MODIFIED")]) - assert modified == [IPY] and added == [other] and renamed == [] + assert modified == [IPY] and added == [other] + assert renamed_m == [] and renamed_u == [] def test_a_purely_additive_pr_is_named_not_dropped(self): rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY, "ADDED")]}) @@ -206,30 +273,82 @@ def test_a_purely_additive_pr_is_named_not_dropped(self): assert rows[0]["would_label"] is False assert errs == [] - def test_a_renamed_notebook_is_declared_unmeasured_not_lost_free(self): - """Un renommage PEUT perdre des exemples : on ne le dit pas « sans perte ». + def test_a_renamed_notebook_without_previous_is_unmeasured_not_lost_free(self): + """Un renommage sans `previousFilename` PEUT perdre des exemples : on ne + le dit pas « sans perte ». - La base est a un autre chemin (`previousFilename` hors du jeu GraphQL - demande ici), donc la comparaison est impossible. Le carnet ne doit ni - etre mesure contre le mauvais chemin, ni etre tu. + `previousFilename` manque (renommage sous seuil de similarite, ou + reponse d'API degradee) : la base est a un autre chemin inconnu, donc la + comparaison est impossible. Le carnet ne doit ni etre mesure contre le + mauvais chemin, ni etre tu (#19251 preserve ce repli). """ - modified, added, renamed = _mod._ipynb_by_change([_nb(IPY, "RENAMED")]) - assert modified == [] and added == [] and renamed == [IPY] + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( + [_nb(IPY, "RENAMED")]) + assert modified == [] and added == [] + assert renamed_m == [] and renamed_u == [IPY] rows, errs = _sweep([_pr(1)], files={1: [_nb(IPY, "RENAMED")]}) assert len(rows) == 1 and rows[0]["renamed_notebooks"] == 1 + assert rows[0]["renamed_measured"] == 0 assert rows[0]["notebooks"] == 0 assert rows[0]["would_label"] is False + def test_a_renamed_notebook_with_previous_is_measured_as_modified(self): + """#19251 : quand `previousFilename` est connu, un renommage est mesure. + + Sa tete se lit au nouveau chemin, sa base a l'ancien -- un `git mv` + suivi d'une edition peut perdre des exemples comme un MODIFIED. + """ + old = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" + new = "MyIA.AI.Notebooks/Search/Part2/y.ipynb" + modified, added, renamed_m, renamed_u = _mod._ipynb_by_change( + [_nb(new, "RENAMED", previous=old)]) + assert modified == [] and added == [] + assert renamed_m == [(new, old)] and renamed_u == [] + rows, errs = _sweep( + [_pr(1)], files={1: [_nb(new, "RENAMED", previous=old)]}, + check=lambda paths, base_ref="", head_ref="", pr_body="", + base_path_of=None: _FakeResult(lost=3, diff_errors=0), + ) + assert errs == [] + assert rows[0]["renamed_notebooks"] == 1 + assert rows[0]["renamed_measured"] == 1 + assert rows[0]["notebooks"] == 1 + assert rows[0]["credited_lost_unexempted"] == 3 + assert rows[0]["would_label"] is True + + def test_the_base_path_of_a_rename_reaches_the_check(self): + """Le contrat de la carte : `base_path_of` porte la correspondance + nouveau chemin -> ancien chemin, pour le cote base du diff credite.""" + old = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" + new = "MyIA.AI.Notebooks/Search/Part2/y.ipynb" + seen = {} + + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): + seen["paths"] = [str(p) for p in paths] + seen["map"] = dict(base_path_of or {}) + return _FakeResult() + + _sweep( + [_pr(1)], files={1: [_nb(new, "RENAMED", previous=old)]}, + check=check, + ) + assert seen["paths"] == [str(Path(new))] + # la carte est clee en POSIX (le chemin tel que rendu par GraphQL), pas + # dans la forme native du systeme : c'est ce que `check_notebooks` + # normalise avant de la consulter (cf. le test de lookup ci-dessous). + assert seen["map"] == {new: old} + def test_a_renamed_notebook_does_not_block_the_other_notebooks(self): """Le faux positif d'erreur produisait un faux zero de pertes (#18761). - Un carnet renomme dans la meme PR ne doit plus empecher la mesure des - carnets modifies -- c'est le defaut que la separation corrige. + Un carnet renomme SANS chemin de base dans la meme PR ne doit pas + empecher la mesure des carnets modifies -- c'est le defaut que la + separation corrige. """ other = "MyIA.AI.Notebooks/Search/Part1/y.ipynb" seen = [] - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): seen.append([str(p) for p in paths]) return _FakeResult(lost=2, diff_errors=0) @@ -240,6 +359,7 @@ def check(paths, base_ref="", head_ref="", pr_body=""): ) assert rows[0]["would_label"] is True, rows assert rows[0]["renamed_notebooks"] == 1 + assert rows[0]["renamed_measured"] == 0 assert seen == [[str(Path(IPY))]] assert errs == [] @@ -258,7 +378,7 @@ class TestHeadIsThePrHead: def test_the_check_receives_the_head_of_the_pr(self): captured = {} - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): captured["base_ref"] = base_ref captured["head_ref"] = head_ref return _FakeResult(1, 0) @@ -394,7 +514,7 @@ class TestARenamedNotebookNoLongerTakesDownTheSweep: """ def test_a_failing_check_names_the_pr_and_the_others_are_still_measured(self): - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): if paths and "ICT-45" in str(paths[0]): raise FileNotFoundError( "MyIA.AI.Notebooks/IIT/ICT-Series/ICT-45-...ipynb") @@ -420,7 +540,7 @@ def test_only_a_missing_notebook_is_named_other_errors_still_propagate(self): se derober en « carnet renomme ». Un repli trop large remplacerait une panne par un chiffre manquant, ce que la fenetre ne distinguerait pas d'une PR sans perte.""" - def check(paths, base_ref="", head_ref="", pr_body=""): + def check(paths, base_ref="", head_ref="", pr_body="", base_path_of=None): raise RuntimeError("bug du compteur") with pytest.raises(RuntimeError): @@ -443,7 +563,8 @@ def _git(repo, *args): subprocess.run( ["git", "-C", str(repo), "-c", "user.name=t", "-c", "user.email=t@t", "-c", "core.autocrlf=false", *args], - check=True, capture_output=True, text=True, + check=True, capture_output=True, text=True, encoding="utf-8", + errors="replace", ) @@ -469,12 +590,14 @@ def _repo_with_head(self, tmp_path, *, tree): _git(repo, "add", "-A") _git(repo, "commit", "-q", "-m", "base") base = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], - capture_output=True, text=True, check=True).stdout.strip() + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() target.write_text(json.dumps(_notebook_json(3)), encoding="utf-8") _git(repo, "add", "-A") _git(repo, "commit", "-q", "-m", "head") head = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], - capture_output=True, text=True, check=True).stdout.strip() + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() if tree is None: target.unlink() else: @@ -513,3 +636,117 @@ def test_without_a_head_ref_the_tree_still_serves(self, tmp_path, monkeypatch): monkeypatch.chdir(repo) result = _cpe.check_notebooks([Path(self.REL)]) assert self._count(result) == 2 + + +def _credited_notebook_json(n, *, tag): + """Un carnet a ``n`` exemples credits, chacun d'identite distincte. + + L'identite d'un exemple credite est ``(credit, cell_id)`` : un exemple + conserve au renommage garde le MEME ``cell_id`` des deux cotes, sinon il + se lirait comme perdu puis reajoute. ``tag`` sert a distinguer des carnets + differents d'un meme test, pas la base de la tete. + """ + cells = [] + for i in range(1, n + 1): + cells.append({ + "cell_type": "markdown", "id": f"{tag}-ex{i}", "metadata": {}, + "source": [f"### Exemple {i}\n", f"[crédité #{1000 + i}]\n"], + }) + cells.append({ + "cell_type": "code", "id": f"{tag}-c{i}", "metadata": {}, + "execution_count": 1, "outputs": [], "source": [f"print({i})\n"], + }) + return {"cells": cells, "metadata": {}, "nbformat": 4, "nbformat_minor": 5} + + +class TestARenamedNotebookIsMeasuredAtItsOldPath: + """#19251 : un RENAMED dont `previousFilename` est connu est MESURE. + + Le renommage est le cas aveugle qui a motive #19251 : la base est a un + autre chemin, donc `base:path` ne trouve rien. Sans la correspondance, la + mesure tombe -- et une mesure tombee se lirait soit comme zero perte (si + on avalait l'erreur), soit comme un diff en erreur (honnete mais inutile). + Ici on epingle la VRAIE mesure : le carnet renomme qui perd un exemple + credite est vu, sur un depot git reel. + """ + + OLD = "MyIA.AI.Notebooks/Search/Part1/moved.ipynb" + NEW = "MyIA.AI.Notebooks/Search/Part2/moved.ipynb" + + def _repo_with_rename(self, tmp_path, *, keep=2): + """Commit base : 3 exemples credites a OLD. Commit tete : `git mv` vers + NEW, ``keep`` exemples conserves (identites stables) -- ``keep=2`` perd + le 3e, ``keep=3`` ne perd rien.""" + repo = tmp_path / "repo" + (repo / Path(self.OLD).parent).mkdir(parents=True) + _git(repo, "init", "-q") + (repo / self.OLD).write_text( + json.dumps(_credited_notebook_json(3, tag="ex")), encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "base") + base = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() + (repo / Path(self.NEW).parent).mkdir(parents=True) + _git(repo, "mv", self.OLD, self.NEW) + (repo / self.NEW).write_text( + json.dumps(_credited_notebook_json(keep, tag="ex")), encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "head") + head = subprocess.run(["git", "-C", str(repo), "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + encoding="utf-8", errors="replace").stdout.strip() + return repo, base, head + + def _verdict(self, result, name): + for bucket in (result.ok, result.sub_threshold, result.parse_errors): + for v in bucket: + if v.path.endswith(name): + return v + raise AssertionError("le carnet n'apparait dans aucun seau") + + def test_the_loss_of_a_renamed_notebook_is_measured(self, tmp_path, monkeypatch): + repo, base, head = self._repo_with_rename(tmp_path) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks( + [Path(self.NEW)], base_ref=base, head_ref=head, + base_path_of={self.NEW: self.OLD}, + ) + verdict = self._verdict(result, "moved.ipynb") + assert verdict.credited_diff_status == "ok", \ + "avec previousFilename, le diff doit etre calcule, pas en erreur" + assert verdict.credited_lost_unexempted == 1, \ + "l'exemple credite perdu au renommage doit etre vu" + + def test_a_rename_without_loss_is_not_a_false_positive( + self, tmp_path, monkeypatch): + """Un renommage qui ne perd rien ne doit pas etre signale. + + Sans ce controle, la mesure du renommage pourrait crier a la perte sur + un simple `git mv` -- le faux positif que #19251 doit eviter autant que + l'angle mort qu'il ferme. + """ + repo, base, head = self._repo_with_rename(tmp_path, keep=3) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks( + [Path(self.NEW)], base_ref=base, head_ref=head, + base_path_of={self.NEW: self.OLD}, + ) + verdict = self._verdict(result, "moved.ipynb") + assert verdict.credited_diff_status == "ok" + assert verdict.credited_lost_unexempted == 0, \ + "aucun exemple perdu au renommage : rien a signaler" + + def test_without_the_mapping_the_rename_is_not_silently_a_zero( + self, tmp_path, monkeypatch): + """Sans `previousFilename`, on ne MESURE pas -- et surtout on ne rend + pas « zero perte » : le diff reste en erreur, que #18761 refuse de + convertir en label. Un zero silencieux serait le vrai mensonge.""" + repo, base, head = self._repo_with_rename(tmp_path) + monkeypatch.chdir(repo) + result = _cpe.check_notebooks( + [Path(self.NEW)], base_ref=base, head_ref=head) + verdict = self._verdict(result, "moved.ipynb") + assert verdict.credited_diff_status.startswith("error:"), \ + "base absente a ce chemin : le diff doit etre en erreur, pas a zero" + assert verdict.credited_lost_unexempted == 0