From b2ef73256a58c5c36426a77b078ff8e08e65f0d4 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Mon, 27 Jul 2026 00:12:26 +0200 Subject: [PATCH] fix(config): run-godot reported a suite that never ran as a pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by walking into it. test_step_canvas_annotation_layer.gd had a parse error from an earlier edit in this session, so gdUnit4 could not load it and ran the other suites instead. The harness printed 3610 passed / 0 failed and exit 0. Fifty tests had not run for hours and nothing said so — the full suite reports 3660 with the file repaired, and that difference was invisible. Two states are now hard harness failures rather than test results: load_error — a suite failed to LOAD. Any pass count excludes it, so a green number is a lie. The hint names the offending file. no_tests — zero tests executed. A run that executes nothing can never be a pass; previously a mistyped --filter printed "Tests passed". Both add a "harness_error" field to the summary JSON and exit 2. The exit code cannot inherit gdUnit4's, which returns 0 in both states — that is precisely why they were invisible. Verified by injecting each failure rather than by reasoning about it. The load_error guard was checked in the case that actually matters: one broken file among many, where total stays large and failed stays zero. That run now reports 3610/0 WITH harness_error and exits 2, where before it was indistinguishable from success. Also repairs the file itself: a missed set_frame() argument (the parse error), and a cell-placement test still asserting pre-inversion spacing. Rewritten to assert the invariant that survives the extent inversion, the viewport aspect ratio and panning — half the SHORT axis is half a rung cell — instead of a literal. Two things it deliberately does not assert, both of which the previous version got wrong: "the corner is half a district away" holds only on a square canvas, and the canvas is one district WIDE without sitting ON a district. It is a free-floating window centred wherever the player panned; zoom is stepped, pan is continuous. A rung names a scale, not a cell you are inside. A second test pins that with a deliberately unaligned world centre, so a future change that snaps the canvas to the rung lattice — making pan step instead of slide — fails here. Pair session with Jeroen, 2026-07-27. Co-Authored-By: Claude --- .pql/changelog/ticket_deps/2026-07.sql | 9 +++ .../test_step_canvas_annotation_layer.gd | 65 ++++++++++++++++--- tests/run-godot | 46 +++++++++++++ 3 files changed, 112 insertions(+), 8 deletions(-) diff --git a/.pql/changelog/ticket_deps/2026-07.sql b/.pql/changelog/ticket_deps/2026-07.sql index 4e752fd98..58abc7f06 100644 --- a/.pql/changelog/ticket_deps/2026-07.sql +++ b/.pql/changelog/ticket_deps/2026-07.sql @@ -27,3 +27,12 @@ INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updat INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FS5XXY5GNCQEXYAYQEQD0CHM', '06FS5XZF4A978WS1J66Q2JBK8C', '2026-07-24 07:12:42.876', '2026-07-24 07:12:42.876', NULL, '6215bc0bc9966f904af9212895fb2a93', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FS5XT949C8D2JZRWDX2CRZN8', '06FRXKFEJMDEPQ0PZ5Q862DHKG', '2026-07-24 07:12:43.096', '2026-07-24 07:12:43.096', NULL, '1813bd9c252aa447c9d7ad88c604d6cd', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FBPTXPE8FR7TGEM7X6QVPV4M', '06FBPTXPPHC9FZ4T0GNKR615PR', '2026-07-25 18:11:40.311', '2026-07-25 18:11:40.311', NULL, '10922d908dd3b4fb42c989bb75391935', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0V7D4VQ6YHAWGFH472ZAJM', '06FT0V8B51YH76EGJ3A3RTRTHC', '2026-07-26 21:55:31.292', '2026-07-26 21:55:31.292', NULL, '5769b4c1244e29ca6b1c417937111b7f', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0V7D4VQ6YHAWGFH472ZAJM', '06FT0VCC9809R320XQ28RNWXN4', '2026-07-26 21:56:03.787', '2026-07-26 21:56:03.787', NULL, '2d1599d90a9ada7990bafdaae8c4c8fb', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0TX2W0BA10PRR7NMJ2362M', '06FT0VERS6AKQHB30Y1RSRW1FM', '2026-07-26 21:56:23.415', '2026-07-26 21:56:23.415', NULL, 'c95679bb40ed7cd8d514b7cd09526f3b', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0V4HQKPYPVQYAKYVFK68C4', '06FT0VERS6AKQHB30Y1RSRW1FM', '2026-07-26 21:56:23.446', '2026-07-26 21:56:23.446', NULL, 'bbd26365d657de3bd63d2fa1b9d33294', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0TYBD74TQNVKTJMKA8D9KM', '06FT0TZC9MJV6KZTSRVRYQ327M', '2026-07-26 21:56:27.160', '2026-07-26 21:56:27.160', NULL, 'd789b277bd2657366354ef108efd0128', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0TYBD74TQNVKTJMKA8D9KM', '06FT0V04XYBWC93Q1QTETMSQE8', '2026-07-26 21:56:27.189', '2026-07-26 21:56:27.189', NULL, '8fd4f8cee992ea0d8e65982055684db3', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0TZC9MJV6KZTSRVRYQ327M', '06FT0V0WBYS20XNMEB67A6AT8G', '2026-07-26 21:56:31.307', '2026-07-26 21:56:31.307', NULL, '062b9827a6527aab503aba3c441bf386', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0V04XYBWC93Q1QTETMSQE8', '06FT0V0WBYS20XNMEB67A6AT8G', '2026-07-26 21:56:31.327', '2026-07-26 21:56:31.327', NULL, '74dfdbce427031866b673b86c60a19b8', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); +INSERT INTO ticket_deps (blocker_record_id, blocked_record_id, created_at, updated_at, deleted_at, hash, canonical_version) VALUES ('06FT0V0WBYS20XNMEB67A6AT8G', '06FT0V18KE3QPZ8CYRTVD883VR', '2026-07-26 21:56:31.473', '2026-07-26 21:56:31.473', NULL, 'd45e5ceb46fd5629d9f08915cbe1cb70', 2) ON CONFLICT(blocker_record_id, blocked_record_id) DO UPDATE SET updated_at=excluded.updated_at, deleted_at=excluded.deleted_at, hash=excluded.hash, canonical_version=excluded.canonical_version WHERE excluded.updated_at > ticket_deps.updated_at OR (excluded.updated_at = ticket_deps.updated_at AND excluded.hash > ticket_deps.hash); diff --git a/client/tests/test_step_canvas_annotation_layer.gd b/client/tests/test_step_canvas_annotation_layer.gd index c664d6f9f..5718979f7 100644 --- a/client/tests/test_step_canvas_annotation_layer.gd +++ b/client/tests/test_step_canvas_annotation_layer.gd @@ -39,15 +39,64 @@ func test_clear_frame_drops_the_held_canvas() -> void: ## placement (center_world_m + (col - half_w) * step_m) — a settlement id ## read from cell (col, row) must map back to the world point that cell was ## actually derived at. -func test_cell_center_world_m_matches_the_servers_own_per_cell_placement() -> void: +## The invariant that survives the D-255 extent inversion, the viewport's +## aspect ratio, AND panning: **half the SHORT axis is half a rung cell.** +## +## Post-inversion the pitch is derived from the canvas rather than fixed per +## rung, so no literal spacing belongs in this test. Two things it must NOT +## assert, both of which an earlier version got wrong: +## +## - *"the corner is half a district away"* holds only on a SQUARE canvas. +## A real widescreen canvas spans one cell on its short axis and ~1.78 on +## its long one, so the corner is half a district on one axis only. +## - The canvas is one district **wide**; it is not sitting **on** a +## district. It is a free-floating window centred wherever the player has +## panned, with no relationship between its edges and any district +## boundary — zoom is stepped, pan is continuous (D-255 premise 3). +## A rung names a scale, the way a paper map says 1:25,000; it does not +## name a cell you are inside. +func test_cell_center_world_m_short_axis_spans_exactly_one_rung_cell() -> void: + var cell_m: float = StepCanvasTransport.RUNG_EXTENT_M["District"] + # Landscape, portrait and square — the short axis is chosen by size, never + # by which axis happens to be the width. + for extent in [Vector2i(8, 4), Vector2i(4, 8), Vector2i(6, 6)]: + var layer: StepCanvasAnnotationLayer = auto_free(StepCanvasAnnotationLayer.new()) + add_child(layer) + layer.set_frame( + {"width": extent.x, "height": extent.y, "courses": []}, + Vector2.ZERO, + "District", + extent, + 0.0 + ) + var corner: Vector2 = layer._cell_center_world_m(0, 0) + # On the SHORT axis, the edge sits half a rung cell from centre. + var short_axis_offset: float = absf(corner.y if extent.y <= extent.x else corner.x) + assert_float(short_axis_offset).override_failure_message( + "%s: short-axis edge is %f m from centre, want half a District (%f)" + % [extent, short_axis_offset, cell_m * 0.5] + ).is_equal_approx(cell_m * 0.5, 0.001) + # And the centre cell is the world centre, whatever the shape. Note the + # centre is (width/2, height/2) — NOT the same index on both axes once + # the canvas is not square. + assert_that(layer._cell_center_world_m(extent.x / 2, extent.y / 2)).is_equal(Vector2.ZERO) + + +## Panning does not align the canvas to anything — the frame's world centre is +## wherever the player stopped, and every cell offset is measured from it. A +## centre deliberately off any round boundary guards against a future change +## quietly snapping the canvas to the rung's own lattice, which would make pan +## step instead of slide. +func test_cell_center_world_m_is_independent_of_grid_alignment() -> void: var layer: StepCanvasAnnotationLayer = auto_free(StepCanvasAnnotationLayer.new()) add_child(layer) - layer.set_frame({"width": 4, "height": 4, "courses": []}, Vector2(0.0, 0.0), "District", Vector2i(4, 4), 0.0) - - # half_w = half_h = 2; spacing = 2048. Cell (0,0) -> (0-2)*2048 = -4096 on - # both axes; cell (2,2) (the center-ish cell) -> (2-2)*2048 = 0. - assert_that(layer._cell_center_world_m(0, 0)).is_equal(Vector2(-4096.0, -4096.0)) - assert_that(layer._cell_center_world_m(2, 2)).is_equal(Vector2.ZERO) + var ragged := Vector2(1_234_567.0, -987_654.0) # deliberately unaligned + layer.set_frame( + {"width": 4, "height": 4, "courses": []}, ragged, "District", Vector2i(4, 4), 0.0 + ) + assert_that(layer._cell_center_world_m(2, 2)).override_failure_message( + "the centre cell must be the frame's world centre exactly — no snapping" + ).is_equal(ragged) func test_cell_center_world_m_offsets_by_the_frames_world_center() -> void: @@ -74,7 +123,7 @@ func test_world_to_local_uses_the_held_frame() -> void: add_child(layer) var world_center := Vector2(5_000.0, -3_000.0) var extent := Vector2i(32, 32) - layer.set_frame({"width": 32, "height": 32, "courses": []}, world_center, "Quarter", extent) + layer.set_frame({"width": 32, "height": 32, "courses": []}, world_center, "Quarter", extent, 0.0) var expected: Vector2 = StepCanvasTransport.world_m_to_canvas_local( world_center, world_center, "Quarter", extent, 0.0 diff --git a/tests/run-godot b/tests/run-godot index b6713e382..d37a1c45f 100755 --- a/tests/run-godot +++ b/tests/run-godot @@ -106,12 +106,34 @@ if [[ "$TOTAL" -eq 0 ]]; then fi fi +# --- Harness-integrity guards (2026-07-26) ------------------------------- +# A test file that fails to PARSE never runs, and gdUnit4 reports whatever +# DID run as a clean pass — so a broken suite reads as success. Both halves +# of that bit on the same day: +# * a filtered run printed {"total":0,...} followed by "Tests passed"; +# * a full run printed 3610 passed / 0 failed while silently dropping an +# entire suite whose parse error had gone unnoticed for hours. +# Neither is a test RESULT, so neither may be reported as one. A run that +# executed zero tests is never a pass, and a run that could not load a suite +# is a harness failure regardless of how many other suites went green. +LOAD_ERROR_COUNT=$(grep -c 'Failed to load script' "$LOG_FILE" 2>/dev/null || true) +LOAD_ERROR_LIST=$(grep -oE 'Failed to load script "[^"]+"' "$LOG_FILE" 2>/dev/null | sort -u || true) +HARNESS_ERROR="" +if [[ "${LOAD_ERROR_COUNT:-0}" -gt 0 ]]; then + HARNESS_ERROR="load_error" +elif [[ "$TOTAL" -eq 0 && "$TIMED_OUT" != "true" ]]; then + HARNESS_ERROR="no_tests" +fi + LOG_LINES=$(wc -l < "$LOG_FILE" 2>/dev/null || echo 0) # Single-line JSON summary on stdout — machine-parseable, small. if [[ "$TIMED_OUT" == "true" ]]; then printf '{"suite":"godot","total":%d,"passed":%d,"failed":%d,"duration_ms":%d,"timeout":true,"timeout_sec":%d,"log":"%s"}\n' \ "${TOTAL:-0}" "${PASSED:-0}" "${FAILED:-0}" "$DURATION_MS" "$TIMEOUT_SEC" "$LOG_FILE" +elif [[ -n "$HARNESS_ERROR" ]]; then + printf '{"suite":"godot","total":%d,"passed":%d,"failed":%d,"duration_ms":%d,"harness_error":"%s","log":"%s"}\n' \ + "${TOTAL:-0}" "${PASSED:-0}" "${FAILED:-0}" "$DURATION_MS" "$HARNESS_ERROR" "$LOG_FILE" else printf '{"suite":"godot","total":%d,"passed":%d,"failed":%d,"duration_ms":%d,"log":"%s"}\n' \ "${TOTAL:-0}" "${PASSED:-0}" "${FAILED:-0}" "$DURATION_MS" "$LOG_FILE" @@ -129,6 +151,23 @@ TEST_TIMEOUT: tests/run-godot exceeded ${TIMEOUT_SEC}s wall-clock cap (killed=${ grep -E 'STARTED|PASSED|FAILED' ${LOG_FILE} | tail -20 The last STARTED without a matching PASSED/FAILED is the hang site. EOF +elif [[ "$HARNESS_ERROR" == "load_error" ]]; then + cat >&2 <&2 <} target=${TEST_TARGET} log=${LOG_FILE} (${LOG_LINES} lines) + Either the filter matched no suite (check the name), or the target failed to + load. A run that executes nothing can never be reported as success. +EOF elif [[ "${FAILED:-0}" -gt 0 ]]; then cat >&2 <&2 fi +# A harness failure must never inherit gdUnit4's exit code — it exits 0 both +# when a suite fails to load and when the filter matches nothing, which is the +# whole reason these two states were invisible. +if [[ -n "$HARNESS_ERROR" && "$EXIT_CODE" -eq 0 ]]; then + exit 2 +fi + exit $EXIT_CODE