diff --git a/harness/tests/shell.spec.js b/harness/tests/shell.spec.js index dcb4142f..96bf1cb6 100644 --- a/harness/tests/shell.spec.js +++ b/harness/tests/shell.spec.js @@ -80,45 +80,6 @@ test.describe("the shell reports what its peer is doing", () => { ); }); - // 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 }) => { - await page.addInitScript(logStatus); - await page.goto(`/index.html?ws=${NO_WEBSOCKET_HERE}`); - // Everything up to here is SETUP, and it is asserted BEFORE `test.fail` - // is applied — otherwise a page that failed to load, or a status that - // never updated at all, would also be an "expected" failure and CI would - // stay green through it (Copilot, PR #24). - // - // The setup asserted is the PRECONDITION of the bug, not merely that - // something happened: the socket must have failed to open (so "cannot - // reach" was written) and then closed (so something could overwrite it). - // Asserted over the recorded history, because the whole subject is a - // message being replaced, which the instantaneous value cannot see. - await expect - .poll(() => page.evaluate(() => window.__statusLog ?? []), { timeout: 15_000 }) - .toEqual(expect.arrayContaining([expect.stringContaining("cannot reach")])); - await page.waitForTimeout(1_000); - - // From here on the known bug owns the outcome. - test.fail(true, "improvise-m18f: the close handler is unconditional on this branch"); - 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. @@ -133,38 +94,41 @@ test.describe("the shell reports what its peer is doing", () => { 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. + // TWO BUGS THIS FILE DELIBERATELY DOES NOT ASSERT, and the reason is worth + // more than the assertions would have been. // - // 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 }) => { - // Close on the page's FIRST FRAME rather than on a timer. The page sends - // its viewport only after `transport.ready` resolves, which happens on - // `open`, so this makes "opened, then dropped" true by construction. A - // timer does not: on a slow host the close can land before the page sees - // `open`, in which case the watchdog is never armed and the test passes - // having exercised nothing — which is what happened in CI. - await page.routeWebSocket("ws://127.0.0.1:8791/", (ws) => { - ws.onMessage(() => ws.close()); - }); - await page.goto("/index.html?ws=127.0.0.1:8791"); - // Setup, asserted before `test.fail`: the socket really opened, and then - // really dropped. Both, in that order. - await expect(status(page)).toContainText("connected", { timeout: 15_000 }); - await expect(status(page)).toContainText("disconnected", { timeout: 15_000 }); - await page.waitForTimeout(HANDSHAKE_TIMEOUT_MS + 1_000); - - test.fail(true, "the close handler does not disarm the handshake watchdog"); - await expect(status(page)).toContainText("disconnected", { timeout: 1_000 }); - }); + // 1. improvise-m18f: the socket's `close` listener is unconditional, so on + // a connection that never opened it overwrites "cannot reach …" with + // "disconnected". + // 2. On a connection that DID open and then dropped, nothing disarms the + // handshake watchdog, so 8s later "disconnected" is replaced by + // "accepted the connection but sent no document" — which is false. Same + // class as the bug `setShellFailure` fixed (a watchdog must be disarmed, + // not pre-empted, gotcha 48), and it SURVIVES the m18f fix, because that + // fix is about which message `close` writes rather than about the timer + // still running. + // + // Both are real and both were measured, on macOS/Chromium: + // + // never-opened: connecting… -> cannot reach -> disconnected + // open-then-close: connecting… -> connected -> disconnected + // -> accepted the connection but sent no document + // + // They were asserted here as `test.fail` expected-failures and **had to be + // removed**: neither reproduces on the GitHub runner, where `close` does + // not follow `error` within the window and the watchdog does not land. An + // expected-failure is green when the bug is present and red when it is + // absent, so one whose trigger is platform-dependent does not merely flake + // — it **inverts CI's meaning**, and it failed the build twice saying + // "expected to fail, but passed". + // + // What that leaves is a genuine limit rather than an oversight: the trigger + // for both is the ORDER AND TIMING of `error`/`close`/timer on the host's + // network stack, which is not a property of `web/app.js` and not something + // a harness can pin. The four assertions that remain in this file are the + // portable half — every message the page must write, and the one case where + // the watchdog must stay silent. Filed for the orchestrator with the + // recipe above; fixing them is `web/app.js`, which is not this unit's file. // The first-frame watchdog (gotcha 46), found by reading. The condition is // one no per-transport check would catch: the peer accepted the connection