fix(ci): address PR #31 review — harden cross-encoder fixture pipeline
- Fail on encode errors instead of silently writing empty .msgpack files - Fail test on missing/empty fixture dir instead of silent skip - Add all missing action variants (MoveSouth, MoveEast, MoveWest, Unpause, ToggleStanceDown, WalkAway) to GDScript fixture generator - Add GDScript fixture staleness check to make pre-pr - Validate repo root detection before writing outside client/ - Add file.flush() before close in headless mode - Document fixture failure recovery in DEVOPS.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -158,13 +158,13 @@ pre-pr-validate: validate-content check-fact-ids
|
|||||||
@echo "--- Content validation: PASS ---"
|
@echo "--- Content validation: PASS ---"
|
||||||
|
|
||||||
pre-pr-fixtures:
|
pre-pr-fixtures:
|
||||||
@echo "Checking fixture staleness..."
|
@echo "Checking Rust->GDScript fixture staleness..."
|
||||||
@cd server && cargo test --test gen_fixtures -- --ignored
|
@cd server && cargo test --test gen_fixtures -- --ignored
|
||||||
@if git diff --quiet client/tests/fixtures/; then \
|
@if git diff --quiet client/tests/fixtures/; then \
|
||||||
echo "--- Fixtures: UP TO DATE ---"; \
|
echo "--- Rust fixtures: UP TO DATE ---"; \
|
||||||
else \
|
else \
|
||||||
echo ""; \
|
echo ""; \
|
||||||
echo "--- FIXTURES STALE ---"; \
|
echo "--- RUST FIXTURES STALE ---"; \
|
||||||
echo " Protocol changed but fixtures not regenerated."; \
|
echo " Protocol changed but fixtures not regenerated."; \
|
||||||
echo " Stale fixtures make all client tests FALSE POSITIVES."; \
|
echo " Stale fixtures make all client tests FALSE POSITIVES."; \
|
||||||
echo ""; \
|
echo ""; \
|
||||||
@@ -174,6 +174,22 @@ pre-pr-fixtures:
|
|||||||
echo " Fix: commit the updated fixtures with your protocol change."; \
|
echo " Fix: commit the updated fixtures with your protocol change."; \
|
||||||
exit 1; \
|
exit 1; \
|
||||||
fi
|
fi
|
||||||
|
@echo "Checking GDScript->Rust fixture staleness..."
|
||||||
|
@$(MAKE) fixtures-client
|
||||||
|
@if git diff --quiet server/tests/fixtures/gdscript/; then \
|
||||||
|
echo "--- GDScript fixtures: UP TO DATE ---"; \
|
||||||
|
else \
|
||||||
|
echo ""; \
|
||||||
|
echo "--- GDSCRIPT FIXTURES STALE ---"; \
|
||||||
|
echo " Protocol changed but GDScript fixtures not regenerated."; \
|
||||||
|
echo " Stale fixtures make cross-encoder tests FALSE POSITIVES."; \
|
||||||
|
echo ""; \
|
||||||
|
echo " Changed files:"; \
|
||||||
|
git diff --stat server/tests/fixtures/gdscript/; \
|
||||||
|
echo ""; \
|
||||||
|
echo " Fix: commit the updated fixtures with your protocol change."; \
|
||||||
|
exit 1; \
|
||||||
|
fi
|
||||||
|
|
||||||
# Branch-specific variants (faster, scope-appropriate)
|
# Branch-specific variants (faster, scope-appropriate)
|
||||||
|
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ extends SceneTree
|
|||||||
|
|
||||||
var _Msgpack: GDScript
|
var _Msgpack: GDScript
|
||||||
var _count := 0
|
var _count := 0
|
||||||
|
var _errors := 0
|
||||||
var _output_dir: String
|
var _output_dir: String
|
||||||
|
|
||||||
|
|
||||||
@@ -22,27 +23,48 @@ func _init():
|
|||||||
func _run():
|
func _run():
|
||||||
_Msgpack = load("res://addons/messagepack/messagepack.gd")
|
_Msgpack = load("res://addons/messagepack/messagepack.gd")
|
||||||
|
|
||||||
|
# Resolve repo root from Godot project root (client/).
|
||||||
|
# Assumes client/ is one level below repo root — validated below.
|
||||||
var project_root := ProjectSettings.globalize_path("res://")
|
var project_root := ProjectSettings.globalize_path("res://")
|
||||||
var repo_root := project_root.rstrip("/").get_base_dir()
|
var repo_root := project_root.rstrip("/").get_base_dir()
|
||||||
_output_dir = repo_root.path_join("server/tests/fixtures/gdscript")
|
_output_dir = repo_root.path_join("server/tests/fixtures/gdscript")
|
||||||
|
|
||||||
|
if not DirAccess.dir_exists_absolute(repo_root.path_join("server")):
|
||||||
|
push_error("Repo root detection failed: %s/server/ does not exist" % repo_root)
|
||||||
|
quit(1)
|
||||||
|
return
|
||||||
|
|
||||||
DirAccess.make_dir_recursive_absolute(_output_dir)
|
DirAccess.make_dir_recursive_absolute(_output_dir)
|
||||||
|
|
||||||
_generate_inputs()
|
_generate_inputs()
|
||||||
_generate_boundary_inputs()
|
_generate_boundary_inputs()
|
||||||
_generate_batch()
|
_generate_batch()
|
||||||
|
|
||||||
|
if _errors > 0:
|
||||||
|
push_error("FAILED: %d encode errors encountered" % _errors)
|
||||||
|
quit(1)
|
||||||
|
return
|
||||||
|
if _count == 0:
|
||||||
|
push_error("FAILED: no fixtures generated")
|
||||||
|
quit(1)
|
||||||
|
return
|
||||||
print("Generated %d GDScript fixtures at %s" % [_count, _output_dir])
|
print("Generated %d GDScript fixtures at %s" % [_count, _output_dir])
|
||||||
quit()
|
quit()
|
||||||
|
|
||||||
|
|
||||||
func _write_fixture(name: String, bytes: PackedByteArray) -> void:
|
func _write_fixture(name: String, bytes: PackedByteArray) -> void:
|
||||||
|
if bytes.is_empty():
|
||||||
|
push_error("Encode produced empty bytes for fixture: %s" % name)
|
||||||
|
_errors += 1
|
||||||
|
return
|
||||||
var path := _output_dir.path_join(name + ".msgpack")
|
var path := _output_dir.path_join(name + ".msgpack")
|
||||||
var file := FileAccess.open(path, FileAccess.WRITE)
|
var file := FileAccess.open(path, FileAccess.WRITE)
|
||||||
if file == null:
|
if file == null:
|
||||||
push_error("Failed to write fixture: %s (error: %d)" % [path, FileAccess.get_open_error()])
|
push_error("Failed to write fixture: %s (error: %d)" % [path, FileAccess.get_open_error()])
|
||||||
|
_errors += 1
|
||||||
return
|
return
|
||||||
file.store_buffer(bytes)
|
file.store_buffer(bytes)
|
||||||
|
file.flush()
|
||||||
file.close()
|
file.close()
|
||||||
print(" Wrote %s (%d bytes)" % [name, bytes.size()])
|
print(" Wrote %s (%d bytes)" % [name, bytes.size()])
|
||||||
_count += 1
|
_count += 1
|
||||||
@@ -90,10 +112,11 @@ func _encode_inputs(inputs: Array) -> PackedByteArray:
|
|||||||
|
|
||||||
|
|
||||||
func _generate_inputs() -> void:
|
func _generate_inputs() -> void:
|
||||||
# Unit variants: movement directions (tick=100)
|
# Unit variants: all 8 movement directions (tick=100)
|
||||||
_write_fixture("input_move_north",
|
for dir_name in [
|
||||||
_encode_input(100, "MoveNorth"))
|
"MoveNorth", "MoveSouth", "MoveEast", "MoveWest",
|
||||||
for dir_name in ["MoveNortheast", "MoveSoutheast", "MoveSouthwest", "MoveNorthwest"]:
|
"MoveNortheast", "MoveSoutheast", "MoveSouthwest", "MoveNorthwest",
|
||||||
|
]:
|
||||||
_write_fixture("input_%s" % dir_name.to_snake_case(),
|
_write_fixture("input_%s" % dir_name.to_snake_case(),
|
||||||
_encode_input(100, dir_name))
|
_encode_input(100, dir_name))
|
||||||
|
|
||||||
@@ -108,8 +131,14 @@ func _generate_inputs() -> void:
|
|||||||
# Other unit variants
|
# Other unit variants
|
||||||
_write_fixture("input_pause",
|
_write_fixture("input_pause",
|
||||||
_encode_input(100, "Pause"))
|
_encode_input(100, "Pause"))
|
||||||
|
_write_fixture("input_unpause",
|
||||||
|
_encode_input(100, "Unpause"))
|
||||||
_write_fixture("input_toggle_stance_up",
|
_write_fixture("input_toggle_stance_up",
|
||||||
_encode_input(100, "ToggleStanceUp"))
|
_encode_input(100, "ToggleStanceUp"))
|
||||||
|
_write_fixture("input_toggle_stance_down",
|
||||||
|
_encode_input(100, "ToggleStanceDown"))
|
||||||
|
_write_fixture("input_walk_away",
|
||||||
|
_encode_input(100, "WalkAway"))
|
||||||
|
|
||||||
|
|
||||||
func _generate_boundary_inputs() -> void:
|
func _generate_boundary_inputs() -> void:
|
||||||
|
|||||||
@@ -82,6 +82,12 @@ The bidirectional protocol is validated by two sets of committed fixtures:
|
|||||||
|
|
||||||
Regenerate both after any protocol change. Commit the updated fixtures alongside the code change.
|
Regenerate both after any protocol change. Commit the updated fixtures alongside the code change.
|
||||||
|
|
||||||
|
**Troubleshooting fixture failures:**
|
||||||
|
|
||||||
|
- **`make fixtures-client` fails with encode errors:** Check that `client/addons/messagepack/messagepack.gd` is up to date. The script exits non-zero on any encode failure.
|
||||||
|
- **`gdscript_generated_fixtures_deserialize` fails:** Fixtures in `server/tests/fixtures/gdscript/` are stale or corrupted. Re-run `make fixtures-client` and commit the updated files.
|
||||||
|
- **Fixture staleness in `make pre-pr`:** Protocol changed but fixtures were not regenerated. Run `make fixtures && make fixtures-client`, then commit both `client/tests/fixtures/` and `server/tests/fixtures/gdscript/`.
|
||||||
|
|
||||||
### Lint
|
### Lint
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
|
|||||||
@@ -1013,15 +1013,14 @@ fn malformed_input_in_batch_rejects_entire_batch() {
|
|||||||
/// (runs client/tests/gen_client_fixtures.gd via Godot headless)
|
/// (runs client/tests/gen_client_fixtures.gd via Godot headless)
|
||||||
#[test]
|
#[test]
|
||||||
fn gdscript_generated_fixtures_deserialize() {
|
fn gdscript_generated_fixtures_deserialize() {
|
||||||
|
// CWD is server/ when cargo test runs (Cargo sets it to the package root)
|
||||||
let fixture_dir = std::path::Path::new("tests/fixtures/gdscript");
|
let fixture_dir = std::path::Path::new("tests/fixtures/gdscript");
|
||||||
|
|
||||||
if !fixture_dir.exists() {
|
assert!(
|
||||||
eprintln!(
|
fixture_dir.exists(),
|
||||||
"SKIP: GDScript fixtures not found at {}. Run `make fixtures-client` to generate.",
|
"GDScript fixture directory not found at {}. Run `make fixtures-client` to generate.",
|
||||||
fixture_dir.display()
|
fixture_dir.display()
|
||||||
);
|
);
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
let mut count = 0;
|
let mut count = 0;
|
||||||
for entry in fs::read_dir(&fixture_dir).expect("read gdscript fixture dir") {
|
for entry in fs::read_dir(&fixture_dir).expect("read gdscript fixture dir") {
|
||||||
@@ -1088,13 +1087,11 @@ fn gdscript_generated_fixtures_deserialize() {
|
|||||||
count += 1;
|
count += 1;
|
||||||
}
|
}
|
||||||
|
|
||||||
if count == 0 {
|
assert!(
|
||||||
eprintln!(
|
count > 0,
|
||||||
"SKIP: no .msgpack files found in {}. Run `make fixtures-client` to generate.",
|
"No .msgpack files found in {}. Run `make fixtures-client` to generate.",
|
||||||
fixture_dir.display()
|
fixture_dir.display()
|
||||||
);
|
);
|
||||||
return;
|
|
||||||
}
|
|
||||||
eprintln!("Verified {} GDScript-generated fixtures", count);
|
eprintln!("Verified {} GDScript-generated fixtures", count);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user