fix(client): address PR #4 review feedback
Hoshe critical fixes:
- _action_enum_to_wire uses InputMapper.Action constants instead of
fragile integer literals; OPEN_MENU explicitly handled as client-only
- Remove int() coercion on tick/entity_id — use direct assignment since
GDScript int is signed 64-bit (safe for realistic tick values)
- Check encode result before buffering in send_input() — reject empty
bytes instead of corrupting the outbound stream
- Test snapshot now uses Protocol format {tick, entities} instead of
legacy schema; GameState updated to derive player position from
entity data; main.gd and world_renderer.gd updated accordingly
Hoshe warnings:
- 5 negative tests added (truncated bytes, wrong type, missing fields,
empty bytes, encode validation) — 20/20 tests pass
- receive_bytes signal is emitted at consume time in poll_snapshot by
design (documented in code)
Tyre suggestions:
- Remove duplicated root-level fixtures — single source of truth in
client/tests/fixtures/msgpack/
- gen_fixtures.rs writes directly to client/ directory
- Add `make fixtures` target for regeneration
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -135,3 +135,36 @@ func test_gdscript_encode_matches_rust_fixture() -> void:
|
||||
assert_that(from_gd.tick).is_equal(from_rust.tick)
|
||||
assert_that(from_gd.action.variant).is_equal(from_rust.action.variant)
|
||||
assert_that(from_gd.action.data).is_equal(from_rust.action.data)
|
||||
|
||||
|
||||
# -- Negative tests: malformed/truncated input -----------------------------------
|
||||
|
||||
func test_decode_snapshot_truncated_bytes() -> void:
|
||||
var truncated := PackedByteArray([0x82, 0xa4]) # Incomplete msgpack map
|
||||
var result = Protocol.decode_snapshot(truncated)
|
||||
assert_that(result).is_null()
|
||||
|
||||
|
||||
func test_decode_snapshot_wrong_type() -> void:
|
||||
# Encode an array instead of a map — should fail validation
|
||||
var encoded = Messagepack.encode([1, 2, 3])
|
||||
var result = Protocol.decode_snapshot(encoded.value)
|
||||
assert_that(result).is_null()
|
||||
|
||||
|
||||
func test_decode_snapshot_missing_fields() -> void:
|
||||
# Map with wrong keys
|
||||
var encoded = Messagepack.encode({"foo": "bar"})
|
||||
var result = Protocol.decode_snapshot(encoded.value)
|
||||
assert_that(result).is_null()
|
||||
|
||||
|
||||
func test_decode_player_input_empty_bytes() -> void:
|
||||
var result = Protocol.decode_player_input(PackedByteArray())
|
||||
assert_that(result).is_null()
|
||||
|
||||
|
||||
func test_encode_returns_empty_on_failure() -> void:
|
||||
# Verify that a valid encode produces non-empty bytes
|
||||
var bytes = Protocol.encode_player_input(1, "MoveNorth")
|
||||
assert_that(bytes.size()).is_greater(0)
|
||||
|
||||
@@ -1,30 +1,24 @@
|
||||
## D-030 Layer 1: Fixture-based tests for snapshot parsing
|
||||
## Validates that GameState correctly parses ObserverSnapshot data
|
||||
## Validates that GameState correctly parses Protocol-format ObserverSnapshot data
|
||||
class_name TestSnapshotParsing
|
||||
extends GdUnitTestSuite
|
||||
|
||||
# Valid snapshot fixture
|
||||
# Valid snapshot in Protocol format (matches Protocol.decode_snapshot output)
|
||||
var _valid_snapshot: Dictionary = {
|
||||
"tick": 1,
|
||||
"player": {
|
||||
"position": [10, 15],
|
||||
"health": 85
|
||||
},
|
||||
"entities": [
|
||||
{"id": 1, "type": "npc", "position": [12, 8], "name": "Test NPC"},
|
||||
{"id": 2, "type": "npc", "position": [5, 20], "name": "Second NPC"},
|
||||
{"entity_id": 1, "x": 10.0, "y": 15.0, "z": 0, "kind": {"variant": "Npc", "data": null}},
|
||||
{"entity_id": 2, "x": 5.0, "y": 20.0, "z": 0, "kind": {"variant": "Npc", "data": null}},
|
||||
],
|
||||
"fog": {"radius": 8},
|
||||
"hud": {"perception_mode": "baseline", "time": "14:30"}
|
||||
}
|
||||
|
||||
func test_apply_valid_snapshot() -> void:
|
||||
GameState.player_entity_id = 1
|
||||
GameState.apply_snapshot(_valid_snapshot)
|
||||
|
||||
assert_that(GameState.current_tick).is_equal(1)
|
||||
assert_that(GameState.player_position).is_equal(Vector2(10, 15))
|
||||
assert_that(GameState.visible_entities.size()).is_equal(2)
|
||||
assert_that(GameState.fog_state).is_equal({"radius": 8})
|
||||
assert_that(GameState.hud_data).is_equal({"perception_mode": "baseline", "time": "14:30"})
|
||||
|
||||
func test_empty_snapshot_no_crash() -> void:
|
||||
# Reset state
|
||||
@@ -37,26 +31,25 @@ func test_empty_snapshot_no_crash() -> void:
|
||||
assert_that(GameState.player_position).is_equal(Vector2.ZERO)
|
||||
assert_that(GameState.visible_entities.size()).is_equal(0)
|
||||
|
||||
func test_malformed_position_no_crash() -> void:
|
||||
var bad_snapshot: Dictionary = {
|
||||
"player": {"position": "not_an_array"},
|
||||
}
|
||||
# Reset
|
||||
GameState.player_position = Vector2.ZERO
|
||||
func test_no_player_entity_position_unchanged() -> void:
|
||||
GameState.player_entity_id = 999 # No entity with this ID
|
||||
GameState.player_position = Vector2(5, 5)
|
||||
|
||||
# Should not crash — logs warning instead
|
||||
GameState.apply_snapshot(bad_snapshot)
|
||||
assert_that(GameState.player_position).is_equal(Vector2.ZERO)
|
||||
GameState.apply_snapshot(_valid_snapshot)
|
||||
|
||||
# Position stays at previous value since no matching entity
|
||||
assert_that(GameState.player_position).is_equal(Vector2(5, 5))
|
||||
|
||||
func test_missing_fields_partial_update() -> void:
|
||||
# First apply valid snapshot
|
||||
GameState.player_entity_id = 1
|
||||
GameState.apply_snapshot(_valid_snapshot)
|
||||
assert_that(GameState.player_position).is_equal(Vector2(10, 15))
|
||||
|
||||
# Apply snapshot with only HUD data — player position unchanged
|
||||
GameState.apply_snapshot({"hud": {"perception_mode": "thermal", "time": "22:00"}})
|
||||
# Apply snapshot with no entities — player position unchanged (no matching entity)
|
||||
GameState.apply_snapshot({"tick": 2})
|
||||
assert_that(GameState.player_position).is_equal(Vector2(10, 15))
|
||||
assert_that(GameState.hud_data.perception_mode).is_equal("thermal")
|
||||
assert_that(GameState.current_tick).is_equal(2)
|
||||
|
||||
func test_sim_bridge_test_snapshot_deterministic() -> void:
|
||||
SimBridge._test_tick = 0
|
||||
@@ -65,8 +58,9 @@ func test_sim_bridge_test_snapshot_deterministic() -> void:
|
||||
|
||||
assert_that(snap1.tick).is_equal(1)
|
||||
assert_that(snap2.tick).is_equal(2)
|
||||
# Snapshot structure is stable
|
||||
assert_that(snap1.has("player")).is_true()
|
||||
# Snapshot matches Protocol format
|
||||
assert_that(snap1.has("tick")).is_true()
|
||||
assert_that(snap1.has("entities")).is_true()
|
||||
assert_that(snap1.has("fog")).is_true()
|
||||
assert_that(snap1.has("hud")).is_true()
|
||||
assert_that(snap1.entities.size()).is_greater(0)
|
||||
assert_that(snap1.entities[0].has("entity_id")).is_true()
|
||||
assert_that(snap1.entities[0].has("kind")).is_true()
|
||||
|
||||
Reference in New Issue
Block a user