Ask where an option's digit stands as a relation, not as a number - #66
Conversation
max-sixty
left a comment
There was a problem hiding this comment.
Reviewed the exact head. The new assertions test the layout relation instead of duplicating the theme’s old 6px result, while retaining the cross-form same-column invariant. Focused browser coverage passes (test_a_questions_digits_are_drawn_whole) and both changed files pass pre-commit. No blocking finding; exact-head full CI remains the merge gate.
leaf-agent
left a comment
There was a problem hiding this comment.
The test check on this head is red, and it is not this change. Run 33033719637 finishes 1 failed, 1330 passed with the single failure in tests/test_render_pages.py::test_opposite_margin_residents_wait_for_the_room_they_need. test_a_questions_digits_are_drawn_whole passes, which is what this PR set out to prove and what the red main run it came from could not: that one finished 2 failed, 1329 passed with both tests down, so the delta between the two runs is exactly this PR's subject and nothing else.
The residual failure reports byte-identical numbers on both runs — assert 720 == (720 - 15), same sidebar rects, same gutter: 15 — so it is deterministic rather than one of the arrival races running-tend warns about, and #65 is where it is being fixed: that branch rewrites the same test function, at test_render_pages.py hunks inside test_opposite_margin_residents_wait_for_the_room_they_need. The practical consequence for merge order is that this PR cannot show an all-green test until #65 lands, so the red X here should not be read as a reason to hold it.
How the two runs were compared
# this PR, 1c3f14c
FAILED tests/test_render_pages.py::test_opposite_margin_residents_wait_for_the_room_they_need
============ 1 failed, 1330 passed, 6 skipped in 1252.28s ============
# main at 6e655de, the run this PR responds to
FAILED tests/test_render_navigation.py::test_a_questions_digits_are_drawn_whole - AssertionError: c-heater's digit sits 15 in from its option's left edge
FAILED tests/test_render_pages.py::test_opposite_margin_residents_wait_for_the_room_they_need
============ 2 failed, 1329 passed, 6 skipped in 1482.36s ============
Reviewed separately from the assertions themselves: afterStatus and x share a coordinate origin because lf-option { display: block; position: relative; } (theme.css) makes the option the containing block for both the status ::before and .lf-address, and x/ends are stated from that same padding box — so the ordering compares boxes in one space. The lower bound the removed round(sits["x"]) == 6 used to hold is re-established and tighter: afterStatus is --sp-2 plus --lf-status-bar-w, so afterStatus < x puts the digit past 11 rather than at 6, and ends < opens bounds the far side against padding-inline-start. INSIDE_ITS_OPTION has one caller, so the added keys break nothing.
|
Closing unmerged because #56/current main supersedes this test-only repair. The merged option regression now checks the same status → keyboard address → prose relationship directly in |
|
Correction to my close: current main checks the status → address → prose relation directly for the card form, but not for the row form, whose horizontal placement historically differed. This PR’s cross-form assertion therefore still adds material coverage. Reopening it; I’ll refresh it onto the repaired main and require exact-head CI/review before merge. |
1c3f14c to
a33e0ab
Compare
|
The rebased code remains a reasonable test hardening, but the description no longer matches the current base. #56 already replaced the stale Please refresh the Problem/Solution/Testing sections to describe that incremental coverage against current |
leaf-agent
left a comment
There was a problem hiding this comment.
One inline note on the new cross-reference comment — it misstates the test it points at.
Separately, on what len(seats) == 1 gates rather than as a defect: .lf-address takes inset-inline-start: var(--lf-address-start) from a single unqualified rule, and --lf-address-start is declared once on lf-options[choose]:not(:where([data-lf-exhibit]) *), so under today's theme the two forms cannot land on different columns — the assertion's live subject is a future form-specific override, which is what the planted 3px offset in the bug-back exercised. Worth stating because that cross-form claim is the coverage this PR was reopened for, and it reads as a stronger present-tense check than it is.
…x retired (#65) ## Problem `main` had two independent stale layout assertions: - The margin-resident test expects a 720px column minus a 15px scrollbar gutter, although #64 now correctly preserves the full 720px column at and above the combined strip floor. - The option-address test pinned a digit to the old 6px inset, although the status stripe now occupies the start of that gutter and the digit correctly begins at 15px. #56 has since repaired the stale option-address assertion on `main` and directly checks the status/address/prose relation for the card form. This branch is therefore back to the scrollbar-gutter repair alone. #66 remains separate: it has been reopened to retain the corresponding row-form and cross-form coverage. ## Solution The runtime now has one `scrollerGutter` reading beside `pageScroller`; `stateStrip` and `stateRoom` both use it. The browser test asks that production helper for the width it drives, but first cross-checks it against an independent body-box reading. That guard catches a correlated overread before the helper can choose and judge the same viewport. The roomy column is then required to remain the full 720px; the retired gutter tolerance and its prose form are removed. ## Testing - `test_opposite_margin_residents_wait_for_the_room_they_need`, the current `test_a_questions_digits_are_drawn_whole`, and #56's direct option-pointer regression: 3 passed together after merging current `main`. - All four files changed relative to current `main` pass pre-commit. - Gutter overread bug-back: without the independent cross-check, a planted 30px reading passes; with the guard, it fails at `30 == 15`. - Removing `scrollerGutter()` from the strip calculation fails at `705 == 720`. Exact-head full CI and review remain the merge gate. --------- Co-authored-by: leaf-agent <318509791+leaf-agent@users.noreply.github.com> Co-authored-by: Maximilian Roos <m@maxroos.com>
|
Description refreshed against current One framing change beyond the rewrite, from the note in my earlier review: The branch was rebased onto |
leaf-agent
left a comment
There was a problem hiding this comment.
Exact-head review on cd24b3a, since that is one of the gates named above. No new findings; the one inline note from the previous review is resolved and nothing in the merge disturbs the change.
The suggestion landed verbatim in c92ed49, and I checked the corrected claim against the test it points at rather than against the commit message: #l-stage is <lf-option id="l-stage" recommended><strong>Migrate in stages</strong>… — a recommended, <strong>-titled cell, so "one recommended cell of the card form" is right on both counts — and the boundary reading in test_the_pointer_does_not_take_a_cells_status_with_it is taken after page.mouse.move(0, 0), which is what "with the pointer moved off it before the reading" now says.
Your merge of main into the branch touches none of this PR's subject: theme.css, render_cases_navigation.py, and test_render_navigation.py are all unchanged by it, and the only test it rewrites is test_a_pick_offered_can_be_pointed_at_too in test_render_options.py, which reads a version-stamping path rather than option geometry. So the green test the description cites on a33e0ab still describes the assertions this PR adds.
CI at this head is not yet verified green: lint is SUCCESS, test was still running when my poll cap expired (ci run 33042285330) — expected, since this suite runs ~21 minutes and the cap is ~9.5. That check is the remaining gate, not anything in the diff. No approval because the PR is bot-authored and GitHub rejects self-approvals.
What I re-derived on the merged tree
The two readings INSIDE_ITS_OPTION added share an origin with x, which is what makes afterStatus < x < ends < opens a comparison of boxes in one space:
lf-option { display: block; position: relative; }(theme.css) makes the option the containing block, so the absolutely positionedlf-option::beforeresolvesleft/widthagainst the option's padding box — the same edge the JS computes asopt.x + parseFloat(s.borderLeftWidth).- The status rule reaches the row form as well as the card form: it is declared as
& > lf-option::before { content: ""; position: absolute; inset-inline-start: var(--sp-2); … inline-size: var(--lf-status-bar-w) }nested underlf-options[choose]:not([settled]):not(:where([data-lf-exhibit]) *), which is unqualified by form. Had it been card-only,leftwould computeautoonr-now/r-later,parseFloatwould giveNaN, and every comparison against it would be false — so the row assertions would fail loudly rather than pass vacuously. opensaspaddingInlineStartis a distance from that same padding-box edge, and the padding it reads is set once for both forms bylf-options[choose]:not(:where([data-lf-exhibit]) *) > lf-option { padding-inline-start: var(--lf-address-col) }.
The len(seats) == 1 framing in the description also holds against the theme as it stands: .lf-address takes inset-inline-start: var(--lf-address-start) from a single unqualified lf-option > .lf-address rule, and the row-only rule is lf-options:not(:has(> lf-option > strong)) > lf-option > .lf-address { top: 0; bottom: 0; margin: auto 0 } — vertical placement alone.
|
Closing the CI gate my last review left open: That failure is inherited from You have since merged main again as How the runs were comparedThe residual failure from my earlier comparison, Both runs are attempt 1; neither was re-run, so the conclusions above are the ones the runs reported. |
Problem
test_a_questions_digits_are_drawn_wholechecks every address chip in the card and row forms, but its horizontal assertion only requiresx > 0. That permits a row address to drift into the status rule, overlap the prose column, or leave the card form's address column while the test remains green. The focused status-to-address-to-prose test covers one recommended card and does not read a row.Solution
INSIDE_ITS_OPTIONnow returns the chip's leading and trailing edges together with the end of the option's::beforestatus rule and the start of its prose column. The navigation test requiresafterStatus < x < ends < opensfor every card and row it already visits, then requires all five chips to share one address column.Testing
uv run pytest tests/test_render_navigation.py::test_a_questions_digits_are_drawn_whole -q -n0(1 passed)pre-commit run --files tests/render_cases_navigation.py tests/test_render_navigation.pygit diff --check main...HEADExact-head CI and review remain the merge gate.