fix(harness): make a vacuous watchdog denial fail, and single-home the window
PR #65's review found this PR had planted a fresh instance of the defect it exists to repair. Both blocking findings fixed, each demonstrated. FINDING 1. "a peer that cannot be reached is named, and the watchdog stays out of it" could not see the property its comment claimed. `shell.arm` runs only after `await transport.ready`, which never resolves on a socket that never opens — so the watchdog is never armed there and denying "sent no document" denies an unreachable event. Reverting PR #21's routing left that clause passing. The aggravating half: `context/test-evidence/clients.md` already said so, and this PR EDITED that paragraph — inserting "runFor(10s) advances the watchdog" immediately above the sentence explaining it is never armed. Not text the PR left alone; text the PR walked past mid-sentence. The clause is gone and the test keeps the naming property it really guards: a `close` on a socket that never opened must not read as "disconnected". And the shape cannot return — `outlastTheWatchdog` now asks the page `timesArmed() > 0` before jumping, so a watchdog denial re-added to that path FAILS BY NAME. It asks *ever armed* rather than *armed now* on purpose: by the time the disconnect test jumps, a correct page has already disarmed, which is the property, so the obvious spelling would have denied the passing case. Building the guard is what surfaced that distinction; reading had not. FINDING 2. The spec kept its own `HANDSHAKE_TIMEOUT_MS` beside 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. The constant is now exported from `web/shell.js` and imported by both, so the disagreement is unconstructible. Shrinking it in its one home still catches the routing revert — that is the demonstration the guard did not go quiet. Filed, not fixed: improvise-78v6 (#61 also deleted the help-overlay spec and nothing replaced it — a PR about a dropped guard that did not diff the dropping PR's test names) and improvise-5ray (--check-evidence collects only `### ` and resolves only Rust fns, so every browser entry's `####` is invisible; appending one that names nothing exits 0, which is how the dangling heading cleared the gate for a release). Verified on the merged tree: node 26/26, bundle 3/3, Playwright 17 passed + 3 skipped, cargo test --workspace 1378 passed, comment-runs clean, --check-evidence 501 resolving, all cited ids resolve, no .beads diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
);
|
||||
});
|
||||
|
||||
+4
-1
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user