From 82c40e009d1b785f715d98dfde4fe7efb15509da Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 9 Aug 2026 16:26:26 +0000 Subject: [PATCH] test: delete the unreachable seal() hook and guard non-object RPC bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, no redesign. seal() was installed after session.close(), so onLate could never fire: once the context is destroyed no route handler and no console listener exists to record anything. It was dead code that the attribution table, run.js and harness.js all described as the final safety net. Deleted rather than moved — take() already drains everything the collector holds before teardown, so it covered nothing, and this harness must not ship a mechanism it cannot demonstrate. The now-unreachable `late > 0` term in the failure condition goes with it. handleRpc() parsed `postData || "null"` and then dereferenced the result, so a POST whose body Playwright reports as null — a bodyless request, or any payload it cannot decode as UTF-8, e.g. sendBeacon with a Blob — threw a TypeError inside the route handler and killed node mid-suite: truncated TAP, no summary, no failure line. Non-object payloads now take the same path as unparseable ones and are reported as unstubbed traffic, which is the entire point of that branch. README claimed unrecognised outbound requests are reported as failures, unqualified, while observation in fact ends TRAILING_WATCH_MS after the last test returns. The limit is now stated where it lands. --- README.md | 10 ++++++++++ tests/e2e/harness.js | 25 +++++++------------------ tests/e2e/network.js | 17 +++++++++++++++++ tests/e2e/run.js | 21 ++++++++------------- 4 files changed, 42 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index 4d75b97..0e8756d 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,16 @@ intercepted at the browser level and served from fixtures in unrecognised outbound requests are reported as failures rather than silently allowed. +That reporting has one bound worth knowing. Observation ends when the browser +context is torn down, and nothing can watch traffic after that, so the run keeps +collecting for a fixed grace period after the last test returns +(`TRAILING_WATCH_MS` in `tests/e2e/run.js`, currently 1500ms) and then closes +the context. A request whose _first_ dispatch falls after that window is never +seen at all and cannot fail the run. In practice a request a test fires without +awaiting reaches the route handler about 10ms later, and anything on a repeating +timer gets observed on an earlier tick during the ~20s suite — but a one-shot +call deliberately deferred past the window will escape. + That interception covers the MV3 background service worker as well as the popup page, which it does not by default — `script/test-e2e` sets `PW_EXPERIMENTAL_SERVICE_WORKER_NETWORK_EVENTS=1` for it. Because that flag is diff --git a/tests/e2e/harness.js b/tests/e2e/harness.js index 0a55a97..f51fe78 100644 --- a/tests/e2e/harness.js +++ b/tests/e2e/harness.js @@ -52,28 +52,22 @@ function isAllowed(text) { // // So there is no window left to fall outside of. take() is the only // reader and it always takes everything outstanding, so successive takes -// partition the entire record stream with no gaps. seal() closes the -// stream once the run is over and routes anything later straight to a -// callback rather than into a list nobody reads again. Attribution is -// therefore total by construction, and the runner turns every attributed -// record into a failure. +// partition the entire record stream with no gaps, and the runner turns +// every record it reads into a failure. +// +// Observation ends when the browser context is closed. Nothing records +// after that — the route handler and the console listeners are gone with +// the context — so there is no post-teardown phase to collect, and this +// class deliberately offers no mechanism pretending to cover one. class ErrorCollector { constructor() { this.entries = []; this.taken = 0; - this.onLate = null; } record(kind, text) { const line = kind + ": " + String(text).split("\n")[0]; if (isAllowed(line)) return; - if (this.onLate) { - // Sealed: no test and no suite phase is left to attribute - // this to, so hand it over now instead of accumulating it - // where nothing will look. - this.onLate(line); - return; - } this.entries.push(line); } @@ -84,11 +78,6 @@ class ErrorCollector { this.taken = this.entries.length; return out; } - - // Close the stream: later records go to onLate instead of the list. - seal(onLate) { - this.onLate = onLate; - } } function attachErrorListeners(ctx, errors) { diff --git a/tests/e2e/network.js b/tests/e2e/network.js index 413a45b..034e940 100644 --- a/tests/e2e/network.js +++ b/tests/e2e/network.js @@ -131,6 +131,23 @@ function handleRpc(route, postData, report) { } // ethers batches by default, so the body may be an array. const batch = Array.isArray(payload) ? payload : [payload]; + + // Anything that is not a JSON-RPC object, or a batch of them, is not + // RPC at all and must be reported like any other unrecognised + // outbound traffic rather than dereferenced. request.postData() + // returns null both for a bodyless POST and for a body Playwright + // cannot decode as UTF-8 (sendBeacon with a Blob, or any binary + // payload), so this is not an empty-string special case: it rejects + // every non-object payload, exactly as the catch above rejects every + // unparseable one. + if ( + payload === null || + typeof payload !== "object" || + !batch.every((req) => req !== null && typeof req === "object") + ) { + report("unstubbed request: POST " + route.request().url()); + return route.abort(); + } const replies = batch.map((req) => { const result = RPC_RESULTS[req.method]; if (result === undefined) { diff --git a/tests/e2e/run.js b/tests/e2e/run.js index f20f0a9..a771e3c 100644 --- a/tests/e2e/run.js +++ b/tests/e2e/run.js @@ -167,7 +167,13 @@ async function main() { // nowhere) // end of test k .. end of k+1 -> test k+1 // last test .. teardown -> the suite, via the trailing drain - // after the trailing drain -> seal(), which fails on the spot + // + // Those three phases cover the entire life of the browser context. + // There is no fourth: once the context is closed nothing can record, + // because the route handler and the console listeners died with it. + // Traffic that a test defers past the trailing drain is therefore + // never observed at all — a real limit of this design, stated in the + // README, and not one any post-teardown hook could close. // // Two green-but-vacuous runs on this harness were the same shape: a // record falling outside somebody's window and being dropped. First @@ -230,17 +236,6 @@ async function main() { // reported, and they fail the run. const trailing = session.errors.take(); - // From here the run is over and there is nothing left to attribute a - // record to, so stragglers fail immediately instead of piling up - // where nothing will read them. - let late = 0; - session.errors.seal((line) => { - late += 1; - console.log("# FAILED: browser error recorded after the run ended"); - console.log("# " + line); - process.exitCode = 1; - }); - console.log( "# " + (tests.length - failed) + "/" + tests.length + " tests passed", ); @@ -257,7 +252,7 @@ async function main() { } } - if (failed > 0 || trailing.length > 0 || late > 0) { + if (failed > 0 || trailing.length > 0) { console.log("# FAILED"); process.exitCode = 1; }