From 1c976390d74ccff5601e738560f8d1accf6b1ae6 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Mon, 7 Sep 2026 05:30:56 +0200 Subject: [PATCH] fix: let a refresh failure reach the exit code --- scripts/generate_pack.py | 3 +++ scripts/pipeline.py | 4 +++- scripts/refresh_data_dirs.py | 28 ++++++++++++++++--------- tests/test_agnostic_scan.py | 6 ++++-- tests/test_audit_regressions.py | 36 +++++++++++++++++++++++++++++++-- tests/test_hash_merge.py | 6 +++--- tests/test_native_mode.py | 8 ++++---- 7 files changed, 70 insertions(+), 21 deletions(-) diff --git a/scripts/generate_pack.py b/scripts/generate_pack.py index 750a66d8..4491cc83 100644 --- a/scripts/generate_pack.py +++ b/scripts/generate_pack.py @@ -2579,6 +2579,9 @@ def main(): updated = sum(1 for v in results.values() if v) if updated: print(f"Refreshed {updated} data director{'ies' if updated > 1 else 'y'}") + failed = sorted(k for k, v in results.items() if v is None) + if failed: + print(f"WARNING: data directory refresh failed: {', '.join(failed)}") emu_profiles = load_emulator_profiles(args.emulators_dir) diff --git a/scripts/pipeline.py b/scripts/pipeline.py index acd94804..48feab43 100644 --- a/scripts/pipeline.py +++ b/scripts/pipeline.py @@ -408,10 +408,12 @@ def main(): # Step 3a: One content answering to two identities. Walks the tree rather # than the index, which keeps a single path per content and therefore # cannot see a file copied under another machine's name. - run( + ok, _ = run( [sys.executable, "scripts/identity.py", "--strict"], "3a/8 file identity", ) + results["identity"] = ok + all_ok = all_ok and ok # Step 3b: Destinations both layers claim, and how each was settled. The # ones the pack settles by itself must stay at zero cost; the rest name an diff --git a/scripts/refresh_data_dirs.py b/scripts/refresh_data_dirs.py index 41adb09b..c8fa6071 100644 --- a/scripts/refresh_data_dirs.py +++ b/scripts/refresh_data_dirs.py @@ -309,7 +309,7 @@ def refresh_entry( force: bool = False, dry_run: bool = False, versions_path: str = VERSIONS_FILE, -) -> bool: +) -> bool | None: """Refresh a single data directory entry. Returns True if the entry was refreshed (or would be in dry-run mode). @@ -334,7 +334,7 @@ def refresh_entry( remote_tag = get_remote_sha(entry["source_url"], version) if remote_tag is None: log.warning("[%s] could not check remote, skipping", key) - return False + return None needs_refresh = remote_tag != cached_tag if not needs_refresh: @@ -368,7 +368,7 @@ def refresh_entry( zipfile.BadZipFile, ) as exc: log.warning("[%s] download failed: %s", key, exc) - return False + return None if remote_tag is None: if source_type == "zip": @@ -390,12 +390,15 @@ def refresh_all( dry_run: bool = False, versions_path: str = VERSIONS_FILE, platform: str | None = None, -) -> dict[str, bool]: +) -> dict[str, bool | None]: """Refresh all entries in the registry. If platform is set, only refresh entries whose for_platforms includes that platform (or entries with no for_platforms restriction). - Returns a dict mapping key -> whether it was refreshed. + Returns a dict mapping key -> True when refreshed, False when already up + to date, None when the refresh failed. A single boolean conflated the last + two, so a run that reached no remote at all exited 0 like a run with + nothing to do. """ results = {} for key, entry in registry.items(): @@ -440,14 +443,21 @@ def main() -> None: if args.key not in registry: log.error("unknown key: %s (available: %s)", args.key, ", ".join(registry)) raise SystemExit(1) - refresh_entry( - args.key, registry[args.key], force=args.force, dry_run=args.dry_run - ) + outcomes = { + args.key: refresh_entry( + args.key, registry[args.key], force=args.force, dry_run=args.dry_run + ) + } else: - refresh_all( + outcomes = refresh_all( registry, force=args.force, dry_run=args.dry_run, platform=args.platform ) + failed = sorted(key for key, outcome in outcomes.items() if outcome is None) + if failed: + log.error("refresh failed: %s", ", ".join(failed)) + raise SystemExit(1) + if __name__ == "__main__": main() diff --git a/tests/test_agnostic_scan.py b/tests/test_agnostic_scan.py index 660d4d96..385398e5 100644 --- a/tests/test_agnostic_scan.py +++ b/tests/test_agnostic_scan.py @@ -252,8 +252,6 @@ class PathTailBeatsABareName(unittest.TestCase): self.assertEqual(status, "name_exact") -if __name__ == "__main__": - unittest.main() class ArchivePrefixCopies(unittest.TestCase): @@ -303,3 +301,7 @@ class ArchivePrefixCopies(unittest.TestCase): {"neogeo.zip": ["abc"]}, ) self.assertEqual(extras, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_audit_regressions.py b/tests/test_audit_regressions.py index 446331f5..c9b7c432 100644 --- a/tests/test_audit_regressions.py +++ b/tests/test_audit_regressions.py @@ -1275,8 +1275,36 @@ class PipelineReportsWhatItDid(unittest.TestCase): stale = re.findall(r'results\["(\w+)"\] = True', source) self.assertEqual(stale, [], f"steps still claiming OK when skipped: {stale}") -if __name__ == "__main__": - unittest.main() +class TestEntryPointsRunEveryClass(unittest.TestCase): + """`unittest.main()` has to sit after the last test class. + + In this file it sat in the middle, so running it directly discovered only + the classes defined above it: eight cases never ran that way, while + `python -m unittest discover` ran all of them. The two roads have to agree. + """ + + def test_no_module_calls_main_before_its_last_class(self): + for path in sorted((ROOT / "tests").glob("test_*.py")): + lines = path.read_text(encoding="utf-8").splitlines() + # Column zero only: the same text appears inside this very test as + # a string literal, and a substring search matched that instead. + entry = next( + (n for n, line in enumerate(lines) + if line.startswith("if __name__ ==")), + None, + ) + if entry is None: + continue + last_class = max( + (n for n, line in enumerate(lines) if line.startswith("class ")), + default=-1, + ) + self.assertGreater( + entry, + last_class, + f"{path.name} calls unittest.main() before its last class, so " + "running the file directly skips what follows", + ) class ScriptsImportThreeWays(unittest.TestCase): @@ -1399,3 +1427,7 @@ class ModuleConstantsDeclaredOnce(unittest.TestCase): if again: offenders[path.name] = again self.assertEqual(offenders, {}) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_hash_merge.py b/tests/test_hash_merge.py index 96a5c578..e4698140 100644 --- a/tests/test_hash_merge.py +++ b/tests/test_hash_merge.py @@ -485,9 +485,6 @@ class TestDiff(unittest.TestCase): self.assertEqual(diff["unchanged"], 1) -if __name__ == "__main__": - unittest.main() - class TestSourceRefDrift(unittest.TestCase): """A set whose ROMs are unchanged but whose driver line moved is an update.""" @@ -586,3 +583,6 @@ class TestDerivativeVersion(unittest.TestCase): entry = self._run(add_new)["files"][0] self.assertEqual(entry["source_ref"], "src/mame/philips/cdi.cpp:484") + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_native_mode.py b/tests/test_native_mode.py index d8884f0b..de203348 100644 --- a/tests/test_native_mode.py +++ b/tests/test_native_mode.py @@ -257,10 +257,6 @@ class GapAnalysisAgreesWithTheBuilder(unittest.TestCase): ) -if __name__ == "__main__": - unittest.main() - - class CandidateVerdict(unittest.TestCase): """Whether a profile entry can be a gap, and whether it is settled. @@ -479,3 +475,7 @@ class OneProfileSelector(unittest.TestCase): 'is a launcher -use the emulator it launches', text, f"{name} still spells the refusal itself", ) + + +if __name__ == "__main__": + unittest.main()