Bound the margin layout wait so a stalled module fails the test, not the job - #154
Conversation
…the job `margins_laid_out` awaited the `/runtime/margin-layout.js` import inside `page.evaluate`, which takes no timeout in any binding. A preview that stalls on the way out would not fail in thirty seconds naming its test — it would hold the worker for the rest of the job step, and the share of the suite xdist had already handed that worker would never run. This is the class #84 and #95 bound elsewhere, reached by a helper added after them. Poll the import from the driver instead, against `SERVED_TIMEOUT_MS`, which is the deadline the payload's own probes carry. One round trip either way, and the layout still runs inside a frame.
leaf-agent
left a comment
There was a problem hiding this comment.
The bound does hold — I checked it against the pinned Playwright (1.62.0) rather than reading it off the docs, since these semantics aren't documented: a predicate returning a promise is awaited, and a never-resolving one raises TimeoutError at the driver deadline. Both call-site tests pass at -n0.
One thing the docstring doesn't say, and it's the half that bites: the , true) is what makes this a bounded single run, not tidying up after the comma operator. A predicate whose promise resolves falsy does not poll again — wait_for_function returns at once with that falsy handle and raises nothing. So returning the layout's own result instead, or dropping the true in a later edit, silently turns the wait into a no-op. Suggestion inline to put that in the docstring, since the paragraph added here is where the next reader will look.
That failure mode is already in the suite at one site, outside this diff. In tests/test_render_widgets.py the wait spelled fetch('/api/state').then(r => r.json()).then(s => s.events.some(e => e.kind === 'action' && e.action === 'accept')) resolves false until the accept lands, so it returns on the first poll and the read_events assertion under it runs unguarded — a wait that proves nothing, which is the class tests/CLAUDE.md opens on. It belongs in its own change, not this one; flagging it here only because this PR is what surfaced the shape.
How I checked
Probed with the repo's own .venv (playwright 1.62.0), against data:text/html:
| predicate | result |
|---|---|
() => new Promise(r => setTimeout(() => r(true), 300)) |
returns after 0.32s, predicate invoked once — the promise is awaited |
() => Promise.resolve().then(() => globalThis.flag) with flag flipping true at 400ms |
returns after 0.00s, one poll, no raise — falsy resolution ends the wait |
() => new Promise(() => {}), timeout=1500 |
raises TimeoutError after 1.50s — the driver-side bound this PR is for |
() => Promise.resolve().then(() => { throw new Error('boom') }) |
raises after 0.02s naming the error |
() => import('/nope/missing.js').then(() => true) |
raises after 0.00s naming the unresolvable specifier |
Call-site tests, --run-nightly -n0 on this branch: test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back and test_render_margin.py::test_the_margin_keeps_its_page_coordinate_while_the_reader_scrolls — 2 passed.
A falsy promise resolution ends wait_for_function rather than polling again, so returning the layout's own result would make the wait a no-op. The paragraph the review asked for records that where the next edit will read it.
… returns at once (#158) `test_accepting_a_suggestion_settles_it_and_reaches_claude` guarded its event-log read with a `wait_for_function` whose predicate fetched `/api/state` and returned a promise resolving to whether the log holds the `accept`. A falsy resolution ends that wait rather than polling again, so it returned `False` on the first poll and the `read_events` assertion under it ran unguarded — a wait that proves nothing, which is the class `tests/CLAUDE.md` opens on. This replaces it with `round_trip(page)`, the boundary that file names for reading the log ("Read the event log only after `round_trip`"), and the one the module's other log reads already use. Verified: `tests/test_render_widgets.py` green (65 passed), and the named test passes at `-n0`. Surfaced by the review on #154; it belongs here rather than in that diff. <details><summary>The falsy-resolution semantics, measured</summary> Not documented, so probed against the pinned Playwright (1.62.0) with the repo's own `.venv`: ```python pg.evaluate("() => { globalThis.flag = false; setTimeout(() => globalThis.flag = true, 800); }") h = pg.wait_for_function("() => Promise.resolve().then(() => globalThis.flag)") # elapsed 0.02s value: False flag now: False ``` The predicate's promise is awaited, but the wait returns on the first resolution whatever its truthiness — it raises nothing and does not poll again. The site here is the only one of this shape left in the suite: the harness's `margins_laid_out` wait resolves `true` explicitly (#154), and no other `wait_for_function` predicate in `tests/` returns a promise. </details> Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com>
Problem
The review on #153 (review) raised this while that PR was in flight; it merged before the point was actioned, so the shape is on
mainnow.margins_laid_outawaits the/runtime/margin-layout.jsimport insidepage.evaluate, whichtests/CLAUDE.mdnames as the unbounded case: "page.evaluatetakes no timeout in any binding, so a promise awaited inside it — an animation'sfinished, a module's load, a listener's next call — is a wait nothing bounds. It does not fail in thirty seconds naming its test: it spends the job's whole step, and the share of the suite already handed to that worker never runs."That is the class #84 and #95 bound at eleven other sites; this helper was added after them and reintroduces it at three call sites —
test_render_pages.pytwice andtest_render_margin.pyonce — all of them on the render suite's ordinary path.Solution
Poll the import from the driver with
wait_for_functionagainstrender_checks.SERVED_TIMEOUT_MS, the same patience the payload's own probes carry. The deadline moves to the driver side, which is the shapetests/CLAUDE.mdprefers; the round trip count and the frame the layout runs in are unchanged. The docstring now says why the wait is bounded from outside, so the next reader does not have to re-derive it.Testing
On this branch, rebased on
6d68a1b:uv run pytest tests— 740 passed, 6 skipped. That is the gate CI runs on this PR.uv run pytest tests/test_render_pages.py::test_a_left_sidebar_uses_the_margin_until_the_page_needs_it_back --run-nightly -n0— passes. This is the test Read the margin layout the resize scheduled before measuring the sidebar page #153 fixed, and the one the helper was written for.uv run pytest tests/test_render_margin.py tests/test_render_pages.py --run-nightly -n2— 62 passed, 2 failed. Both failures are pre-existing and untouched here (below).ruff checkandruff format --checkover the changed file pass.The two nightly failures, and why they are not this change
test_render_margin.py::test_g_addresses_the_page_map_prefix_in_its_announced_orderandtest_one_margin_item_owns_a_targets_controls_information_and_more_actionsboth wait form\s*page-map itemson the key line and readgm1–9page-map items/gm1–4page-map itemsinstead. I reran both at-n0with this diff stashed, on6d68a1bunmodified: they fail identically. They belong toaa41f7a("Keep chord hints stable through progress"), which changed the chord row to carry the item count, and #153's body already named them on its own base. No open issue tracks them yet; the nextcionmainshould route them totend-ci-fix.