feat(mister): classify arcade hardware systems#1094
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds curated arcade system metadata and MiSTer launchers, then changes random and search commands to resolve primary systems and fallbacks in ordered tiers. ChangesArcade system definitions and MiSTer launchers
Tiered random and search resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformLaunchers
participant arcadeSystemCache
participant ArcadeDatabase
participant MRAFiles
PlatformLaunchers->>arcadeSystemCache: load arcade classifications
arcadeSystemCache->>ArcadeDatabase: read arcade set names
arcadeSystemCache->>MRAFiles: scan and read MRA files
arcadeSystemCache-->>PlatformLaunchers: cache per-system scan results
sequenceDiagram
participant LaunchCommands
participant orderedSystemTiers
participant TieredMediaLookup
participant gamesdb
LaunchCommands->>orderedSystemTiers: resolve primary and fallback systems
orderedSystemTiers-->>TieredMediaLookup: return ordered tiers
TieredMediaLookup->>gamesdb: query current tier
gamesdb-->>TieredMediaLookup: return results or no rows
TieredMediaLookup-->>LaunchCommands: return first matching tier
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/platforms/mister/arcade_systems_test.go`:
- Around line 18-28: Extend the arcadeSetSystems test data with a CPS-3 entry
and assert that its normalized set name maps to systemdefs.SystemCPS3. Keep the
existing CPS-1, CPS-1.5, PGM, and unknown-platform assertions unchanged.
In `@pkg/platforms/mister/arcade_systems.go`:
- Around line 175-247: Add unit-test coverage for addNeoGeoMVSLauncher using a
stub base Scanner: invoke the updated NeoGeo scanner to populate the cache, then
invoke the NeoGeoMVS Scanner and verify it returns the same entries without
calling the stub again. Also verify the expected scanner results and errors
while exercising the shared cache behavior.
- Around line 52-75: Update scanArcadeFiles and its walk callback to check ctx
and stop with ctx.Err() on cancellation. Make load propagate cancellation
without marking the cache loaded or retaining partial results, and have
captureScanner and scanner return the context error instead of success. Add
tests covering cancellation during scanning and ensuring a later scan reloads
rather than reusing partial results.
- Around line 27-39: Add a CPS-3 entry to misterArcadeSystemSpecs using
systemdefs.SystemCPS3 and the platform label “Capcom CPS-3”, so CPS-3 titles are
classified correctly and receive a dedicated launcher.
In `@pkg/zapscript/launch_test.go`:
- Around line 1281-1308: The test
TestSearchMediaBySystemTier_DoesNotCombineFallback currently returns a primary
result and skips the fallback loop. Update or add a focused test where the
primary AmigaCD32 search returns an empty result, the Amiga fallback search is
invoked and returns a result, and the assertions verify that fallback result is
returned without combining tiers. Ensure both mock expectations are configured
and asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1531be65-a456-4662-94e5-f784101fac8b
📒 Files selected for processing (18)
pkg/assets/systems/CPS1.jsonpkg/assets/systems/CPS2.jsonpkg/assets/systems/CPS3.jsonpkg/assets/systems/IremM72.jsonpkg/assets/systems/IremM92.jsonpkg/assets/systems/JalecoMegaSystem1.jsonpkg/assets/systems/NamcoSystem1.jsonpkg/assets/systems/PGM.jsonpkg/assets/systems/SegaSTV.jsonpkg/assets/systems/SegaSystem16.jsonpkg/assets/systems/SegaSystem18.jsonpkg/assets/systems/TaitoF2.jsonpkg/database/systemdefs/systemdefs.gopkg/platforms/mister/arcade_systems.gopkg/platforms/mister/arcade_systems_test.gopkg/platforms/mister/platform.gopkg/zapscript/launch.gopkg/zapscript/launch_test.go
Comment on lines
+175
to
+247
| func addNeoGeoMVSLauncher( | ||
| platform *Platform, | ||
| neoGeo *platforms.Launcher, | ||
| ) (updatedNeoGeo, neoGeoMVS platforms.Launcher) { | ||
| updatedNeoGeo = *neoGeo | ||
| baseScanner := updatedNeoGeo.Scanner | ||
| var mu syncutil.Mutex | ||
| var cached []platforms.ScanResult | ||
| var loaded bool | ||
|
|
||
| updatedNeoGeo.Scanner = func( | ||
| ctx context.Context, | ||
| cfg *config.Instance, | ||
| systemID string, | ||
| results []platforms.ScanResult, | ||
| ) ([]platforms.ScanResult, error) { | ||
| scanned, err := baseScanner(ctx, cfg, systemID, results) | ||
| if err == nil { | ||
| mu.Lock() | ||
| cached = append([]platforms.ScanResult(nil), scanned...) | ||
| loaded = true | ||
| mu.Unlock() | ||
| } | ||
| return scanned, err | ||
| } | ||
|
|
||
| neoGeoMVS = platforms.Launcher{ | ||
| ID: systemdefs.SystemNeoGeoMVS, | ||
| SystemID: systemdefs.SystemNeoGeoMVS, | ||
| Folders: []string{"NEOGEO"}, | ||
| Extensions: []string{".neo", ".zip"}, | ||
| Test: func(_ *config.Instance, path string) bool { | ||
| return filepath.Ext(path) == "" | ||
| }, | ||
| SkipFilesystemScan: true, | ||
| Launch: launchNeoGeoMVS(platform), | ||
| Scanner: func( | ||
| ctx context.Context, | ||
| cfg *config.Instance, | ||
| _ string, | ||
| _ []platforms.ScanResult, | ||
| ) ([]platforms.ScanResult, error) { | ||
| mu.Lock() | ||
| wasLoaded := loaded | ||
| results := append([]platforms.ScanResult(nil), cached...) | ||
| mu.Unlock() | ||
| if wasLoaded { | ||
| return results, nil | ||
| } | ||
| return baseScanner(ctx, cfg, systemdefs.SystemNeoGeo, nil) | ||
| }, | ||
| } | ||
| return updatedNeoGeo, neoGeoMVS | ||
| } | ||
|
|
||
| func launchNeoGeoMVS(platform *Platform) func( | ||
| *config.Instance, string, *platforms.LaunchOptions, | ||
| ) (*os.Process, error) { | ||
| baseLaunch := launch(platform, systemdefs.SystemNeoGeo) | ||
| return func(cfg *config.Instance, path string, opts *platforms.LaunchOptions) (*os.Process, error) { | ||
| launchOpts := platforms.LaunchOptions{} | ||
| if opts != nil { | ||
| launchOpts = *opts | ||
| } | ||
| if launchOpts.SetName == "" { | ||
| launchOpts.SetName = systemdefs.SystemNeoGeoMVS | ||
| } | ||
| if launchOpts.SetNameSameDir == "" { | ||
| launchOpts.SetNameSameDir = "true" | ||
| } | ||
| return baseLaunch(cfg, path, &launchOpts) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add direct NeoGeo MVS scanner coverage.
No supplied test exercises the new split scanner/cache behavior. Add a unit test with a stub base scanner that verifies the NeoGeo scan populates the cache and the MVS scanner returns the same entries without rescanning.
As per coding guidelines, “Write tests for all new code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/platforms/mister/arcade_systems.go` around lines 175 - 247, Add unit-test
coverage for addNeoGeoMVSLauncher using a stub base Scanner: invoke the updated
NeoGeo scanner to populate the cache, then invoke the NeoGeoMVS Scanner and
verify it returns the same entries without calling the stub again. Also verify
the expected scanner results and errors while exercising the shared cache
behavior.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/platforms/mister/arcade_systems_test.go (2)
18-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCPS-3 gap resolved; consider covering remaining granular systems too.
The previously flagged missing CPS-3 case is now covered. Since the PR extends classification to Sega, Taito, Namco, Irem, and Jaleco systems as well, consider adding at least one entry per remaining
misterArcadeSystemSpecsplatform group to this table so a typo in any curated platform name (not just CPS/PGM) would be caught.♻️ Example extension
{Setname: "CPS1GAME", Platform: "Capcom CPS-1"}, {Setname: "cps15game", Platform: "Capcom CPS-1.5"}, {Setname: "CPS3GAME", Platform: "Capcom CPS-3"}, {Setname: "pgmgame", Platform: "IGS PGM"}, + {Setname: "segasystem16game", Platform: "Sega System 16"}, + {Setname: "taitof2game", Platform: "Taito F2"}, + {Setname: "namcosystem1game", Platform: "Namco System 1"}, + {Setname: "iremm72game", Platform: "Irem M72"}, + {Setname: "jalecomegasystem1game", Platform: "Jaleco Mega System 1"}, {Setname: "unknown", Platform: "Unique hardware"},(Adjust platform strings to match the actual curated values in
misterArcadeSystemSpecs.)As per coding guidelines, "Write tests for all new code."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/arcade_systems_test.go` around lines 18 - 34, Extend TestArcadeSetSystemsMapsCuratedPlatforms with representative entries and assertions for each remaining platform group defined in misterArcadeSystemSpecs, including Sega, Taito, Namco, Irem, and Jaleco. Use the exact curated platform strings and expected systemdefs values, and retain the existing CPS/PGM and unknown-platform coverage.Source: Coding guidelines
66-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache sharing is only tested in one call order (NeoGeo before MVS).
Per
addNeoGeoMVSLauncher(arcade_systems.go:204-258), onlyupdatedNeoGeo.Scannerpopulatescached/loaded;neoGeoMVS.Scannerreads the cache but, when not yet loaded, callsbaseScannerdirectly without populating it. This test only exercisesupdated.Scannerfirst thenmvs.Scanner, so it can't catch the reverse order (MVS scanned before NeoGeo), where the cache would never be shared and every MVS scan would redundantly re-invokebaseScanner.Consider adding a subtest that calls
mvs.Scannerfirst, thenupdated.Scanner, to confirm whether the "shared cache" contract holds regardless of call order — or confirm elsewhere that random/search resolution always scans the base NeoGeo system before NeoGeoMVS.🧪 Suggested additional subtest
t.Run("mvs scanned before neogeo does not share cache", func(t *testing.T) { t.Parallel() expected := []platforms.ScanResult{{Path: "mslug.neo", Name: "Metal Slug"}} calls := 0 neoGeo := platforms.Launcher{Scanner: func( context.Context, *config.Instance, string, []platforms.ScanResult, ) ([]platforms.ScanResult, error) { calls++ return expected, nil }} _, mvs := addNeoGeoMVSLauncher(NewPlatform(), &neoGeo) _, err := mvs.Scanner(context.Background(), &config.Instance{}, systemdefs.SystemNeoGeoMVS, nil) require.NoError(t, err) _, err = mvs.Scanner(context.Background(), &config.Instance{}, systemdefs.SystemNeoGeoMVS, nil) require.NoError(t, err) // Documents current behavior: cache is never populated from the MVS side alone. assert.Equal(t, 2, calls) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/arcade_systems_test.go` around lines 66 - 111, Add a reverse-order subtest to TestAddNeoGeoMVSLauncherSharesScannerCache that invokes mvs.Scanner before updated.Scanner, then repeats the MVS scan and verifies the intended cache-sharing behavior and base scanner call count. If the contract requires sharing regardless of order, update addNeoGeoMVSLauncher so the MVS scanner populates the shared cache; otherwise document and assert the current non-sharing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/platforms/mister/arcade_systems_test.go`:
- Around line 18-34: Extend TestArcadeSetSystemsMapsCuratedPlatforms with
representative entries and assertions for each remaining platform group defined
in misterArcadeSystemSpecs, including Sega, Taito, Namco, Irem, and Jaleco. Use
the exact curated platform strings and expected systemdefs values, and retain
the existing CPS/PGM and unknown-platform coverage.
- Around line 66-111: Add a reverse-order subtest to
TestAddNeoGeoMVSLauncherSharesScannerCache that invokes mvs.Scanner before
updated.Scanner, then repeats the MVS scan and verifies the intended
cache-sharing behavior and base scanner call count. If the contract requires
sharing regardless of order, update addNeoGeoMVSLauncher so the MVS scanner
populates the shared cache; otherwise document and assert the current
non-sharing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 644f85b0-265f-4970-a970-37dd5d6881c6
📒 Files selected for processing (3)
pkg/platforms/mister/arcade_systems.gopkg/platforms/mister/arcade_systems_test.gopkg/zapscript/launch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/platforms/mister/arcade_systems.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/platforms/mister/arcade_systems_test.go (1)
246-268: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the base Arcade scanner behavior.
The test verifies launcher configuration but does not prove that the base Arcade launcher still returns all classified and unclassified results. Add a mixed-input scan assertion to protect the “complete arcade indexing” requirement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/arcade_systems_test.go` around lines 246 - 268, Extend TestArcadeSystemLaunchersPreserveArcadeAndAddGranularSystems to invoke the base Arcade launcher’s Scanner with mixed classified and unclassified arcade inputs, then assert that all results are returned. Preserve the existing launcher configuration checks and use the scanner behavior exposed by the arcade launcher rather than testing only metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/platforms/mister/arcade_systems_test.go`:
- Around line 76-84: Create a separate copy of input before calling
cache.captureScanner, then compare unchanged against that pre-call copy instead
of the original slice. Keep the existing captureScanner invocation and error
assertion unchanged.
---
Nitpick comments:
In `@pkg/platforms/mister/arcade_systems_test.go`:
- Around line 246-268: Extend
TestArcadeSystemLaunchersPreserveArcadeAndAddGranularSystems to invoke the base
Arcade launcher’s Scanner with mixed classified and unclassified arcade inputs,
then assert that all results are returned. Preserve the existing launcher
configuration checks and use the scanner behavior exposed by the arcade launcher
rather than testing only metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17608211-5460-48ea-a682-9dbb51ef8412
📒 Files selected for processing (3)
pkg/platforms/mister/arcade_systems.gopkg/platforms/mister/arcade_systems_test.gopkg/zapscript/launch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/platforms/mister/arcade_systems.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1050
Summary by CodeRabbit