fix: let a refresh failure reach the exit code

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)
 

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

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(
+        outcomes = {
-            args.key, registry[args.key], force=args.force, dry_run=args.dry_run
+            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()

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()

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__":
+class TestEntryPointsRunEveryClass(unittest.TestCase):
-    unittest.main()
+    """`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()

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()

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()