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

test(harness): Firefox runs the contract too, and test.fail stops hiding setup

Browse Source 
Two Copilot findings on PR #24, both correct, both applied.

**`test.fail` was masking setup failures.** Applied before navigation, it
made any earlier failure — the page not loading, the script not starting,
`#status` never updating — an *expected* failure, so CI stayed green through
it. That is precisely the check-the-checker defect these tests exist to
catch, wearing the name of its cure. Both tests now assert their setup
first and apply `test.fail` immediately before the one assertion the known
bug owns. Verified by breaking the setup (making `paintStatus` a no-op):
before, 2 passed; after, **2 failed**.

**Firefox now runs the whole battery, and it passes.** improvise-xf1's
acceptance criterion names Chrome, Firefox and Safari, and the reason is
real rather than ceremonial: `createSyncAccessHandle` is the one API this
design rests on (gotcha 28) and each engine implemented it separately, so
Chromium alone was one engine's opinion. `storage_contract` against
`OpfsFs` now passes on **two** engines.

**Safari is not covered, and the criterion is met for two of three.**
Playwright's WebKit build provides no OPFS at all —
`navigator.storage.getDirectory()` itself rejects with `UnknownError` — so
there is nothing there for `OpfsFs` to be right or wrong about. Rather than
delete the project and quietly narrow the claim, it is kept and the spec
skips on a **runtime probe**: the day that build gains OPFS the checks start
running with no CI change. The skip is conditional on the probe and not on
the browser name, and an unexpected skip on any other engine **fails** with
"that is a regression, not a known gap" — a silently skipped check being the
failure this harness exists to prevent.

The shell spec stays Chromium-only on purpose: it is about `web/app.js`'s
own logic, which is engine-independent, and running `routeWebSocket` plus
the worker handshake three times is slower and flakier to no benefit.

14 passing, 3 skipped, 36s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Edward Langley
2026-09-19 23:23:44 -07:00
parent f90d1757be
commit 3c36a3b57f
4 changed files with 82 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/workflows/ci.yml
+7 -2
View File
@@ -120,9 +120,14 @@ jobs:
- name: Install the harness
working-directory: harness
run: npm ci
- name: Install Chromium
# All three, because improvise-xf1's acceptance criterion names Chrome,
# Firefox and Safari and `createSyncAccessHandle` is implemented
# separately by each engine. WebKit is installed even though its OPFS is
# absent today: the spec skips on a runtime probe, so the day that build
# gains OPFS the check starts running without a CI change.
- name: Install browsers
working-directory: harness
run: npx playwright install --with-deps chromium
run: npx playwright install --with-deps chromium firefox webkit
# Builds both site bundles, the harness's own wasm bundle, and assembles
# `harness/serve` — the same tree `pages-build` publishes, plus /opfs/.
- name: Build the harness
harness/playwright.config.js
+30 -1
View File
@@ -38,7 +38,36 @@ export default defineConfig({
baseURL: "http://127.0.0.1:8099",
trace: "retain-on-failure",
},
projects: [{ name: "chromium", use: { ...devices["Desktop Chrome"] } }],
// improvise-xf1's acceptance criterion names Chrome, Firefox and Safari,
// and the OPFS spec is the reason: `createSyncAccessHandle` is the one API
// this whole design rests on (gotcha 28), its sync read/write/truncate
// signatures were added later than the handle itself, and each engine
// implemented it separately. Chromium alone would be one engine's opinion.
//
// The shell spec is Chromium-only on purpose — it is about `web/app.js`'s
// own logic, which is engine-independent, and `routeWebSocket` plus the
// worker handshake are slower and flakier to no benefit across three.
//
// Result today: Chromium and Firefox run the whole battery and pass.
// **Playwright's WebKit provides no OPFS at all** — `getDirectory()` itself
// rejects with `UnknownError` — so Safari is NOT covered and xf1's
// criterion is met for two engines of three. The project is kept rather
// than deleted so that it starts running the day that build gains OPFS;
// `opfs.spec.js` skips on a runtime probe, and an unexpected skip on any
// other engine is a failure.
projects: [
{ name: "chromium", use: { ...devices["Desktop Chrome"] } },
{
name: "firefox",
use: { ...devices["Desktop Firefox"] },
testMatch: /opfs\.spec\.js/,
},
{
name: "webkit",
use: { ...devices["Desktop Safari"] },
testMatch: /opfs\.spec\.js/,
},
],
webServer: {
command: "python3 -m http.server 8099 --bind 127.0.0.1 --directory serve",
url: "http://127.0.0.1:8099/index.html",
harness/tests/opfs.spec.js
+29 -1
View File
@@ -32,8 +32,36 @@ const EXPECTED = [
];
test.describe("OpfsFs in a real browser", () => {
test.beforeEach(async ({ page }) => {
test.beforeEach(async ({ page, browserName }, testInfo) => {
await page.goto("/opfs/index.html");
// improvise-xf1's acceptance criterion names Chrome, Firefox and Safari.
// Chromium and Firefox run the whole battery. Playwright's WebKit build
// provides **no OPFS at all** — `navigator.storage.getDirectory()` itself
// rejects with `UnknownError` — so there is nothing there for `OpfsFs` to
// be right or wrong about, and Safari stays uncovered. See the hand-back
// and `playwright.config.js`.
//
// The skip is conditional on the PROBE, not on the browser name, so it
// cannot quietly swallow a real regression: if Chromium or Firefox ever
// stops providing OPFS, this fails rather than skipping. A silently
// skipped check is exactly the failure this harness exists to prevent, so
// an unexpected skip also fails.
const opfs = await page.evaluate(async () => {
try {
await navigator.storage.getDirectory();
return "yes";
} catch (err) {
return String(err?.name ?? err);
}
});
if (opfs !== "yes") {
expect(
browserName,
`${browserName} lost OPFS (${opfs}) — that is a regression, not a known gap`,
).toBe("webkit");
testInfo.skip(true, `this WebKit build provides no OPFS (${opfs})`);
}
});
// Check the checker, and do it first: a green run against a worker that
harness/tests/shell.spec.js
+16 -6
View File
@@ -88,14 +88,20 @@ test.describe("the shell reports what its peer is doing", () => {
// 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.
// 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). Waiting for the status to
// settle on SOMETHING rather than on a particular message is what makes
// this arm work under both implementations: "cannot reach" would retry
// against a value the page has already replaced, and "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);
// 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 });
});
@@ -124,13 +130,17 @@ test.describe("the shell reports what its peer is doing", () => {
// 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");
// Setup, asserted before `test.fail` for the reason above: a socket that
// never opened would otherwise make this pass as an expected failure
// while testing nothing.
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 });
});