From 1e591dd9495edf43cba3fc4fbe16e8ca4488ae40 Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Wed, 19 Aug 2026 06:00:21 -0400 Subject: [PATCH] [FIX] migration state: a test result belongs to a step, not to the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit You were right that it read as global — and the cause ran deeper than the display. Twenty step headers go through add_comment_progression against seven through print_step, and the whole version-bump loop uses only the first. The current step was set in the other one, so every tool verdict of every bump carried a stale step. The header itself now becomes the step, at the one place the journal is already cut on. And the summary groups by (step, tool) instead of by tool alone: a migration runs the smoke test at EVERY bump, and one line said « smoke_public_url ✅ » while bump 14 had passed and bump 17 had fallen. Within a step the last verdict still wins, with its run count — that is a repair, not two bumps. The order is the journal's, because sorting step names puts « 4.10 » before « 4.2 ». --- FR --- [FIX] état de migration : un résultat de test appartient à une étape Vous aviez raison, cela se lisait comme global — et la cause était plus profonde que l'affichage. Vingt en-têtes d'étape passent par add_comment_progression contre sept par print_step, et toute la boucle des paliers n'utilise que la première. L'étape courante était posée dans l'autre : chaque verdict d'outil portait donc une étape périmée. L'en-tête devient désormais l'étape, au seul endroit où le journal est déjà découpé. Et le résumé groupe par (étape, outil) : une migration lance le test de fumée à CHAQUE palier, et une seule ligne annonçait « smoke_public_url ✅ » quand le palier 14 était passé et le 17 tombé. Dans une étape, le dernier verdict l'emporte toujours — c'est une réparation, pas deux paliers. Assisted-by: Claude Opus 5 --- script/todo/migration_status.py | 59 +++++++---- script/todo/migration_status_tui.py | 9 +- script/todo/todo_upgrade.py | 13 +++ test/test_migration_status.py | 154 ++++++++++++++++++++++++++++ 4 files changed, 214 insertions(+), 21 deletions(-) diff --git a/script/todo/migration_status.py b/script/todo/migration_status.py index 579e10c..bba12b9 100755 --- a/script/todo/migration_status.py +++ b/script/todo/migration_status.py @@ -242,22 +242,39 @@ def verdict(status): def tests_summary(dct): - """Le DERNIER verdict de chaque outil, et le compte des passages. + """Le dernier verdict de chaque outil, PAR ÉTAPE, et son nombre de passages. - Un outil relancé après correction a deux verdicts contradictoires dans - le journal, et c'est le dernier qui décrit la base telle qu'elle est. - Afficher les deux sans les distinguer ferait lire une réparation comme - un échec persistant. + Par étape, car c'est la question qu'on pose. Une migration lance le + test de fumée à CHAQUE palier ; regrouper sur le seul nom d'outil n'en + laissait qu'une ligne, et l'on lisait « smoke_public_url ✅ » sans voir + que le palier 14 était passé et le 17 tombé. + + Dans une étape, le DERNIER verdict l'emporte : un outil relancé après + correction a deux verdicts contradictoires, et c'est le second qui + décrit la base telle qu'elle est. Les afficher tous deux sans les + distinguer ferait lire une réparation comme un échec persistant. + + L'ordre est celui du journal, donc celui de la migration. Trier les + étapes par leur nom mettrait « 4.10 » avant « 4.2 ». """ dernier = {} for item in events(dct, kind="test"): - nom = item.get("name") or "?" - entree = dernier.setdefault(nom, {"name": nom, "runs": 0}) + cle = (item.get("step") or "", item.get("name") or "?") + entree = dernier.setdefault( + cle, {"name": cle[1], "step": cle[0], "runs": 0} + ) entree["runs"] += 1 entree["status"] = item.get("status") entree["at"] = item.get("at") - entree["step"] = item.get("step") - return [dernier[nom] for nom in sorted(dernier)] + return list(dernier.values()) + + +def tests_by_step(dct): + """Les verdicts groupés sous leur étape, dans l'ordre de la migration.""" + par_etape = {} + for item in tests_summary(dct): + par_etape.setdefault(item["step"], []).append(item) + return list(par_etape.items()) def failures(dct): @@ -325,18 +342,22 @@ def render_text(dct, limit_cmd=12, colour=None): lignes.append(f"\n🧪 {t('Test results')}") if not lst_test: lignes.append(f" {t('No tool has run yet.')}") - for item in lst_test: - icone, phrase = verdict(item.get("status")) - rejeu = ( - f" ({item['runs']} {t('runs')})" - if item.get("runs", 1) > 1 - else "" - ) - teinte = VERDICT_COLOUR.get(item.get("status"), "dim") - nom = f"{item['name']:<24}" + for etape, lst_item in tests_by_step(dct): lignes.append( - f" {icone} {paint(nom, teinte, colour)} {phrase}{rejeu}" + f" {paint(etape or t('before the first step'), 'step', colour)}" ) + for item in lst_item: + icone, phrase = verdict(item.get("status")) + rejeu = ( + f" ({item['runs']} {t('runs')})" + if item.get("runs", 1) > 1 + else "" + ) + teinte = VERDICT_COLOUR.get(item.get("status"), "dim") + nom = f"{item['name']:<24}" + lignes.append( + f" {icone} {paint(nom, teinte, colour)} {phrase}{rejeu}" + ) lst_failure = failures(dct) lignes.append(f"\n❌ {t('Commands that failed')} : {len(lst_failure)}") diff --git a/script/todo/migration_status_tui.py b/script/todo/migration_status_tui.py index 93ec246..865eb7d 100644 --- a/script/todo/migration_status_tui.py +++ b/script/todo/migration_status_tui.py @@ -64,10 +64,15 @@ def rows(dct): lst = [] for item in status.tests_summary(dct): icone, _phrase = status.verdict(item.get("status")) + # Le NUMÉRO de l'étape en tête : une migration lance le même outil + # à chaque palier, et sans lui la liste montrait six lignes + # identiques sans dire laquelle appartenait à quel palier. + numero = (item.get("step") or "").split(" - ")[0] + tete = f"{numero} " if numero else "" lst.append( { "kind": "test", - "label": f"{icone} {item['name']}", + "label": f"{icone} {tete}{item['name']}", "detail": str(item.get("runs", 1)), "data": item, } @@ -99,9 +104,9 @@ def pane_text(dct, row, colour=False): teinte = status.VERDICT_COLOUR.get(item.get("status"), "dim") lignes = [ f"{icone} {status.paint(item['name'], teinte, colour)}", + f" {status.paint(item.get('step') or '?', 'step', colour)}", f" {phrase} ({t('exit code')} {item.get('status')})", f" {t('runs')} : {item.get('runs')}", - f" {t('current step')} : {item.get('step') or '?'}", f" {item.get('at') or ''}", ] return "\n".join(lignes) diff --git a/script/todo/todo_upgrade.py b/script/todo/todo_upgrade.py index 96b4c42..d0e98d3 100755 --- a/script/todo/todo_upgrade.py +++ b/script/todo/todo_upgrade.py @@ -4044,7 +4044,20 @@ class TodoUpgrade: return status def add_comment_progression(self, comment): + """Marquer une étape dans le journal, et devenir CETTE étape. + + Vingt en-têtes d'étape passent par ici contre sept par + `print_step` : toute la boucle des paliers n'utilise que celle-ci. + Ne poser l'étape courante que dans l'autre laissait donc chaque + verdict d'outil estampillé d'une étape périmée — et le résumé des + tests semblait global alors qu'il aurait dû suivre les paliers. + + C'est le MÊME marqueur que `journal_by_step` découpe : une seule + notion d'étape, posée à un seul endroit. + """ comment_to_add = f"# {comment}" self.lst_command_executed.append(comment_to_add) self.dct_progression["command_executed"] = self.lst_command_executed + self.current_step = comment + self.open_step_log(comment) self.write_config() diff --git a/test/test_migration_status.py b/test/test_migration_status.py index 9b98fb8..f7b9426 100644 --- a/test/test_migration_status.py +++ b/test/test_migration_status.py @@ -123,6 +123,120 @@ class TestARepairMustNotReadAsAFailure(Base): smoke = [x for x in lst if x["name"] == "smoke_public_url"][0] self.assertEqual(smoke["status"], 0) + def test_the_verdicts_are_kept_PER_STEP(self): + """La demande, et le défaut qu'elle a révélé. + + Une migration lance le test de fumée à CHAQUE palier. Regrouper sur + le seul nom d'outil n'en laissait qu'une ligne : on lisait + « smoke_public_url ✅ » sans voir que le palier 14 était passé et + le 17 tombé. + """ + dct = progression( + lst_event=[ + { + "at": "1", + "step": "4.1 - v14", + "kind": "test", + "name": "smoke_public_url", + "status": 0, + }, + { + "at": "2", + "step": "4.2 - v15", + "kind": "test", + "name": "smoke_public_url", + "status": 2, + }, + ] + ) + lst = status.tests_summary(dct) + self.assertEqual(len(lst), 2) + self.assertEqual( + [(x["step"], x["status"]) for x in lst], + [("4.1 - v14", 0), ("4.2 - v15", 2)], + ) + + def test_a_repair_within_a_step_still_collapses(self): + # Deux verdicts pour le MÊME palier : c'est une réparation, pas + # deux paliers. Le second décrit la base telle qu'elle est. + dct = progression( + lst_event=[ + { + "at": "1", + "step": "4.2 - v15", + "kind": "test", + "name": "smoke_public_url", + "status": 2, + }, + { + "at": "2", + "step": "4.2 - v15", + "kind": "test", + "name": "smoke_public_url", + "status": 0, + }, + ] + ) + lst = status.tests_summary(dct) + self.assertEqual(len(lst), 1) + self.assertEqual(lst[0]["status"], 0) + self.assertEqual(lst[0]["runs"], 2) + + def test_the_order_is_the_migration_s_own(self): + # Trier les étapes par leur nom mettrait « 4.10 » avant « 4.2 ». + dct = progression( + lst_event=[ + { + "at": "1", + "step": "4.2 - v15", + "kind": "test", + "name": "outil", + "status": 0, + }, + { + "at": "2", + "step": "4.10 - v18", + "kind": "test", + "name": "outil", + "status": 0, + }, + ] + ) + self.assertEqual( + [etape for etape, _lst in status.tests_by_step(dct)], + ["4.2 - v15", "4.10 - v18"], + ) + + def test_the_report_shows_the_step_number(self): + dct = progression( + lst_event=[ + { + "at": "1", + "step": "4.1 - Ready to work with version 14", + "kind": "test", + "name": "database_cleanup", + "status": 0, + }, + ] + ) + texte = status.render_text(dct, colour=False) + self.assertIn("4.1 - Ready to work with version 14", texte) + + def test_the_full_screen_shows_it_too(self): + dct = progression( + lst_event=[ + { + "at": "1", + "step": "4.1 - v14", + "kind": "test", + "name": "smoke_public_url", + "status": 0, + }, + ] + ) + ligne = [x for x in tui.rows(dct) if x["kind"] == "test"][0] + self.assertIn("4.1", ligne["label"]) + def test_but_the_earlier_runs_are_still_counted(self): # Les taire ferait croire à un premier essai réussi, et l'on # perdrait la trace de ce qui a demandé une réparation. @@ -439,6 +553,46 @@ class TestWhatSurvivesClosingTheTool(DiskCase): self.assertIn("premier", noms) self.assertIn("dernier", noms) + def test_a_step_header_becomes_the_current_step(self): + """La cause racine de « ça semble global ». + + Vingt en-têtes d'étape passent par `add_comment_progression` contre + sept par `print_step`, et toute la boucle des paliers n'utilise que + la première. Ne poser l'étape courante que dans l'autre laissait + chaque verdict estampillé d'une étape périmée. + """ + obj = self.upgrade() + obj.add_comment_progression("4.2 - Ready to work with version 15") + obj.record_event("test", "smoke_public_url", 0) + obj.close_step_log() + lst = status.merge_events({"config_database_name": "essai_db"}) + self.assertEqual(lst[0]["step"], "4.2 - Ready to work with version 15") + + def test_a_step_header_opens_its_log_too(self): + # Même raison : sans cela, la sortie des commandes d'un palier + # allait dans le fichier de l'étape d'AVANT. + obj = self.upgrade() + obj.add_comment_progression("4.2 - Migrate database") + obj.note_step_log("quelque chose") + obj.close_step_log() + self.assertIsNotNone( + status.step_log_path( + {"config_database_name": "essai_db"}, "4.2 - Migrate database" + ) + ) + + def test_two_bumps_do_not_share_a_step(self): + # Le symptôme exact : six paliers, un seul nom d'étape. + obj = self.upgrade() + for palier in ("4.1 - version 14", "4.2 - version 15"): + obj.add_comment_progression(palier) + obj.record_event("test", "smoke_public_url", 0) + obj.close_step_log() + lst = status.merge_events({"config_database_name": "essai_db"}) + self.assertEqual( + [x["step"] for x in lst], ["4.1 - version 14", "4.2 - version 15"] + ) + def test_a_migration_without_a_database_writes_nowhere(self): obj = self.upgrade(database=None) obj.dct_progression = {}