Repository navigation
test(runner): make the A6 and A8 asset checks follow #61's held catch - #66
Merged
Merged
Conversation
[Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] #61 changed the pursuer's catch: the drone now leaps past Hoppy, clamps its claws at CLAMP_AT (1.6 s), drops a world-space quarantine field and holds while the threat stays at 1. It also raised caught_sequence_seconds from 1.6 to 3.0. The asset checks, which CI didn't run, still expected the old catch, and both had failed since (#65). A6 leaves the quarantine field out of the hunter envelope, checks the claws at CLAMP_AT, and asserts the leap past Hoppy without scaling. It also adds three checks: the field drops once the claws shut, the catch holds at threat 1, and a run reset ends it. The retreat pose comes from pursuit, not from the held catch. A8 runs the mock timeline until the demo finishes instead of for a fixed 405 ticks, so the Game Over sting still fires whatever the configured catch length, and it asserts that the timeline reached Game Over. The docs' A6 and A8 rows quote the current results. Closes #65.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The updated checks remain directly invokable as documented, and no concrete new merge-blocking risk is established. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review details
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
Issue #65 requires updated A6 and A8 checks and removal of both corresponding skip entries in the same pull request. The changed paths update tests/assets/a6_check.gd, tests/assets/a8_check.gd, an… |
Remove the two A6 and A8 [skip] entries from tools/tests/suites.toml in this pull request. Then run the affected suites with the repository test command and confirm that they pass without skips. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly identifies the A6 and A8 asset-check updates and connects them to the held catch introduced by #61.Description check The description explains the failure cause, implementation changes, verification results, and the dependency on #40. It does not use the exact template headings or include the checklist, but it provid…Out of Scope Changes check The changed test logic directly covers the failures in issue #65. The A6 changes align bounds and catch assertions with the current pursuer choreography. The A8 change runs the mock timeline to `finis…Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Full details: Linked Issues check
-
Explanation
Issue
#65requires updated A6 and A8 checks and removal of both corresponding skip entries in the same pull request. The changed paths updatetests/assets/a6_check.gd,tests/assets/a8_check.gd, and documentation. The PR description explicitly defers removal of the skips to#40or a follow-up. The check updates address the stale choreography and the reported A6/A8 failures, but the skip-removal requirement remains unmet.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
esthor
added a commit
that referenced
this pull request
Oct 15, 2026
…deterministically (#40) * test: run every suite in CI, fail on engine errors, and settle audio deterministically [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] CI ran a hand-maintained list of 24 suites and failed only on a nonzero exit or "SCRIPT ERROR". New tests silently never ran, and the 11 runner asset checks were never in CI at all. Meanwhile one of them, a1_check, had been failing since an editor session let Godot's detect-3D import flip the Hoppy runner textures from lossless to VRAM compression. - tools/tests/run_tests.py discovers every tests/**/*.gd, runs them in parallel with a per-suite HOME (so user:// never touches real files), and fails on a nonzero exit, a timeout, engine or script errors, or exit-time leaks. tools/tests/suites.toml holds the six skipped GPU, window and hover suites with reasons, plus runner_logic's one intentional error; stale entries fail the run. - AudioService.settle() stops the shared music and waits until the audio thread has released each playback. The fixed 0.3 s waits raced the mixer under parallel load and leaked at exit; every suite now settles before quitting. - The Hoppy texture imports are lossless again with detect-3D disabled, which matches a1_check and keeps the editor from flipping them back. - CI caches .godot imports, runs four suites at a time, uploads the results summary, and unit-tests the runner in the lint job. * fix(tests): isolate user:// on every platform and run timing suites alone [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] - CI on Linux failed assets/prewarm_check: its 1000 ms cold-warm budget can't hold while three other suites share the runner's CPU. A new [serial] table in suites.toml runs timing suites alone after the parallel batch. - assets/starship_check quit without settling the shared audio and leaked at exit; it now awaits AudioService.settle() like the others. - CodeRabbit (#40): Windows keeps user:// under %APPDATA%, which HOME doesn't redirect. The per-suite environment now also sets APPDATA, LOCALAPPDATA and USERPROFILE. Before running, the runner asks Godot where user:// resolves and refuses to start if it escapes the private directory. * fix(tests): verify isolation before any Godot run and name leaked objects [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] - CodeRabbit (#40): the isolation probe now runs before the resource import, and the import itself uses the isolated environment, so no Godot process touches the caller's editor data or user:// first. - When a suite fails only on an exit-time leak, the runner reruns it once with --verbose and appends the Leaked instance / Resource still in use lines to the failure. The suite still fails; the rerun is diagnosis only. assets/starship_check leaks on Linux CI but not locally, and this will say what is leaking. * fix(tests): fail the import on engine errors and let CI use the runner's import [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] Addresses CodeRabbit review on #40: a zero-exit import that reports an engine or script error now stops the run, using the same detection as the suites. CI drops its separate import step so that check applies there too. * fix(tests): classify exit leaks from verbose output and tolerate only the audio-teardown race [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] Linux CI still reported the shared music and UI voices as leaked in the 3D suites (a2_check, starship_check, runner_smoke) even after AudioService.settle(): the engine releases audio playbacks on its audio thread, which a loaded runner starves. A diagnostic rerun can't classify a transient leak, so every suite now runs with --verbose and its own log names what leaked. A leak made only of AudioStream* instances and audio files is recorded as a note. Any other leaked instance or resource fails the suite with its class names. Verbose loader noise is dropped from the displayed log. * fix(tests): count Ogg internals as audio teardown and settle the booth suite [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] - Linux CI reported heroes_villains_logic's music as leaked through Ogg Vorbis's internal OggPacketSequence and OggPacketSequencePlayback objects, the same audio-thread race as the MP3 playbacks. They now count as audio for the tolerated shape; any other leaked class still fails. A unit test covers it. - tests/booth_mode.gd (merged in #43) now ends with AudioService.settle() like every other suite. - Merged main (#43, #67); the vendor-lock check in lint and the pre-commit hook are kept. * chore(tests): run A6 and A8 again now that #66 fixed them [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] #66 merged first, so drop the two [skip] entries that pointed at #65. * fix(tests): run a2_check alone and classify main's new scripts [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] godot-tests failed on assets/a2_check ("asynchronous prewarm completed before texture census") in three of the last four CI runs. a2_check waits at most 15 s of wall time for the runner's asynchronous prewarm, which runs on Godot's single low-priority worker. The CI runner has 2 cores ("WorkerThreadPool: 2 threads, 1 max low-priority") and the batch runs 4 suites at once, so the prewarm starves. It passed when the batch was lighter. The check itself is unchanged; it now runs in the serial phase with prewarm_check, the way main's one-at-a-time loop ran it. The merge brought in scripts that main's hand list never ran: - runner_synth_threads parks a thread on every core and releases them together. It needs idle cores and starves its neighbours: serial. - capture_code_city_bosses writes docs/previews/ stills in a graphical run: skipped, like capture_runner_menu_previews. - exit_leak needs a --scenario per process and is driven by tools/checks/exit_leak.py, which CI runs as its own step: skipped. * fix(tests): keep non-audio exit leaks failing and record setup failures [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] Addresses the open CodeRabbit comments on #40: - A suite whose log had the audio-teardown race plus another exit leak, such as "RID allocations ... were leaked at exit", passed: run_suite() dropped every leak line once the verbose details were all audio. Only the two engine summary lines that those details explain are dropped now, and any other leak report fails the suite. run_suite() is tested with a mixed audio and RID log. - An isolation or import failure returned before summary.json was written, so CI uploaded no results. Both now write the summary with a setup_error, covered by two tests. - README says how suites are discovered (scripts that extend SceneTree or MainLoop), not that every script under tests/ is one. - Docstrings on the runner's remaining functions. * fix(runner): restore the boss recipe sources the committed models were baked from [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] Running every suite showed assets/boss_legacy_monolith_check failing on main: "provenance hash matches source" for build_legacy_monolith.py and boss_kit.py. main's CI never ran the asset checks, so nobody saw it. Each boss bake records the SHA-256 of its recipe sources in its .build.json and in the GLB's asset extras. The four bosses were baked at 4ab289d in #64's branch. #68, a lint autofix merged into that branch afterwards, then removed unused imports from boss_kit.py, build_legacy_monolith.py and build_merge_conflict_hydra.py. That edit was import-only, so it doesn't change any output, but the recorded hashes no longer matched the sources. All four reports were stale. Only the monolith check compares them. This restores those three files byte for byte to 4ab289d. All four reports now match their sources and GLBs again, and no asset changes. ruff.toml exempts only those three files from F401, with the reason, so the next lint fix doesn't drift them again. A rebake can drop the imports and the exemption together. * fix(tests): record a probe or import that can't start or times out [Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none] Addresses CodeRabbit's full review of #40: - check_isolation() returns a reason when the probe can't launch (a wrong --godot path) or times out, instead of raising. main() then refuses to run and writes the summary with setup_error. - The resource import handles the same two cases: it writes the summary and returns 1 without a traceback. - Tests cover both probe cases and the import timeout. - CONTRIBUTING names MainLoop beside SceneTree in the discovery rule.
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.
[Agent: Claude Code | Model: Opus 5.5 | Thinking: High | Acting on behalf of: Erik | Skill: none]
Closes #65.
Why
assets/a6_check(pursuer) andassets/a8_check(runner audio) have failed onmainsince #61. I bisected it: both pass at1fede76and fail atf586a70. CI doesn't run the asset checks yet (#40 changes that), so nothing flagged it.#61 deliberately changed the pursuer's catch. Its description says "The catch now holds while the threat stays at 1 instead of snapping to idle at 1.4 s". The drone now leaps past Hoppy, clamps at
CLAMP_AT(1.6 s) and drops a world-space quarantine field, andcaught_sequence_secondswent from 1.6 to 3.0. #61 updated A1 and the visual smoke for these changes but not A6 or A8. These checks were stale; the game code is fine.What
tests/assets/a6_check.gd:CLAMP_ATNew checks:
tests/assets/a8_check.gd: runs the mock timeline until the demo reachesfinished(bounded at 600 ticks) instead of a fixed 405 ticks. That way the Game Over sting fires whatevercaught_sequence_secondsis, and a new check asserts the timeline reached Game Over.docs/endless-runner.md: the A6 row now describes the current hunter and its census (12 draws, 3,840 triangles, no textures); both rows quote the current results.Verification (Godot 4.7.1, macOS, headless)
mainassets/a6_checkassets/a8_checktools/checks/repo_hygiene.py --allpasses, and no new line is over gdlint's length limit.Order with #40: #40 skips these two suites in
tools/tests/suites.toml, citing #65. If this merges first, I'll remove those skips from #40 before it merges. Otherwise a one-line follow-up removes them.Summary by CodeRabbit