Refuse a generated spec that waits for the network to fall idle - #258
Conversation
Six of the browser-test timeouts on recent runs were one line:
await page.waitForLoadState("networkidle");
networkidle resolves only after 500ms with nothing in flight, and this app
opens a Server-Sent Events stream on mount and keeps it open by design. The
wait can never return, so the spec burns its whole 30s budget three times over
however right the rest of it is. Playwright discourages networkidle for exactly
this reason. The lint now refuses it and names what to write instead: an
assertion on a locator, which retries until the page settles.
Two things the review changed beyond the rule itself. The wait is matched by
pairing the call with its own argument in code position, because matching the
string and the call independently fired on a type alias, a mocked API body and
a loop over fixture names — and a spec rejected for something it does not do
spends the one repair it gets. And the certain rules now run on every answer
rather than only the first: a repair that fixed some other complaint used to
ship the dead wait anyway, which is how a nudge silently becomes nothing.
A hard sleep is refused alongside it, as a nudge rather than a certainty: it is
the substitute a model reaches for once it cannot wait on the network.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
DevAsign Code Review
No issues found
✅ Merge score: 100/100
8 of 8 acceptance criteria met.
The change splits the never-returning waits (networkidle load-state and navigation waitUntil, plus hard sleeps) out of the taste-based specLint into a code-position matcher, with specLintCertain re-run on every answer.
Tests: 8 passed, 0 unverifiable — see the "Tests by DevAsign" comment.
Tests by DevAsign✅ 8 of 8 criteria verified by tests. Each verdict below links to its evidence. 1 — A generated spec containing a call that waits for the 'networkidle' load state (e.g. page.waitForLoadState("networkidle")) is refused by the lint. (pass)Verdict: pass Spec with page.waitForLoadState('networkidle') is refused with exactly one problem, and the same spec without that line passes clean. Test: 2 — The refusal message for a networkidle wait names the replacement: an assertion on a locator that retries until the page settles. (pass)Verdict: pass The networkidle refusal message names the retrying assertion as the replacement, not a fixed sleep. Test: 3 — The networkidle wait is also caught in its formatter-wrapped form, when written with single quotes, and in the goto({ waitUntil: 'networkidle' }) spelling. (pass)Verdict: pass Formatter-wrapped, single-quoted, and goto({ waitUntil: 'networkidle' }) forms are each flagged once. Test: 4 — The matcher does not flag lines that merely mention 'networkidle' or a wait independently, such as a type alias, a mocked API response body, or a loop over fixture names; the call and its argument must be paired in code position. (pass)Verdict: pass Type aliases, mocked response bodies, comments, and fixture-name loops mentioning networkidle are not flagged; call and argument must be paired. Test: 5 — The rule that refuses waits which can never return runs on every answer, not only the first answer. (pass)Verdict: pass A networkidle wait is flagged on the first answer and again on the repair answer, confirming the certain rule runs on every answer. Test: 6 — The taste-based lint rules retain their first-answer-only behaviour. (pass)Verdict: pass Taste-based nudges appear in specLint but are absent from specLintCertain's always-run output. Test: 7 — A hard sleep (waitForTimeout, or a hand-rolled setTimeout promise) is refused as a nudge rather than a certainty. (pass)Verdict: pass Both waitForTimeout and a hand-rolled setTimeout promise are refused as nudges with a suggested retrying assertion, and omitted from specLintCertain. Test: 8 — The lint rules apply to a spec regardless of whether it imports from '@playwright/test'; a CommonJS spec is gated on the runner and does not skip the rules. (pass)Verdict: pass A CommonJS spec under a playwright runner is flagged; under a non-playwright runner it is not, confirming gating by runner not import style. Test: |
Step 1 of the e2e diagnosis. Six of the browser-test timeouts on recent runs come down to one line.
The bug
And the line immediately above it in the same log:
The boot is fine.
networkidleresolves only after ~500 ms with zero connections in flight, and the app opens a Server-Sent Events stream on mount (live-context.tsx:46, served astext/event-streamat api.ts:2245) and holds it open by design. The condition can never be met, so the spec burns its full 30 s, three attempts, however correct the rest of it is. Playwright marksnetworkidleDISCOURAGED for exactly this reason.The lint now refuses it and names the replacement — an assertion on a locator, which retries until the page settles.
Two things review changed beyond the rule
The matcher pairs the call with its own argument. The first version tested "is
networkidleanywhere on this line" and "is there a wait call anywhere on this line" independently, which fired on a type alias, a mocked API response body, and a loop over fixture names. That matters more than a normal false positive: a spec gets one repair pass, so rejecting it for something it doesn't do can cost the criterion its only test.The certain rules now run on every answer, not just the first.
specLintwas first-answer-only by design (a pattern check is a nudge, and a spec that insists may be right). But a repair that fixed some other complaint would then ship the dead wait anyway — the nudge silently becoming nothing. Waits that can never return are now split intospecLintCertain, which runs on each answer; the taste-based rules keep their first-answer-only behaviour.Also: the
from '@playwright/test'gate was letting a CommonJS spec skip every rule in the file, and the rule is now gated on the runner instead.A hard sleep (
waitForTimeout, and the hand-rolledsetTimeoutpromise) is refused alongside — as a nudge, not a certainty, since it flakes rather than always failing. It's the substitute a model reaches for once it can't wait on the network.Verification
Backend 1751 pass, frontend 270, CLI 125, typecheck clean. 15 review findings, all confirmed and all applied.
I checked the rule myself rather than take the report — against the incident's exact line, the formatter-wrapped form, single quotes, and the
goto({ waitUntil })spelling (all caught), and against every false-positive shape the reviewer constructed (all clean).What this does not fix
The other 12 timeouts are separate and diagnosed: 9 specs wait for Workflow-header chrome from a page that doesn't have it (
/redirects to/agent, where.wf-repo-btnand the Verification button don't exist), and 3 look for app state that isn't there (acme/widgets, "Select repository" — the booted app seedsephemeral-tester/demoand already has it selected). I proved both by booting the app locally and driving it with Playwright.The already-committed
boot-check-reason.spec.tson this repo still holds the bad line and will keep timing out until it is regenerated.🤖 Generated with Claude Code