Skip to content

Read the page-map chord's progress from its key faces, not the key line's text - #159

Closed
leaf-agent wants to merge 1 commit into
mainfrom
fix/ci-33343915646
Closed

Read the page-map chord's progress from its key faces, not the key line's text#159
leaf-agent wants to merge 1 commit into
mainfrom
fix/ci-33343915646

Conversation

@leaf-agent

Copy link
Copy Markdown
Collaborator

Problem

ci on aa41f7a ("Keep chord hints stable through progress") failed with 33 nightly tests red. Most have since been fixed on main (#134, #137, #145, #148, #149, #150, #151, #152, #153) or are carried by an open PR (#155's ring walk, #156's catalog digest). The two still failing on main at c0a5b65 are test_render_margin.py::test_g_addresses_the_page_map_prefix_in_its_announced_order and test_one_margin_item_owns_a_targets_controls_information_and_more_actions, which #154's body already named as aa41f7a's and left for this run.

Both are stale readings of the key line, not a regression. They pressed g, asserted the line contained m page-map items, pressed m, then asserted it contained 1–9 page-map items — a text read that only distinguished the two stages while the line dropped the keys the chord had consumed. aa41f7a is the change that stopped it doing that: keyline.js now draws every destination's complete route (completeChordSteps) and marks progress with data-lf-key-state, so the row reads gm1–9page-map items both before and after m — which is the behavior the commit was for. That commit updated the same assertions in test_render_navigation.py and test_render_anchors.py; test_render_margin.py spelled them differently and was missed.

Solution

Assert what now carries the distinction: the row's keycaps and their faces. After g the page-map row reads g m 1–9 with the list key neutral; after m the same three keys, with that key pressed. This is the idiom aa41f7a established in test_render_navigation.py, and it proves more than the old read did — the route's identity and its stability under progress, not just that some substring moved.

The selector for one binding's keycaps is now key_line_route in tests/render_harness.py beside key_line, so the next change to this shape has one name to grep rather than a per-file spelling. The five existing sites in test_render_navigation.py are left as they are: three of them also need the enclosing .lf-key-sequence element for sequence_geometry, so converting them is a refactor of green tests rather than part of this fix.

Testing

On this branch at c0a5b65, with Playwright driving the real headless shell:

  • uv run pytest tests/test_render_margin.py --run-nightly -n0 — 22 passed. Before the change, on the same checkout, both tests fail with Actual value: gTThreads panelgAAsks panelgMPage mapgm1–4page-map itemsgg / Gtop / bottomesccancel?more, which is where the 1–N between m and the word comes from.
  • uv run pytest tests/test_render_navigation.py tests/test_render_margin.py --run-nightly -n2 — 84 passed. The navigation file is included because it owns the assertions this one is being brought into line with, and because the new helper lives in the shared harness.
  • uv run pytest tests — 740 passed, 6 skipped. That is the gate CI runs on this PR.
  • ruff check tests/ and ruff format over the changed files — clean.

Seventeen of the run's other failures were re-run here to place them rather than assume the merges cleared them: tests/test_render_drafts.py --run-nightly -n2 — 50 passed, so all fourteen Locator.fill timeouts are gone, and the three navigation failures (test_a_questions_digits_are_drawn_whole, test_c_in_a_seated_conversation_reaches_the_thread_it_is_in, test_the_ring_holds_on_a_seat_the_agent_has_still_to_answer) passed in the navigation run above. The rest sit in test_render_controls.py, test_render_options.py, test_render_projection.py, test_render_gate.py, test_render_conversations.py, and test_render_anchors.py; #155 already owns the one known to survive there.


Automated fix for failed run

@leaf-agent

Copy link
Copy Markdown
Collaborator Author

This and #157 are the same fix, opened twelve minutes apart by two ci-fix runs, and neither references the other — so whichever lands second conflicts rather than becoming a no-op. Only one should merge. I was dispatched for run 33344080789 (21332fc), derived this same change a third time, and am dropping it rather than opening a fourth.

Both edit the same two page.keyboard.press("g") sites in tests/test_render_margin.py onto the same reading — locate the row by data-lf-commands~="navigation.page-map-item", assert the keycaps spell g m 1–n, and take the progress off data-lf-key-state. The differences are where the helper lives and how the span is spelled: this PR puts key_line_route in render_harness.py beside key_line, and writes 1–9 out at the first call site; #157 keeps expect_page_map_address local to the test module and derives the span from the list's count through address.js's own 1–n-or-1 rule, so a corpus change that drops PAGE_MAP_PAGE to a single map location keeps that site honest. Against that, the harness is the better home for the selector if these five sites in test_render_navigation.py are ever converted, which this PR's body already notes it is deliberately not doing.

Both are correct as tests: the row's text is unchanged by the m press, so the settle has to rest on a positive fact, and in both it does — to_have_text only matches once g has armed the chord and drawn the row, and the post-m assertion is the retrying pressed read rather than the neutral one.

Verification

uv run pytest --run-nightly over all 32 node ids from run 33344080789 at c0a5b65: 3 failed, 29 passed — these two margin cases and #156's catalog digest. With the equivalent change applied locally, tests/test_render_margin.py --run-nightly — 22 passed, and the full nightly surface green on this runner: everyday tests 740 passed / 6 skipped, the other nineteen test_render_*.py files 780 passed across three batches, test_site.py 20 passed.

@leaf-agent

Copy link
Copy Markdown
Collaborator Author

Closing this as the duplicate, in favour of #157 — the comment above established that only one should merge, and nothing since has picked one, so the second to land would conflict rather than no-op.

#157 is the one to keep on three counts: it was opened twelve minutes earlier, its test and lint checks are green at a9411f8 while this branch's are still running, and it already carries a review. On the substance, it derives the span from the list's own count at both call sites through address.js's 1–n-or-1 rule, where this PR writes 1–9 out at the first site — the difference the comment above already named as the honest one.

The one thing worth keeping from here is key_line_route in tests/render_harness.py: a single named selector for "the keycaps one key-line binding draws" is the better home if the five sites in test_render_navigation.py are ever converted, and it is a small graft onto #157 if a maintainer wants it. The branch is left in place rather than deleted, so nothing here is lost.

Diagnosed while handling run 33346442884 (6d68a1b), whose five failures resolve to #157's two margin cases, #156's catalog digest, and two Asks (N) count cases that #151 already fixed on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant