From d45afb5dde070de0dffb5eabd94631f77ad618af Mon Sep 17 00:00:00 2001 From: Georges-Antoine Assi Date: Sun, 12 Apr 2026 18:32:15 -0400 Subject: [PATCH] more fixes --- backend/adapters/services/rahasher.py | 21 ++-- backend/handler/filesystem/roms_handler.py | 2 - .../tests/adapters/services/test_rahasher.py | 115 ++++++------------ .../handler/filesystem/test_roms_handler.py | 5 + backend/utils/filesystem.py | 2 +- 5 files changed, 51 insertions(+), 94 deletions(-) diff --git a/backend/adapters/services/rahasher.py b/backend/adapters/services/rahasher.py index 760a5d190..2e1897fd2 100644 --- a/backend/adapters/services/rahasher.py +++ b/backend/adapters/services/rahasher.py @@ -94,6 +94,10 @@ PLATFORM_SLUG_TO_RETROACHIEVEMENTS_ID: dict[UPS, int] = { UPS.WONDERSWAN_COLOR: 53, } +RA_BUFFER_HASH_UNSUPPORTED_IDS: frozenset[int] = frozenset( + PLATFORM_SLUG_TO_RETROACHIEVEMENTS_ID[ups] for ups in RA_BUFFER_HASH_UNSUPPORTED +) + class RAHasherError(Exception): ... @@ -101,29 +105,22 @@ class RAHasherError(Exception): ... class RAHasherService: """Service to calculate RetroAchievements hashes using RAHasher.""" - async def calculate_hash( - self, platform: RAGamesPlatform, file_path: str, file_extension: str - ) -> str: + async def calculate_hash(self, platform: RAGamesPlatform, file_path: str) -> str: # Skip the subprocess entirely when the file is an archive and the # RA platform needs an on-disk disc image. RAHasher would just spawn, # fail with "Unsupported console for buffer hash: {id}", and return # nothing — paying process-spawn overhead per ROM for no result. - normalized_ext = f".{file_extension.lstrip('.').lower()}" - if normalized_ext in COMPRESSED_FILE_EXTENSIONS: - unsupported_ids = { - PLATFORM_SLUG_TO_RETROACHIEVEMENTS_ID[ups] - for ups in RA_BUFFER_HASH_UNSUPPORTED - } - if platform["ra_id"] in unsupported_ids: + if file_path.endswith(tuple(COMPRESSED_FILE_EXTENSIONS)): + if platform["ra_id"] in RA_BUFFER_HASH_UNSUPPORTED_IDS: log.debug( f"Skipping {hl('RAHasher', color=LIGHTMAGENTA)} for archived " - f"{hl(platform['slug'], color=LIGHTMAGENTA)} file {hl(file_path.split('/')[-1])}: " + f"{platform['slug']} file {hl(file_path)}: " f"disc-based platforms don't support buffer hashing" ) return "" log.debug( - f"Executing {hl('RAHasher', color=LIGHTMAGENTA)} for platform: {hl(platform['slug'], color=LIGHTMAGENTA)} - file: {hl(file_path.split('/')[-1])}" + f"Executing {hl('RAHasher', color=LIGHTMAGENTA)} for platform: {hl(platform['slug'], color=LIGHTMAGENTA)} - file: {hl(file_path)}" ) args = (str(platform["ra_id"]), file_path) diff --git a/backend/handler/filesystem/roms_handler.py b/backend/handler/filesystem/roms_handler.py index c71e5e3eb..bc1d847f6 100644 --- a/backend/handler/filesystem/roms_handler.py +++ b/backend/handler/filesystem/roms_handler.py @@ -450,7 +450,6 @@ class FSRomsHandler(FSHandler): rom_ra_h = await RAHasherService().calculate_hash( ra_platform, f"{abs_fs_path}/{rom.fs_name}/*", - rom.fs_extension, ) for f_path, file_name in iter_files( @@ -542,7 +541,6 @@ class FSRomsHandler(FSHandler): rom_ra_h = await RAHasherService().calculate_hash( ra_platform, f"{abs_fs_path}/{rom.fs_name}", - rom.fs_extension, ) file_hash = FileHash( diff --git a/backend/tests/adapters/services/test_rahasher.py b/backend/tests/adapters/services/test_rahasher.py index 2420c83e9..716e4a18a 100644 --- a/backend/tests/adapters/services/test_rahasher.py +++ b/backend/tests/adapters/services/test_rahasher.py @@ -51,7 +51,7 @@ class TestRAHasherService: return RAHasherService() @pytest.mark.asyncio - async def test_calculate_hash_success(self, service): + async def test_calculate_hash_success(self, service: RAHasherService): """Test successful hash calculation.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 0 # RAHasher returns 0 on success @@ -60,7 +60,7 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "a1b2c3d4e5f6789012345678901234ab" @@ -68,20 +68,20 @@ class TestRAHasherService: mock_proc.stdout.read.assert_called_once() @pytest.mark.asyncio - async def test_calculate_hash_rahasher_not_found(self, service): + async def test_calculate_hash_rahasher_not_found(self, service: RAHasherService): """Test when RAHasher executable is not found.""" with patch( "asyncio.create_subprocess_exec", side_effect=FileNotFoundError("RAHasher not found"), ): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" @pytest.mark.asyncio - async def test_calculate_hash_rahasher_failure(self, service): + async def test_calculate_hash_rahasher_failure(self, service: RAHasherService): """Test when RAHasher fails with non-1 return code.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 2 # Error return code @@ -89,7 +89,7 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" @@ -97,7 +97,7 @@ class TestRAHasherService: mock_proc.stderr.read.assert_called_once() @pytest.mark.asyncio - async def test_calculate_hash_no_stdout(self, service): + async def test_calculate_hash_no_stdout(self, service: RAHasherService): """Test when RAHasher has no stdout.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 1 @@ -106,13 +106,13 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" @pytest.mark.asyncio - async def test_calculate_hash_empty_output(self, service): + async def test_calculate_hash_empty_output(self, service: RAHasherService): """Test when RAHasher returns empty output.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 1 @@ -121,13 +121,13 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" @pytest.mark.asyncio - async def test_calculate_hash_invalid_hash_format(self, service): + async def test_calculate_hash_invalid_hash_format(self, service: RAHasherService): """Test when RAHasher returns invalid hash format.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 1 @@ -136,13 +136,13 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" @pytest.mark.asyncio - async def test_calculate_hash_with_extra_output(self, service): + async def test_calculate_hash_with_extra_output(self, service: RAHasherService): """Test when RAHasher returns hash with extra text.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 0 @@ -153,13 +153,13 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "a1b2c3d4e5f6789012345678901234ab" @pytest.mark.asyncio - async def test_calculate_hash_subprocess_args(self, service): + async def test_calculate_hash_subprocess_args(self, service: RAHasherService): """Test that subprocess is called with correct arguments.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 1 @@ -170,7 +170,7 @@ class TestRAHasherService: "asyncio.create_subprocess_exec", return_value=mock_proc ) as mock_subprocess: await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) mock_subprocess.assert_called_once_with( @@ -182,15 +182,15 @@ class TestRAHasherService: ) @pytest.mark.asyncio - async def test_calculate_hash_different_platforms(self, service): + async def test_calculate_hash_different_platforms(self, service: RAHasherService): """Test hash calculation for different platforms.""" test_cases = [ - (3, "/path/to/game.smc", "snes", ".smc"), - (1, "/path/to/game.md", "genesis", ".md"), - (4, "/path/to/game.gb", "gb", ".gb"), + (3, "/path/to/game.smc", "snes"), + (1, "/path/to/game.md", "genesis"), + (4, "/path/to/game.gb", "gb"), ] - for platform_id, file_path, platform_slug, file_extension in test_cases: + for platform_id, file_path, platform_slug in test_cases: mock_proc = AsyncMock() mock_proc.wait.return_value = 0 mock_proc.stdout.read.return_value = b"a1b2c3d4e5f6789012345678901234ab\n" @@ -202,7 +202,6 @@ class TestRAHasherService: result = await service.calculate_hash( {"ra_id": platform_id, "slug": platform_slug}, file_path, - file_extension, ) assert result == "a1b2c3d4e5f6789012345678901234ab" @@ -215,7 +214,7 @@ class TestRAHasherService: ) @pytest.mark.asyncio - async def test_calculate_hash_stderr_handling(self, service): + async def test_calculate_hash_stderr_handling(self, service: RAHasherService): """Test proper handling of stderr when RAHasher fails.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 2 @@ -223,14 +222,14 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" mock_proc.stderr.read.assert_called_once() @pytest.mark.asyncio - async def test_calculate_hash_stderr_none(self, service): + async def test_calculate_hash_stderr_none(self, service: RAHasherService): """Test handling when stderr is None.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 2 @@ -238,7 +237,7 @@ class TestRAHasherService: with patch("asyncio.create_subprocess_exec", return_value=mock_proc): result = await service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, "/path/to/game.nes" ) assert result == "" @@ -264,7 +263,7 @@ class TestRAHasherArchiveSkip: ], ) async def test_skips_subprocess_for_archive_on_disc_platform( - self, service, ups, ext + self, service: RAHasherService, ups, ext ): """No subprocess should be spawned; calculate_hash returns '' immediately.""" assert ups in RA_BUFFER_HASH_UNSUPPORTED @@ -274,33 +273,15 @@ class TestRAHasherArchiveSkip: result = await service.calculate_hash( {"ra_id": platform_id, "slug": "disc-platform"}, f"/path/to/game{ext}", - ext, ) assert result == "" mock_subprocess.assert_not_called() @pytest.mark.asyncio - @pytest.mark.parametrize( - "ext_input", - [".zip", "zip", ".ZIP", "ZIP", ".Zip", "Zip"], - ) - async def test_extension_normalization_skips_subprocess(self, service, ext_input): - """Extension is normalized: leading dot optional, case-insensitive.""" - platform_id = PLATFORM_SLUG_TO_RETROACHIEVEMENTS_ID[UPS.PSP] - - with patch("asyncio.create_subprocess_exec") as mock_subprocess: - result = await service.calculate_hash( - {"ra_id": platform_id, "slug": "psp"}, - "/path/to/game.zip", - ext_input, - ) - - assert result == "" - mock_subprocess.assert_not_called() - - @pytest.mark.asyncio - async def test_does_not_skip_archive_for_cartridge_platform(self, service): + async def test_does_not_skip_archive_for_cartridge_platform( + self, service: RAHasherService + ): """Cartridge platforms (e.g. NES) support buffer hash; don't skip.""" assert UPS.NES not in RA_BUFFER_HASH_UNSUPPORTED nes_id = PLATFORM_SLUG_TO_RETROACHIEVEMENTS_ID[UPS.NES] @@ -314,14 +295,16 @@ class TestRAHasherArchiveSkip: "asyncio.create_subprocess_exec", return_value=mock_proc ) as mock_subprocess: result = await service.calculate_hash( - {"ra_id": nes_id, "slug": "nes"}, "/path/to/game.zip", ".zip" + {"ra_id": nes_id, "slug": "nes"}, "/path/to/game.zip" ) assert result == "a1b2c3d4e5f6789012345678901234ab" mock_subprocess.assert_called_once() @pytest.mark.asyncio - async def test_does_not_skip_raw_iso_for_disc_platform(self, service): + async def test_does_not_skip_raw_iso_for_disc_platform( + self, service: RAHasherService + ): """Raw disc images must still go through RAHasher for disc platforms.""" platform_id = PLATFORM_SLUG_TO_RETROACHIEVEMENTS_ID[UPS.PSP] @@ -334,7 +317,7 @@ class TestRAHasherArchiveSkip: "asyncio.create_subprocess_exec", return_value=mock_proc ) as mock_subprocess: result = await service.calculate_hash( - {"ra_id": platform_id, "slug": "psp"}, "/path/to/game.iso", ".iso" + {"ra_id": platform_id, "slug": "psp"}, "/path/to/game.iso" ) assert result == "a1b2c3d4e5f6789012345678901234ab" @@ -362,32 +345,6 @@ class TestRAHasherError: assert str(e) == message -# Integration-style tests (these would use real RAHasher if available) -class TestRAHasherServiceIntegration: - """Integration tests for RAHasher service (requires RAHasher executable).""" - - @pytest.fixture - def service(self): - """Create a RAHasherService instance for integration testing.""" - return RAHasherService() - - @pytest.mark.asyncio - @pytest.mark.skipif(True, reason="Requires RAHasher executable and test ROM files") - async def test_calculate_hash_real_rahasher(self, service): - """Test with real RAHasher executable (skipped by default).""" - # This test would require: - # 1. RAHasher executable in PATH - # 2. A test ROM file - # 3. Known expected hash for that ROM - - # Example (uncomment and modify for real testing): - # result = await service.calculate_hash( - # {"ra_id": 7, "slug": "nes"}, "/path/to/test.nes", ".nes" - # ) - # assert result == "expected_hash_value" - pass - - # Performance tests class TestRAHasherServicePerformance: """Performance tests for RAHasher service.""" @@ -398,7 +355,7 @@ class TestRAHasherServicePerformance: return RAHasherService() @pytest.mark.asyncio - async def test_concurrent_hash_calculations(self, service): + async def test_concurrent_hash_calculations(self, service: RAHasherService): """Test multiple concurrent hash calculations.""" mock_proc = AsyncMock() mock_proc.wait.return_value = 0 @@ -409,7 +366,7 @@ class TestRAHasherServicePerformance: # Run 5 concurrent hash calculations tasks = [ service.calculate_hash( - {"ra_id": 7, "slug": "nes"}, f"/path/to/game{i}.nes", ".nes" + {"ra_id": 7, "slug": "nes"}, f"/path/to/game{i}.nes" ) for i in range(5) ] diff --git a/backend/tests/handler/filesystem/test_roms_handler.py b/backend/tests/handler/filesystem/test_roms_handler.py index 4694d64bc..f393006b5 100644 --- a/backend/tests/handler/filesystem/test_roms_handler.py +++ b/backend/tests/handler/filesystem/test_roms_handler.py @@ -48,6 +48,7 @@ class TestFSRomsHandler: id=1, fs_name="Paper Mario (USA).z64", fs_path="n64/roms", + fs_extension="z64", platform=platform, full_path="n64/roms/Paper Mario (USA).z64", ) @@ -58,6 +59,7 @@ class TestFSRomsHandler: id=3, fs_name="Sonic (EU) [T]", fs_path="n64/roms", + fs_extension="", platform=platform, full_path="n64/roms/Sonic (EU) [T]", files=[ @@ -80,6 +82,7 @@ class TestFSRomsHandler: id=2, fs_name="Super Mario 64 (J) (Rev A)", fs_path="n64/roms", + fs_extension="", platform=platform, files=[ RomFile( @@ -687,6 +690,7 @@ class TestFSRomsHandler: rom = Rom( id=1, fs_name="test.chd", + fs_extension="chd", fs_path=str(roms_path.relative_to(tmp_path)), platform=platform, ) @@ -742,6 +746,7 @@ class TestFSRomsHandler: rom = Rom( id=1, fs_name="old_format.chd", + fs_extension="chd", fs_path=str(roms_path.relative_to(tmp_path)), platform=platform, ) diff --git a/backend/utils/filesystem.py b/backend/utils/filesystem.py index 7fa4ca002..45c911d78 100644 --- a/backend/utils/filesystem.py +++ b/backend/utils/filesystem.py @@ -7,7 +7,7 @@ from pathlib import Path # (roms_handler for hashing decisions, rahasher for skipping disc-platform # buffer-hash attempts, feeds for PKGi passthrough). COMPRESSED_FILE_EXTENSIONS: frozenset[str] = frozenset( - (".7z", ".bz2", ".gz", ".rar", ".tar", ".zip") + ("7z", "bz2", "gz", "rar", "tar", "zip") )