diff --git a/context/code/clients.md b/context/code/clients.md index e01c7b53..33f187f4 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -1215,6 +1215,24 @@ collapsed window, which a synchronous `runFor` gives no opportunity to deliver; that is a race whose outcome the page does not promise, so there is nothing to assert. State it rather than re-derive it. +**A negative assertion must demonstrate the denied event is reachable on that +exact code path** — not on a sibling path, and not in the rule's own unit test. +This file's worked example cost four review rounds. `shell.arm` runs only after +`await transport.ready`, so on a socket that never opens the watchdog is never +armed; a test denying "sent no document" there denies nothing, and passed +against both the routing revert and the disarm regression. `outlastTheWatchdog` +now asks the page `timesArmed() > 0` first, which turns that review finding into +a failure by name. Note *ever armed* and not *armed now*: a correct page has +already disarmed by the time the disconnect test jumps — that is the property — +so the obvious spelling would have denied the passing case. + +**And a constant a test must exceed has one home.** `HANDSHAKE_TIMEOUT_MS` lived +in `web/app.js` and again in the spec, under a comment saying a silent +disagreement "would pass for the wrong reason". It did: the spec at 1000 against +the page's 8000 made the suite green over the reverted routing, because the jump +landed inside the window. It is exported from `web/shell.js` and imported by +both, so the disagreement is unconstructible rather than discouraged. + Everything **between** those seams is `node --test web/tests`, and it may assume the boundary holds rather than re-establishing it: transport selection from `?ws=`, the two facts `#status` composes, the watchdog's arm and disarm, the diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 9aff23c3..cb394e24 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -534,16 +534,33 @@ injected at the *peer*, not in the page** — a mocked socket that answers the handshake with a frame the client cannot fold — so this exercises the real `on_frame` and the real catch. -#### "a peer that cannot be reached is named, and the watchdog stays out of it" +#### "a peer that cannot be reached is named, and not called a disconnect" improvise-hwpi: without an `error` listener, a connection that never opened left `#status` blank, and "the server is not running" read exactly like "still -connecting". The watchdog half is asserted on the same page load, and since -PR #61's follow-up it costs no wall clock at all: `page.clock` fakes the PAGE's -timers, so `runFor(10s)` advances the watchdog without the test sleeping. It holds for a reason -its own comment does not lead you to expect: `armWatchdog` runs only after -`transport.ready` resolves, which happens on `open` — so on a socket that never -opened the watchdog is never armed at all. +connecting". **This test no longer says anything about the watchdog, and the +paragraph below is why it never could.** `shell.arm` runs only after +`await transport.ready`, which resolves on `open` — so on a socket that never +opens the watchdog is **never armed at all**, and denying its message here +denies an unreachable event. + +**That sentence was already in this file, five lines above an assertion that +contradicted it.** PR #65 added the watchdog clause *and edited this very +paragraph*, inserting "`runFor(10s)` advances the watchdog" immediately before +the sentence saying it is never armed. Reverting PR #21's routing, or stripping +`fail()` of its `sawFrame()`, both left that clause **passing**. So a PR whose +subject was "a deleted guard went unnoticed" planted the same shape one level +up: three injections, three distinct test names, and two of them proving +nothing. What the test guards now is the **naming** — `close` on a socket that +never opened must not read as "disconnected" (improvise-taxx) — which is the +half injection 3 really does exercise. + +**The mechanism, so it cannot come back.** `outlastTheWatchdog` now asks the +page `timesArmed() > 0` before jumping, and a watchdog denial re-added to this +path **fails by name**. It asks *ever armed* rather than *armed now*: by the +time the disconnect test jumps, a correct page has already disarmed — that is +the property — so `armed()` would have denied the passing case. Building that +guard is what surfaced the distinction, which no amount of reading had. #### "a socket that opened and then dropped reads as disconnected, and stays that way" @@ -589,6 +606,16 @@ watchdog never given the chance — vacuous, and green. `outlastTheWatchdog` rea 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. +**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 +the spec — with a comment saying a silent disagreement "would pass for the +wrong reason" and nothing checking it. Setting the spec's copy to 1000 against +the page's 8000 made the whole suite green **against the reverted routing**: +the jump landed inside the window, the watchdog had not yet fired, and the +denial held vacuously. It is now exported from `web/shell.js` and imported by +both, so the two cannot disagree; shrinking it in its one home still catches +the routing revert, which is the demonstration that the guard did not go quiet. + **The faked clock is what makes the restoration cheap without weakening it**, and the injections are the proof of that too: if `page.clock.runFor` did not really advance the page's timers, a broken routing could not surface the diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index 4f27c2f7..f494c340 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -4,6 +4,8 @@ import { test, expect } from "@playwright/test"; +import { HANDSHAKE_TIMEOUT_MS } from "../../web/shell.js"; + const status = (page) => page.locator("#status"); /// A peer that accepts the TCP connection and then refuses the WebSocket @@ -11,9 +13,9 @@ const status = (page) => page.locator("#status"); /// the harness's OWN server, so it follows the port the config chose. const NO_WEBSOCKET_HERE = `127.0.0.1:${process.env.IMPROVISE_HARNESS_PORT ?? "8099"}`; -/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Three tests have to outlast it, and -/// a number that silently disagreed with the page would pass for the wrong reason. -const HANDSHAKE_TIMEOUT_MS = 8_000; +/// The page's own constant, imported rather than copied: a spec that +/// understated it would jump short of the window and every watchdog assertion +/// below would pass for the wrong reason — demonstrated, PR #65 review. /// Record every distinct #status the page shows, not just the last one: the /// properties below are about a message being REPLACED, which a final read @@ -31,6 +33,14 @@ const json = (v) => JSON.stringify(v); /// Walk past the watchdog without waiting for it. `page.clock` fakes the /// page's own timers, so the 8s is the PAGE's and the wall clock is not. async function outlastTheWatchdog(page) { + // A negative assertion about the watchdog is vacuous where the watchdog was + // never armed, and `shell.arm` runs only after `await transport.ready`. It + // asks whether it was EVER armed: by here a correct page has disarmed it, + // which is the property, so `armed()` would deny the passing case. + expect( + await page.evaluate(() => window.__improviseShell?.timesArmed() ?? 0), + "the watchdog was never armed on this path: denying its message proves nothing", + ).toBeGreaterThan(0); const jump = HANDSHAKE_TIMEOUT_MS + 2_000; const before = await page.evaluate(() => Date.now()); await page.clock.runFor(jump); @@ -54,19 +64,19 @@ test.describe("the shell reports what its peer is doing", () => { await expect(status(page)).toContainText("bad frame", { timeout: 15_000 }); }); - // improvise-hwpi, plus the half `shell.test.js` cannot reach: that app.js - // ROUTES this arm through `shell.fail`, which disarms the watchdog. - test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({ + // improvise-hwpi. This peer NEVER OPENS, so `await transport.ready` never + // resolves and `shell.arm` never runs — the watchdog is not on this path. + test("a peer that cannot be reached is named, and not called a disconnect", async ({ page, }) => { - await page.clock.install(); await page.addInitScript(logStatus); await page.goto(`/app/index.html?ws=${NO_WEBSOCKET_HERE}`); await expect(status(page)).toContainText("cannot reach", { timeout: 15_000 }); - await outlastTheWatchdog(page); + // The naming is the whole property here: `close` on a socket that never + // opened must not read as "disconnected" (improvise-taxx). const log = await page.evaluate(() => window.__statusLog); - expect(log.some((t) => t.includes("sent no document")), `#status said: ${json(log)}`).toBe( + expect(log.some((t) => t.includes("disconnected")), `#status said: ${json(log)}`).toBe( false, ); }); diff --git a/web/app.js b/web/app.js index c1fb2f15..5b1d14da 100644 --- a/web/app.js +++ b/web/app.js @@ -4,6 +4,7 @@ import init, { Client } from "./pkg/improvise_wasm_client.js"; import { COL_PX, + HANDSHAKE_TIMEOUT_MS, ROW_PX, createShell, extentOf, @@ -40,13 +41,15 @@ const client = new Client(); // A peer that never answers the handshake, and the two facts `#status` shows. // Both are decided in shell.js; this is the wire to the element. -const HANDSHAKE_TIMEOUT_MS = 8000; const shell = createShell({ onStatus: (text) => { el.status.textContent = text; }, timeoutMs: HANDSHAKE_TIMEOUT_MS, }); +// The harness asks whether the watchdog is ARMED, because a negative assertion +// about it is vacuous where it is not (clients.md). Read by tests, nothing else. +window.__improviseShell = shell; // What the grid's own frame costs, in whole cells, as of the last repaint — // measured off the painted model, never predicted. context/code/clients.md. diff --git a/web/shell.js b/web/shell.js index ced087f1..3bbf7508 100644 --- a/web/shell.js +++ b/web/shell.js @@ -27,6 +27,11 @@ export function indicator(collapsed) { export const ROW_PX = 26; export const COL_PX = 110; +/** How long a peer has to answer the handshake. Exported because the harness + * must jump *past* it: a spec with its own copy can understate it silently + * and every watchdog assertion then passes vacuously (clients.md). */ +export const HANDSHAKE_TIMEOUT_MS = 8000; + /** The extent a box of this size can show, in whole DATA cells: the frame is * charged for first, and at least one cell is always asked for. */ export function extentOf(box, frameCost, metrics = { rowPx: ROW_PX, colPx: COL_PX }) { @@ -107,6 +112,7 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT let shell = ""; let model = ""; let watchdog = null; + let arms = 0; const paint = () => onStatus(composeStatus(shell, model)); return { @@ -121,6 +127,7 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT /** A peer that never answers the handshake. Deliberately about the *first * frame*, not either peer's own failure modes — clients.md. */ arm(message) { + arms += 1; clearTimer(watchdog); watchdog = setTimer(() => { watchdog = null; @@ -140,5 +147,8 @@ export function createShell({ onStatus, timeoutMs, setTimer = setTimeout, clearT this.setShell(text); }, armed: () => watchdog !== null, + /** 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. */ + timesArmed: () => arms, }; }