test(harness): close the stale-assemble hole, and decide the dropped assertion
improvise-669b. PR #65 single-homed HANDSHAKE_TIMEOUT_MS in web/shell.js, but the home is single in the TREE: the spec imports web/shell.js and the page loads harness/serve/app/shell.js. createShell now exposes timeoutMs() beside timesArmed(), and outlastTheWatchdog requires the running page's value to equal the one it imported. Injected with the ticket's recipe (8000 -> 100 in source, no rebuild): 5 passed / 1 failed where it was previously 6/6 green. Also in scope, the same file's stale line: HANDSHAKE_TIMEOUT_MS restates nothing and one test outlasts it, not two. improvise-qf57. The formula-row click's `#grid input` toHaveCount(0) stays dropped, and now deliberately. The reviewer's argument was that the exact `#mode` assertion subsumes it; measuring gives a better one. app.js creates a grid <input> in exactly one place, the grid cell's own mousedown, so no panel handler can reach it — and an invented formula-row activation opens the MINIBUFFER, not a grid editor. Injecting ChangeMode(formula_edit()) into click_formula_row: the toHaveCount(0) probe PASSES and the run fails on the #mode assertion below it. Restoring it would add an assertion that cannot fail, which context/test-evidence/clients.md already records as the reason a sibling assert! was removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -810,6 +810,22 @@ the keyboard disagreed. `the_formula_panel_has_no_enter_to_mirror_and_the_view_p
|
|||||||
asserts the binding's absence and names `click_formula_row` as what must change
|
asserts the binding's absence and names `click_formula_row` as what must change
|
||||||
if it ever appears.
|
if it ever appears.
|
||||||
|
|
||||||
|
**The browser assertion that PR #60 dropped stays dropped, and improvise-qf57
|
||||||
|
is the decision rather than the oversight.** `#grid input` `toHaveCount(0)` was
|
||||||
|
argued to be redundant with the `#mode` assertion on the grounds that
|
||||||
|
`toHaveText("FORMULAS")` is exact and `AppMode::FormulaEdit` labels as
|
||||||
|
`"FORMULA"` — a coincidence of two strings differing by one character, which
|
||||||
|
would be a thin reason to leave it out. Measuring it gives a better one: it is
|
||||||
|
not redundant, it is **unreachable**. `web/app.js` creates a grid `<input>` in
|
||||||
|
exactly one place, the grid cell's own `mousedown`, gated on `cell.editable`;
|
||||||
|
no repaint creates one and no panel handler can reach it. And an invented
|
||||||
|
formula-row activation opens the **minibuffer**, not a grid editor. Injecting
|
||||||
|
`Command::ChangeMode(AppMode::formula_edit())` into `click_formula_row` and
|
||||||
|
running the spec: `#grid input` `toHaveCount(0)` **passes**, and the run fails
|
||||||
|
on the `#mode` assertion below it. Restoring it would add an assertion that
|
||||||
|
cannot fail — the same defect this file already records one line down, where an
|
||||||
|
explanatory `assert!` was removed for never being able to be the failing one.
|
||||||
|
|
||||||
**Rule 2 — a click is all of its intents or none of them** (improvise-hkr8).
|
**Rule 2 — a click is all of its intents or none of them** (improvise-hkr8).
|
||||||
Each `click_*` returns `Option<Vec<Command>>` and `click` does the single
|
Each `click_*` returns `Option<Vec<Command>>` and `click` does the single
|
||||||
`unwrap_or_default`. The case that matters is an activation the client cannot
|
`unwrap_or_default`. The case that matters is an activation the client cannot
|
||||||
|
|||||||
@@ -537,9 +537,13 @@ file server — **deterministic where a closed port is not**: a refused port's
|
|||||||
`close` timing is a property of the host's network stack, and on the GitHub
|
`close` timing is a property of the host's network stack, and on the GitHub
|
||||||
runner it did not arrive inside the window. The page cannot tell the two apart
|
runner it did not arrive inside the window. The page cannot tell the two apart
|
||||||
and neither can `web/app.js`: in both cases the socket never opened.
|
and neither can `web/app.js`: in both cases the socket never opened.
|
||||||
`HANDSHAKE_TIMEOUT_MS` restates `web/app.js`'s own constant, because two tests
|
`HANDSHAKE_TIMEOUT_MS` is **imported from `web/shell.js`** and restates nothing
|
||||||
have to outlast it and a number that silently disagreed with the page would make
|
(PR #65), and **one** test outlasts it — this sentence said the opposite of both
|
||||||
them pass for the wrong reason. `logStatus` records **every distinct `#status`**,
|
for a while, which is gotcha 48's third clause: a fact stated twice is changed by
|
||||||
|
a grep, not an edit. Since improvise-669b the spec also compares the *running
|
||||||
|
page's* window against the one it imported, because the single home is in the
|
||||||
|
TREE and the page loads `harness/serve/app/shell.js` — see
|
||||||
|
`outlastTheWatchdog` below. `logStatus` records **every distinct `#status`**,
|
||||||
not just the last one, because several of these tests are about a message being
|
not just the last one, because several of these tests are about a message being
|
||||||
*replaced*, which the final value cannot see.
|
*replaced*, which the final value cannot see.
|
||||||
|
|
||||||
@@ -624,6 +628,25 @@ watchdog never given the chance — vacuous, and green. `outlastTheWatchdog` rea
|
|||||||
moved. Demonstrated both ways: with the install removed the suite passed 6/6,
|
moved. Demonstrated both ways: with the install removed the suite passed 6/6,
|
||||||
and with the guard in place the same omission fails by name.
|
and with the guard in place the same omission fails by name.
|
||||||
|
|
||||||
|
**And it guards its second premise since improvise-669b: that the page runs the
|
||||||
|
constant the spec imported.** PR #65 single-homed the handshake window in
|
||||||
|
`web/shell.js`, which closed the *understated spec constant* — but the home is
|
||||||
|
single in the **tree**, not in what the page runs: the spec imports
|
||||||
|
`../../web/shell.js` and the page loads `harness/serve/app/shell.js`, the copy
|
||||||
|
`web/assemble.sh` put there. A stale `serve/` restores the exact vacuity, and
|
||||||
|
review demonstrated it — shrink the constant in source only, revert both socket
|
||||||
|
arms, 6/6 green. `createShell` now exposes `timeoutMs()` beside `timesArmed()`
|
||||||
|
and `outlastTheWatchdog` reads the running page's value and requires it to equal
|
||||||
|
the import. Injected with the ticket's own recipe (`8000` → `100` in
|
||||||
|
`web/shell.js`, no rebuild): the run goes to 5 passed / 1 failed, on
|
||||||
|
"the page's handshake window is not the one this spec imported", where the same
|
||||||
|
staleness was previously invisible.
|
||||||
|
|
||||||
|
Note what this does and does not reach: it guards the tests that *call the
|
||||||
|
helper*. `"a peer that accepts and then says nothing trips the watchdog"` also
|
||||||
|
names `HANDSHAKE_TIMEOUT_MS`, but as a Playwright `timeout:` margin rather than
|
||||||
|
as a window to outlast — a stale value there costs patience, not evidence.
|
||||||
|
|
||||||
**The window is single-homed, and that is the second half of the same
|
**The window is single-homed, and that is the second half of the same
|
||||||
finding.** `HANDSHAKE_TIMEOUT_MS` lived twice — once in `web/app.js`, once in
|
finding.** `HANDSHAKE_TIMEOUT_MS` lived twice — once in `web/app.js`, once in
|
||||||
the spec — with a comment saying a silent disagreement "would pass for the
|
the spec — with a comment saying a silent disagreement "would pass for the
|
||||||
|
|||||||
@@ -214,6 +214,8 @@ test.describe("a pointer event reaches the client and its answer is painted", ()
|
|||||||
|
|
||||||
// A formula row: navigation only, because FormulaPanel binds no Enter and
|
// A formula row: navigation only, because FormulaPanel binds no Enter and
|
||||||
// a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md.
|
// a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md.
|
||||||
|
// `#grid input` toHaveCount(0) belongs here and is deliberately absent:
|
||||||
|
// it cannot fail, measured by injection (improvise-qf57, clients.md).
|
||||||
await page.keyboard.press("Shift+F");
|
await page.keyboard.press("Shift+F");
|
||||||
await expect(page.locator("#formulas li").first()).toBeVisible();
|
await expect(page.locator("#formulas li").first()).toBeVisible();
|
||||||
await page.keyboard.press("Escape");
|
await page.keyboard.press("Escape");
|
||||||
|
|||||||
@@ -41,6 +41,12 @@ async function outlastTheWatchdog(page) {
|
|||||||
await page.evaluate(() => window.__improviseShell?.timesArmed() ?? 0),
|
await page.evaluate(() => window.__improviseShell?.timesArmed() ?? 0),
|
||||||
"the watchdog was never armed on this path: denying its message proves nothing",
|
"the watchdog was never armed on this path: denying its message proves nothing",
|
||||||
).toBeGreaterThan(0);
|
).toBeGreaterThan(0);
|
||||||
|
// The import is the SOURCE tree's constant; the page runs the ASSEMBLED
|
||||||
|
// copy. A stale `serve/` makes them differ — improvise-669b, clients.md.
|
||||||
|
expect(
|
||||||
|
await page.evaluate(() => window.__improviseShell?.timeoutMs() ?? null),
|
||||||
|
"the page's handshake window is not the one this spec imported: re-run harness/build.sh",
|
||||||
|
).toBe(HANDSHAKE_TIMEOUT_MS);
|
||||||
const jump = HANDSHAKE_TIMEOUT_MS + 2_000;
|
const jump = HANDSHAKE_TIMEOUT_MS + 2_000;
|
||||||
const before = await page.evaluate(() => Date.now());
|
const before = await page.evaluate(() => Date.now());
|
||||||
await page.clock.runFor(jump);
|
await page.clock.runFor(jump);
|
||||||
|
|||||||
@@ -153,6 +153,9 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT
|
|||||||
this.setShell(text);
|
this.setShell(text);
|
||||||
},
|
},
|
||||||
armed: () => watchdog !== null,
|
armed: () => watchdog !== null,
|
||||||
|
/** The window this shell was built with. Exposed so a spec can compare the
|
||||||
|
* RUNNING page's value against the one it imported — improvise-669b. */
|
||||||
|
timeoutMs: () => timeoutMs,
|
||||||
/** How many times the watchdog has been armed. A test denying its message
|
/** How many times the watchdog has been armed. A test denying its message
|
||||||
* needs this to be nonzero, or it is denying an unreachable event. */
|
* needs this to be nonzero, or it is denying an unreachable event. */
|
||||||
timesArmed: () => arms,
|
timesArmed: () => arms,
|
||||||
|
|||||||
Reference in New Issue
Block a user