diff --git a/scripts/validate_pr.py b/scripts/validate_pr.py index a7f3a0fc0..c1a702816 100644 --- a/scripts/validate_pr.py +++ b/scripts/validate_pr.py @@ -36,6 +36,7 @@ from common import ( ) from hashing import parse_hash_list from nativemode import digest_algorithm +from dedup import _in_nodedup_dir from validation import frontend_digest_matches, inner_rom_check try: @@ -79,6 +80,10 @@ class ValidationResult: self.md5 = "" self.crc32 = "" self.size = 0 + # "held" (a copy of a file the collection has) or "unknown" (no list + # or profile declares it): no check fails, and neither is a file + # the pull request adds to the collection. + self.outcome = "" def add_check(self, passed: bool, message: str) -> None: self.checks.append(("PASS" if passed else "FAIL", message)) @@ -93,8 +98,12 @@ class ValidationResult: def passed(self) -> bool: return all(s != "FAIL" for s, _ in self.checks) + @property + def status(self) -> str: + return "FAIL" if not self.passed else "REVIEW" if self.outcome else "PASS" + def to_markdown(self) -> str: - status = "PASS" if self.passed else "FAIL" + status = self.status lines = [f"### {status} `{self.filename}`"] lines.append("") lines.append(f"- **Path**: `{self.filepath}`") @@ -374,9 +383,15 @@ def validate_file( # PR that regenerated database.json was told each new file # duplicated itself. result.add_check(True, "Already in the database at this path") + elif _in_nodedup_dir(relative): + result.add_info( + f"Identical to `{existing['path']}`, kept: this tree is read by path" + ) else: + result.outcome = "held" result.add_warning( - f"Duplicate: identical file already exists at `{existing['path']}`" + f"Already held at `{existing['path']}`: nothing to add, " + "leave this copy out of the pull request" ) else: result.add_check(True, "Not a duplicate in database") @@ -421,8 +436,11 @@ def validate_file( "Filename matches a known requirement but hash differs - may be a variant" ) else: + result.outcome = "unknown" result.add_warning( - "File not referenced in any platform config - needs manual review" + "Not recognised: no platform list or emulator profile declares this " + "file. Say in the pull request which emulator reads it, for which " + "system, and where the dump comes from" ) normalized = os.path.normpath(relative) @@ -452,6 +470,8 @@ def validate_file( result.add_check(True, f"Correct placement: bios/{parts[1]}/{parts[2]}/") else: result.add_warning("File should be in bios/Manufacturer/Console/ structure") + elif result.outcome == "held": + pass elif expected_folders: # The contributor asks before forking: say where it goes. result.add_warning( @@ -750,6 +770,7 @@ def main() -> None: { "file": r.filepath, "passed": r.passed, + "outcome": r.outcome or "new", "sha1": r.sha1, "md5": r.md5, "size": r.size, @@ -759,7 +780,15 @@ def main() -> None: print(json.dumps(output, indent=2)) elif args.markdown: lines = ["## BIOS Validation Report", ""] - status = "all checks passed" if all_passed else "some checks failed" + held = sum(r.outcome == "held" for r in results) + unknown = sum(r.outcome == "unknown" for r in results) + review = ", ".join(part for part in ( + f"{held} already held" if held else "", + f"{unknown} not recognised" if unknown else "", + ) if part) + status = ("some checks failed" if not all_passed + else f"no check failed, to review: {review}" if review + else "all checks passed") lines.append(f"**Status**: {status}") lines.append("") @@ -770,8 +799,7 @@ def main() -> None: print("\n".join(lines)) else: for r in results: - status = "PASS" if r.passed else "FAIL" - print(f"\n[{status}] {r.filepath}") + print(f"\n[{r.status}] {r.filepath}") if r.sha1: print(f" SHA1: {r.sha1}") print(f" MD5: {r.md5}") diff --git a/tests/test_validate_pr.py b/tests/test_validate_pr.py index 93575737d..04b7a78fa 100644 --- a/tests/test_validate_pr.py +++ b/tests/test_validate_pr.py @@ -243,20 +243,38 @@ class HashRecognition(_Fixture): self.assertIn(".variants/known.bin.", markdown) def test_an_unreferenced_file_asks_for_review(self): + """Random bytes read "PASS" and "all checks passed", and the warning + named the platform configs alone though the profiles were searched.""" rel = self._bios_file("bios/Sony/PS/mystery.bin") - markdown = validate_pr.validate_file(rel, None, EMPTY_HASHES).to_markdown() - self.assertIn("needs manual review", markdown) + result = validate_pr.validate_file(rel, None, EMPTY_HASHES) + markdown = result.to_markdown() + self.assertEqual(result.outcome, "unknown") + self.assertIn("### REVIEW", markdown) + self.assertIn("no platform list or emulator profile declares this file", markdown) + self.assertIn("which emulator reads it", markdown) def test_a_duplicate_of_a_known_file_is_reported(self): - rel = self._bios_file("bios/Sony/PS/dupe.bin") + """A copy of a held file passed, and was sent to the folder that + already holds it.""" + rel = self._bios_file("elsewhere/dupe.bin") probe = validate_pr.validate_file(rel, None, EMPTY_HASHES) db = {"files": {probe.sha1: {"path": "bios/Sony/PS/original.bin"}}} - markdown = validate_pr.validate_file(rel, db, EMPTY_HASHES).to_markdown() - self.assertIn("Duplicate", markdown) - self.assertIn("original.bin", markdown) + known = {**EMPTY_HASHES, "sha1": {probe.sha1}, + "folders": {probe.sha1: {"bios/Sony/PS/"}}} + result = validate_pr.validate_file(rel, db, known) + markdown = result.to_markdown() + self.assertEqual(result.outcome, "held") + self.assertIn("### REVIEW", markdown) + self.assertIn("Already held at `bios/Sony/PS/original.bin`: nothing to add", markdown) + self.assertNotIn("not under bios/", markdown) # A duplicate is worth saying out loud but must not block the PR. - self.assertTrue(validate_pr.validate_file(rel, db, EMPTY_HASHES).passed) + self.assertTrue(result.passed) + def test_a_copy_in_a_tree_read_by_path_is_kept(self): + rel = self._bios_file("bios/ScummVM/a/dupe.bin") + probe = validate_pr.validate_file(rel, None, EMPTY_HASHES) + db = {"files": {probe.sha1: {"path": "bios/ScummVM/b/dupe.bin"}}} + self.assertNotEqual(validate_pr.validate_file(rel, db, EMPTY_HASHES).outcome, "held") def test_the_file_the_database_holds_is_not_its_own_duplicate(self): """The documented example on a tracked file warned "Duplicate: @@ -334,7 +352,8 @@ class Placement(_Fixture): class Reporting(_Fixture): def test_markdown_carries_every_digest_and_the_verdict(self): rel = self._bios_file("bios/Sega/Saturn/sega.bin") - result = validate_pr.validate_file(rel, None, EMPTY_HASHES) + probe = validate_pr.validate_file(rel, None, EMPTY_HASHES) + result = validate_pr.validate_file(rel, None, {**EMPTY_HASHES, "sha1": {probe.sha1}}) markdown = result.to_markdown() for value in (result.sha1, result.md5, result.crc32): self.assertIn(value, markdown) @@ -344,6 +363,26 @@ class Reporting(_Fixture): rel = self._bios_file("bios/Sega/Saturn/bad.exe") self.assertIn("### FAIL", validate_pr.validate_file(rel, None, EMPTY_HASHES).to_markdown()) + @unittest.skipUnless((REPO_ROOT / "database.json").is_file(), "database.json is not built") + def test_the_status_line_names_what_is_left_to_review(self): + import shutil + import subprocess + + held = REPO_ROOT / "bios" / "Sony" / "PlayStation Portable" / "ppge_atlas.zim" + if not held.is_file(): + self.skipTest(f"{held} is not here") + shutil.copy(held, self.tmp / "ppge_atlas.zim") + (self.tmp / "mystery.bin").write_bytes(os.urandom(4096)) + proc = subprocess.run( + [sys.executable, str(REPO_ROOT / "scripts" / "validate_pr.py"), "--markdown", + str(self.tmp / "mystery.bin"), str(self.tmp / "ppge_atlas.zim")], + capture_output=True, text=True, timeout=300, check=False, + ) + self.assertEqual(proc.returncode, 0, proc.stderr) + self.assertIn("**Status**: no check failed, to review: 1 already held, " + "1 not recognised", proc.stdout) + self.assertNotIn("all checks passed", proc.stdout) + class PlatformHashLoading(_Fixture): def test_missing_directory_yields_empty_sets(self):