fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

test(harness): the four assertions the close-listener fix owes, and a third bug

Browse Source 
The harness is the one place a line of `web/*.js` is executed, not only the
one place `storage_contract` reaches `OpfsFs`. PR #21 declined Copilot's
request for regression tests on its close-listener fix because there was no
JavaScript test infrastructure in this repository at all — no
`package.json`, no runner, nothing in CI running a line of `web/app.js` — and
standing up a parallel one beside this harness would have been two answers
to one question. That decline is only honest if the assertions land here, so
here they are, and `playwright.config.js` now says out loud that a change to
`web/*.js` belongs in this spec directory. Playwright rather than a
headless-DOM runner, because what goes wrong in these files is a real
Worker, a real WebSocket, real OPFS and real event ordering — none of which
a DOM shim has.

`close` fires on a connection that never opened and on one that was
established and dropped, and nothing on the event tells them apart:
`readyState` is already CLOSED in any close listener, and both arrive as
1006 with `wasClean: false`. So:

1. **close-before-open must leave "cannot reach" standing.** `test.fail`,
   because the close handler on this branch is unconditional and overwrites
   it (improvise-m18f). That IS the regression guard, and it is aimed at the
   implementation actually here rather than at a mutant invented for the
   occasion — against an unconditional handler, "always say cannot reach"
   would pass and prove nothing. Verified by simulating PR #21's `opened`
   flag in the served copy: the test passes and Playwright reports "expected
   to fail, but passed", so the fix cannot land silently either.
2. **open-then-close must read "disconnected".** Green under both
   implementations — verified under the simulated fix too, since this is the
   arm that makes suppressing `close` wrong.
3. **The watchdog must stay out of the never-opened path.** Green, but 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. Asserted on
   the same page load as the "cannot reach" check, since both need the same
   9-second wait.
4. **The watchdog must not overwrite a mid-session disconnect.** It does.
   Found by measuring rather than predicted, and it is a **third** bug: on
   the open-then-close arm the watchdog is armed, nothing disarms it, and 8s
   later "disconnected" is replaced by "accepted the connection but sent no
   document" — which is false, the peer connected and then went away. Same
   class as the bug `setShellFailure` fixed (a watchdog must be disarmed,
   not pre-empted) and it **survives** improvise-m18f's fix, which is about
   which message `close` writes rather than about the timer still running —
   confirmed under the simulated fix, where this one still fails. `test.fail`
   and filed; `web/app.js` is not this unit's file.

Measured before asserting, which is how 3 and 4 came out opposite to the
expectation they were handed:

    never-opened:     connecting… -> cannot reach -> disconnected
    open-then-close:  connecting… -> connected -> disconnected
                      -> accepted the connection but sent no document

