Pathfinding - 1 - Tests #2805
Pathfinding - 1 - Tests #2805
Conversation
WalkthroughThis change adds a new "world" test map asset with configuration files (info.json and manifest.json), registers it in the maps registry, and introduces comprehensive test suites for pathfinding components (MiniAStar and PathFinder) with supporting test utilities for map creation from string representations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Fix all issues with AI agents
In @map-generator/main.go:
- Line 71: Remove the duplicate map registry entry for Name: "world" that has
IsTest: true; locate the maps slice/registry where entries like {Name: "world",
IsTest: ...} are defined and delete the test duplicate (the one with IsTest:
true) so the world map is registered only once, or alternatively remove the
non-test entry if you intend the test variant to be canonical—ensure only a
single entry for Name "world" remains.
🧹 Nitpick comments (2)
tests/core/pathfinding/utils.ts (1)
112-112: GameMapType.Asia is hardcoded regardless of actual map used.The
gameConfigusesGameMapType.Asiaeven when this utility is used to create world maps or other test maps. This is just metadata and doesn't affect pathfinding tests since the actual terrain comes from the constructedGameMapImpl, but it could be confusing when debugging.tests/core/pathfinding/PathFinder.test.ts (1)
195-259: Consider extracting coordinate constants for reusability.The world map tests are well structured and the coordinate comment is helpful. However, the coordinates are duplicated across tests (e.g., Spain appears in tests on lines 204 and 241).
🔎 Optional refactor to reduce duplication
describe("PathFinder world map tests", () => { // Ocean shoreline coordinates: - // Spain east coast: [926, 283], France south coast: [950, 257] - // Poland north coast: [1033, 175], Miami: [488, 355], Rio: [680, 658] + const COORDS = { + SPAIN_EAST: { x: 926, y: 283 }, + FRANCE_SOUTH: { x: 950, y: 257 }, + POLAND_NORTH: { x: 1033, y: 175 }, + MIAMI: { x: 488, y: 355 }, + RIO: { x: 680, y: 658 }, + }; test("finds path Spain to France (Mediterranean)", async () => { const game = await setup("world"); const pathFinder = createPathFinder(game); - const src = game.ref(926, 283); // Spain east coast - const dst = game.ref(950, 257); // France south coast + const src = game.ref(COORDS.SPAIN_EAST.x, COORDS.SPAIN_EAST.y); + const dst = game.ref(COORDS.FRANCE_SOUTH.x, COORDS.FRANCE_SOUTH.y); const result = navigateTo(pathFinder, src, dst, 500); expect(result.reached).toBe(true); expect(result.steps).toBeGreaterThan(0); }); // ... apply to other tests
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
map-generator/assets/test_maps/world/image.pngis excluded by!**/*.pngtests/testdata/maps/world/map.binis excluded by!**/*.bintests/testdata/maps/world/map16x.binis excluded by!**/*.bintests/testdata/maps/world/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (7)
map-generator/assets/test_maps/world/info.jsonmap-generator/main.gotests/core/pathfinding/MiniAStar.test.tstests/core/pathfinding/PathFinder.test.tstests/core/pathfinding/utils.tstests/testdata/maps/world/manifest.jsontests/testdata/maps/world/thumbnail.webp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-19T11:00:55.422Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1864
File: resources/maps/arabianpeninsula/manifest.json:13-170
Timestamp: 2025-08-19T11:00:55.422Z
Learning: In OpenFrontIO, nation names in map manifests are displayed directly in the UI without translation. They do not need to be added to resources/lang/en.json or processed through translateText(). This is the established pattern across all existing maps including Europe, World, Asia, Africa, and others.
Applied to files:
map-generator/assets/test_maps/world/info.jsontests/testdata/maps/world/manifest.json
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.
Applied to files:
map-generator/assets/test_maps/world/info.json
📚 Learning: 2025-08-10T14:37:35.777Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 1770
File: resources/maps/world/manifest.json:279-355
Timestamp: 2025-08-10T14:37:35.777Z
Learning: The OpenFrontIO World Map includes both real-world nations and fictional/creative entities like "Polar Bears" as playable nations. These special nations may use non-two-letter flag codes (e.g., "polar_bears" instead of a two-letter code), and this is intentional design. The corresponding flag assets are stored in resources/flags/ with matching filenames.
Applied to files:
map-generator/assets/test_maps/world/info.jsontests/testdata/maps/world/manifest.json
📚 Learning: 2025-05-22T23:43:54.789Z
Learnt from: jrouillard
Repo: openfrontio/OpenFrontIO PR: 848
File: src/core/execution/TradeShipExecution.ts:110-134
Timestamp: 2025-05-22T23:43:54.789Z
Learning: In the OpenFrontIO project, MiniAStar.compute() only returns PathFindResultType.Pending, PathFindResultType.Completed, or PathFindResultType.PathNotFound - it doesn't return PathFindResultType.NextTile. When PathFindResultType.Completed is returned, the full path can be retrieved via reconstructPath().
Applied to files:
tests/core/pathfinding/MiniAStar.test.ts
🧬 Code graph analysis (1)
tests/core/pathfinding/MiniAStar.test.ts (3)
src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/pathfinding/MiniAStar.ts (1)
MiniAStar(41-101)tests/core/pathfinding/utils.ts (1)
mapFromString(30-134)
🔇 Additional comments (3)
tests/core/pathfinding/PathFinder.test.ts (3)
58-102: LGTM! Basic behavior tests are well structured.The test suite covers the essential state machine behaviors: first call, completion, immediate completion, and path continuation. The type guard on line 86 is good defensive programming.
104-145: LGTM! Destination change tests cover dynamic scenarios.These tests properly validate that the pathfinder handles destination changes during navigation, both for sequential goals and mid-path recomputation.
179-192: LGTM! Good practice to document known limitations.Using
test.failsto track a known bug is excellent. When the upscaling feature is implemented, remember to remove the.failsmodifier so this test validates the fix.
| {Name: "ocean_and_land", IsTest: true}, | ||
| {Name: "plains", IsTest: true}, | ||
| {Name: "giantworldmap", IsTest: true}, | ||
| {Name: "world", IsTest: true}, |
There was a problem hiding this comment.
Remove duplicate "world" map entry.
The maps registry already has a "world" entry at line 62. Adding another "world" entry with IsTest: true at line 71 creates a duplicate that will cause the world map to be processed twice when no --maps flag is provided. While they write to different output directories (test vs production), this wastes resources during map generation.
🔎 Proposed fix
If you need a test version of the world map, either:
- Remove the non-test entry at line 62 and keep only the test entry at line 71, or
- Remove the test entry at line 71 if the production world map is sufficient for testing
- {Name: "world", IsTest: true},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {Name: "world", IsTest: true}, |
🤖 Prompt for AI Agents
In @map-generator/main.go at line 71, Remove the duplicate map registry entry
for Name: "world" that has IsTest: true; locate the maps slice/registry where
entries like {Name: "world", IsTest: ...} are defined and delete the test
duplicate (the one with IsTest: true) so the world map is registered only once,
or alternatively remove the non-test entry if you intend the test variant to be
canonical—ensure only a single entry for Name "world" remains.
| function navigateTo( | ||
| pathFinder: PathFinder, | ||
| from: TileRef, | ||
| to: TileRef, | ||
| maxIter = 100, | ||
| ): { | ||
| reached: boolean; | ||
| notFound: boolean; | ||
| pos: TileRef; | ||
| steps: number; | ||
| path: TileRef[]; | ||
| } { | ||
| const status = { | ||
| reached: false, | ||
| notFound: false, | ||
| pos: from, | ||
| steps: 0, | ||
| path: [] as TileRef[], | ||
| }; | ||
|
|
||
| for (let i = 0; i < maxIter; i++) { | ||
| const result = pathFinder.nextTile(status.pos, to); | ||
|
|
||
| if (result.type === PathFindResultType.NextTile) { | ||
| status.path.push(result.node); | ||
| status.pos = result.node; | ||
| status.steps++; | ||
| } else if (result.type === PathFindResultType.Completed) { | ||
| status.path.push(result.node); | ||
| status.reached = true; | ||
| return status; | ||
| } else if (result.type === PathFindResultType.PathNotFound) { | ||
| status.notFound = true; | ||
| return status; | ||
| } | ||
| } | ||
|
|
||
| return status; | ||
| } |
There was a problem hiding this comment.
Clarify the third parameter usage and timeout state.
Two observations:
-
Parameter inconsistency: Line 37 calls
pathFinder.nextTile(status.pos, to)with 2 arguments, but line 83 shows the method accepts a third parameter. If this parameter is meaningful, it should be passed here. -
Ambiguous timeout state: When
maxIteris reached, bothreachedandnotFoundremainfalse. Consider adding a third state liketimeout: booleanto distinguish timeouts from in-progress navigation.
🔎 Suggested improvements
function navigateTo(
pathFinder: PathFinder,
from: TileRef,
to: TileRef,
maxIter = 100,
): {
reached: boolean;
notFound: boolean;
+ timeout: boolean;
pos: TileRef;
steps: number;
path: TileRef[];
} {
const status = {
reached: false,
notFound: false,
+ timeout: false,
pos: from,
steps: 0,
path: [] as TileRef[],
};
for (let i = 0; i < maxIter; i++) {
const result = pathFinder.nextTile(status.pos, to);
if (result.type === PathFindResultType.NextTile) {
status.path.push(result.node);
status.pos = result.node;
status.steps++;
} else if (result.type === PathFindResultType.Completed) {
status.path.push(result.node);
status.reached = true;
return status;
} else if (result.type === PathFindResultType.PathNotFound) {
status.notFound = true;
return status;
}
}
+ status.timeout = true;
return status;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function navigateTo( | |
| pathFinder: PathFinder, | |
| from: TileRef, | |
| to: TileRef, | |
| maxIter = 100, | |
| ): { | |
| reached: boolean; | |
| notFound: boolean; | |
| pos: TileRef; | |
| steps: number; | |
| path: TileRef[]; | |
| } { | |
| const status = { | |
| reached: false, | |
| notFound: false, | |
| pos: from, | |
| steps: 0, | |
| path: [] as TileRef[], | |
| }; | |
| for (let i = 0; i < maxIter; i++) { | |
| const result = pathFinder.nextTile(status.pos, to); | |
| if (result.type === PathFindResultType.NextTile) { | |
| status.path.push(result.node); | |
| status.pos = result.node; | |
| status.steps++; | |
| } else if (result.type === PathFindResultType.Completed) { | |
| status.path.push(result.node); | |
| status.reached = true; | |
| return status; | |
| } else if (result.type === PathFindResultType.PathNotFound) { | |
| status.notFound = true; | |
| return status; | |
| } | |
| } | |
| return status; | |
| } | |
| function navigateTo( | |
| pathFinder: PathFinder, | |
| from: TileRef, | |
| to: TileRef, | |
| maxIter = 100, | |
| ): { | |
| reached: boolean; | |
| notFound: boolean; | |
| timeout: boolean; | |
| pos: TileRef; | |
| steps: number; | |
| path: TileRef[]; | |
| } { | |
| const status = { | |
| reached: false, | |
| notFound: false, | |
| timeout: false, | |
| pos: from, | |
| steps: 0, | |
| path: [] as TileRef[], | |
| }; | |
| for (let i = 0; i < maxIter; i++) { | |
| const result = pathFinder.nextTile(status.pos, to); | |
| if (result.type === PathFindResultType.NextTile) { | |
| status.path.push(result.node); | |
| status.pos = result.node; | |
| status.steps++; | |
| } else if (result.type === PathFindResultType.Completed) { | |
| status.path.push(result.node); | |
| status.reached = true; | |
| return status; | |
| } else if (result.type === PathFindResultType.PathNotFound) { | |
| status.notFound = true; | |
| return status; | |
| } | |
| } | |
| status.timeout = true; | |
| return status; | |
| } |
1 participant
wasn't supposed to land in main repo