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

test(harness): drop two expected-failures whose trigger is platform-dependent

Browse Source 
An expected-failure is green when the bug is present and red when it is
absent. One whose trigger depends on the host therefore does not merely
flake — it **inverts CI's meaning**. These two failed the browser job twice
with "expected to fail, but passed", and the second time was *after* the
determinism work in the previous commit, which is the evidence that the
problem is not the assertions' precision.

Neither bug reproduces on the GitHub runner:

- improvise-m18f, where the unconditional `close` listener overwrites
  "cannot reach …" with "disconnected" — on the runner `close` does not
  follow `error` inside the window, even against a server that accepts the
  TCP connection and then refuses the upgrade.
- The handshake watchdog replacing a mid-session "disconnected" with
  "accepted the connection but sent no document" — on the runner it does not
  land, though the page demonstrably opened and then dropped, since both are
  asserted as setup.

The trigger for both is the **order and timing of `error`, `close` and a
timer on the host's network stack**. That is not a property of `web/app.js`
and not something this harness can pin, so pretending otherwise was the
mistake. Both are real, both were measured on macOS/Chromium, and the
measurement is recorded in the file with the exact status sequences so the
next person does not re-derive it — including the fact that the watchdog one
**survives** the m18f fix, because that fix is about which message `close`
writes rather than about the timer still running.

What remains is the portable half, and it is the half that was doing the
work: every message the page must write ("bad frame", "cannot reach",
"connected"/"disconnected", "sent no document"), the one case where the
watchdog must stay silent, and the two standalone-deployment tests. Filed
for the orchestrator; `web/app.js` is not this unit's file.

12 passing, 3 skipped (WebKit has no OPFS), 23s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 23:38:09 -07:00
parent affb9a1812
commit 4abadb8e6d
1 changed files with 34 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
harness/tests/shell.spec.js
+34 -70
View File
@@ -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