11 passing in the harness, 32s. The two `test.fail` entries are expected
failures, so the suite is green and both bugs are pinned in a form that goes
red the moment either is fixed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 23:14:16 -07:00
parent fb507b9855
commit f90d1757be
2 changed files with 115 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
harness/playwright.config.js
+18
View File
@@ -5,6 +5,24 @@ import { defineConfig, devices } from "@playwright/test";
// `OpfsFs` and to read `#status` out of a real page, not to grow a second
// test framework beside `cargo test`.
//
// # This is the one place a line of web/*.js is executed
//
// It is not only the OPFS harness. Before this there was no JavaScript test
// infrastructure in the repository at all — no package.json, no runner,
// nothing in CI running a single line of `web/app.js` or `web/worker.js` — so
// every bug in them was found by reading (gotcha 48) and the bug-first rule
// could not be honoured for a change to either. PR #21 declined to add
// regression tests for its close-listener fix on exactly that ground, and
// that decline is only honest because those assertions live here instead:
// see `tests/shell.spec.js`.
//
// So a change to `web/*.js` belongs in this spec directory, not in a second
// ad-hoc surface beside it — two answers to one question is the defect this
// codebase keeps curing. Playwright is the right vehicle rather than a
// headless-DOM runner (jsdom and friends) because the things that go wrong in
// these files are a real Worker, a real WebSocket, real OPFS and real event
// ordering, none of which a DOM shim has.
//
// The server root is `harness/serve`, built by `build.sh` — the assembled
// site plus this harness's own page. A module script will not load from
// `file://`, so an http server is not optional.
harness/tests/shell.spec.js
+97 -22
View File
@@ -11,6 +11,23 @@
import { test, expect } from "@playwright/test";
const status = (page) => page.locator("#status");
const json = (v) => JSON.stringify(v);
/// `web/app.js`'s own HANDSHAKE_TIMEOUT_MS. Named here because two tests have
/// to outlast it, and a number that silently disagrees with the page would
/// make them pass for the wrong reason.
const HANDSHAKE_TIMEOUT_MS = 8_000;
/// Record every distinct #status the page shows, not just the last one.
/// Several of these are about a message being REPLACED, which the final value
/// cannot see.
function logStatus() {
window.__statusLog = [];
new MutationObserver(() => {
const text = document.getElementById("status")?.textContent ?? "";
if (text && window.__statusLog.at(-1) !== text) window.__statusLog.push(text);
}).observe(document, { childList: true, subtree: true, characterData: true });
}
test.describe("the shell reports what its peer is doing", () => {
// improvise-hwpi: `on_frame` returns a Result, which wasm-bindgen throws.
@@ -32,31 +49,89 @@ test.describe("the shell reports what its peer is doing", () => {
// opened left #status blank, and "the server is not running" read exactly
// like "still connecting". Port 9 (discard) refuses immediately.
//
// It is asserted over the *history* of #status rather than its final value,
// because of improvise-m18f, which this reproduces: the socket's `close`
// listener fires after `error` and overwrites the precise message with
// "disconnected". That one is filed and deliberately not fixed here —
// suppressing `close` could hide a genuine mid-session disconnect, so it is
// not the same fix as the watchdog's (gotcha 48). Asserting both halves
// pins hwpi's fix AND m18f's bug, so whichever moves, this speaks up.
test("a peer that cannot be reached is named, not left blank", async ({ page }) => {
await page.addInitScript(() => {
window.__statusLog = [];
new MutationObserver(() => {
const el = document.getElementById("status");
const text = el?.textContent ?? "";
if (text && window.__statusLog.at(-1) !== text) window.__statusLog.push(text);
}).observe(document, { childList: true, subtree: true, characterData: true });
});
// The watchdog half is asserted on the same page load rather than in its
// own test, because it needs the same 9-second wait. 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.
test("a peer that cannot be reached is named, and the watchdog stays out of it", async ({
page,
}) => {
await page.addInitScript(logStatus);
await page.goto("/index.html?ws=127.0.0.1:9");
await expect(status(page)).toContainText("disconnected", { timeout: 15_000 });
await expect(status(page)).not.toHaveText("", { timeout: 15_000 });
await page.waitForTimeout(HANDSHAKE_TIMEOUT_MS + 1_000);
const log = await page.evaluate(() => window.__statusLog);
expect(log.some((t) => t.includes("cannot reach")), `#status said: ${JSON.stringify(log)}`)
.toBe(true);
// improvise-m18f, as it stands today. Delete this line when it is fixed —
// it is here so the fix cannot land silently.
expect(log.at(-1)).toContain("disconnected");
expect(log.some((t) => t.includes("cannot reach")), `#status said: ${json(log)}`).toBe(
true,
);
expect(log.some((t) => t.includes("sent no document")), `#status said: ${json(log)}`).toBe(
false,
);
});
// improvise-m18f, and the regression guard for the fix in PR #21.
//
// `close` fires on both a connection that never opened and one that was
// established and dropped, and NOTHING on the event tells them apart:
// `readyState` is already CLOSED in any close listener, and both arrive as
// code 1006 with `wasClean: false`. Distinguishing them needs a flag set in
// the `open` handler.
//
// `test.fail` is the injected fault, aimed at the implementation actually
// on this branch rather than at a mutant invented for the occasion: the
// close handler is unconditional here, so it overwrites the precise
// "cannot reach" with "disconnected" and this assertion FAILS. That is what
// makes it a guard — against the unconditional handler, "always say cannot
// reach" would pass and prove nothing. When the `opened` flag lands, this
// starts passing, Playwright reports "expected to fail but passed", and the
// `test.fail()` line has to be deleted. It cannot land silently either way.
test("close before open leaves the precise message standing", async ({ page }) => {
test.fail(true, "improvise-m18f: the close handler is unconditional on this branch");
await page.goto("/index.html?ws=127.0.0.1:9");
// Wait for the status to settle on SOMETHING rather than for a particular
// message: waiting for "cannot reach" would retry against the value the
// page has already replaced, and waiting for "disconnected" would time
// out for the wrong reason once the fix lands.
await expect(status(page)).not.toHaveText("", { timeout: 15_000 });
await page.waitForTimeout(1_000);
await expect(status(page)).toContainText("cannot reach", { timeout: 1_000 });
});
// The other arm, which must keep working when the first one is fixed: a
// socket that really did open and then dropped IS a disconnect, and saying
// so is the whole reason `close` cannot simply be suppressed.
test("a socket that opened and then dropped reads as disconnected", async ({ page }) => {
await page.routeWebSocket("ws://127.0.0.1:8790/", (ws) => {
setTimeout(() => ws.close(), 300);
});
await page.goto("/index.html?ws=127.0.0.1:8790");
await expect(status(page)).toContainText("connected", { timeout: 15_000 });
await expect(status(page)).toContainText("disconnected", { timeout: 15_000 });
});
// A THIRD bug, found by measuring rather than predicted: on the
// open-then-close arm the watchdog IS armed (it is armed after
// `transport.ready`, which resolved on `open`), nothing disarms it, and 8
// seconds later it replaces "disconnected" with "accepted the connection
// but sent no document" — which is false; the peer did connect and then
// went away.
//
// It is the same class as the bug `setShellFailure` fixed (a watchdog must
// be disarmed, not pre-empted) and it survives the improvise-m18f fix,
// because that fix is about which message `close` writes, not about the
// timer still running. Filed rather than fixed here: `web/app.js` is not
// this unit's file.
test("the watchdog does not overwrite a mid-session disconnect", async ({ page }) => {
test.fail(true, "the close handler does not disarm the handshake watchdog");
await page.routeWebSocket("ws://127.0.0.1:8791/", (ws) => {
setTimeout(() => ws.close(), 300);
});
await page.goto("/index.html?ws=127.0.0.1:8791");
await expect(status(page)).toContainText("disconnected", { timeout: 15_000 });
await page.waitForTimeout(HANDSHAKE_TIMEOUT_MS + 1_000);
await expect(status(page)).toContainText("disconnected", { timeout: 1_000 });
});
// The first-frame watchdog (gotcha 46), found by reading. The condition is