qa: fixtures, screenshot diffing, and a CI job that can actually fail - #2819
qa: fixtures, screenshot diffing, and a CI job that can actually fail#2819kushagrasarathe wants to merge 13 commits into
Conversation
…no backend A screen in a specific state was, until now, something you reproduced: seed a user, move the balance, wait for a provider, and hope the state survived until the screenshot. The states that break layouts — a 17-character username, a nine-digit points total, an empty list, a rejected document — are exactly the ones nobody reproduces twice. `<route>?__fixture=<name>` answers every API call from a named app state. The same URL works in a browser, in the /dev/devices harness at six widths, on a preview build, and under Playwright later. 38 states: one per screen, plus the text, number, empty, KYC and error cases that carry the layout risk. The baseline is the demo API, not a new mock layer. demo-api already answers every route the app calls and already sits on this exact line in callApi, so a fixture states only what differs and most are one to three lines. Four screens needed more than API answers. The kernel address, the balance and the charges socket are not responses, so a fixture screen waited forever on state no API can give it. Each extends the isDemoMode() branch that already exists rather than adding a second concept. Everything hangs off DEV_TOOLS_ENABLED. Verified on a production build served with next start: /profile?__fixture=… redirects to /setup, no cookie is written, no session key is set, nothing renders. /dev/fixtures answers 404. Known and deliberate: the registry still reaches production chunks (~35 KB of fixture names and English descriptions — no credentials, no user data). Webpack emits a chunk for every import() in the source, so neither the gate nor a lazy import removes it. /dev/devices, /dev/ds and /dev/debug already ship the same way. Dropping them needs a build rule over every /dev route, which is a repo-wide decision and not this change.
Verifying a design-system change meant building the base branch, taking screenshots by hand, building the head branch, taking them again, and pairing them into a table. PR #2758 shipped 25 of those, at one width, and nobody could tell from the table which screens had actually moved. `npm run test:visual` builds and captures every fixture at 320, 375, 393 and 430. 320 is an iPhone on Larger Text and finds the most bugs; 430 is a Pro Max. `test:visual:diff` pairs two capture directories and prints what changed, worst first. 152 screens, about 70 seconds after the build. The registry is the only route list. Adding a fixture adds four screenshots; there is no second list to forget. A diff tool that cries wolf gets ignored in a week, so the run pins everything that moves: a frozen clock, no cross-origin requests, no service worker, CSS animation and transitions off, fonts and images awaited, and a wait for two equal text samples because the points total counts up from JavaScript and ignores the stylesheet. Two full runs of the same build: 0 changed, 152 unchanged. A deliberate margin change on one heading: exactly the three /limits fixtures at all four widths, nothing else. The run fails rather than shoots when a loader is on camera. A green run of 152 spinners looks exactly like a green run of 152 screens, and that mistake has been made in this repo before. No API, no database, no provider keys, no harness secret — the fixtures answer every call, so this can run anywhere a build runs.
A fixture that says one thing and renders another is worse than no fixture. It becomes a screenshot baseline, and every later diff compares against a screen nobody looked at. Reading all 152 screenshots found eight lying and five unreachable. `kyc-action-required` promised a rejected document and rendered four green-ticked regions. `error-user` promised the backend error screen and rendered an unsupported-browser modal. `huge-limits` promised nine-figure caps that sit one screen deeper than the route it pointed at. `withdraw` promised two saved accounts and rendered "No accounts yet". Most of these overrode an endpoint the screen never calls. The withdraw list comes from `user.accounts`, not `GET /users/accounts`. The region lists come from capabilities rails, not from `identityVerification.status`. The activity list injects a row per badge and one for the ID check, so clearing `entries` alone still leaves four rows. Five are deleted rather than faked: their states need a tap the camera cannot make, or a query string the route field cannot carry. 33 fixtures, all verified by eye at 375 and at 320 for the overflow cases. Two captures of the same build still differ by nothing. Found and left alone, both worth their own change: `BackendErrorScreen` is unreachable, because the redirect effect in `(mobile-ui)/layout.tsx` fires on the same state without a `userFetchError` guard — a backend outage sends signed-in users to the signup flow. And the "Pending" region badge on /limits can never render, because `hasFunctionalRail` counts a pending rail as unlocked before the badge is ever considered.
The `e2e` job has reported success without running a test since 2026-04-27. It dies in globalSetup on a missing TEST_HARNESS_SECRET, and the continue-on-error added that day paints the failure green. The artifact it uploads is empty for the same reason. Adding the secret would not fix it. requireTestMode only accepts a database on localhost, so no hosted API can ever serve /dev/test-session. Keeping the specs would mean running Postgres and the API inside the job. The specs stay on disk; a later change folds them into the fixture approach, which needs neither. `ds-shots` replaces it with something that can actually fail. It captures 33 fixtures at four phone widths, diffs them against the base branch, and keeps one comment up to date with the screens that moved, worst first, with the images inline. No API, no database, no provider key, no secret. It never blocks a merge — it is not in ci-success.needs, and the gate stays at about 110 seconds. Removing e2e takes a little off the median and much more off the tail: e2e carried a 20 minute timeout, hung three times on 2026-08-18, and report and ci-success both waited on it, so ci-success never reported at all and the content automerge stalled with it. The baseline is captured on pushes to dev and main, not inside the PR. A cache written by a PR run is scoped to that PR, so a sibling PR cannot read it; ten PRs on one base restore the baseline instead of rebuilding it. Fork PRs skip both jobs. They get a read-only token and no submodule token, so the checkout and the comment cannot work, and a red job would tell the author nothing they can fix. Assets go to pr-assets-<n> as a force-pushed orphan under a per-commit path, so the branch never grows and raw.githubusercontent cannot serve yesterday's image. A new workflow deletes the branch when the PR closes. The 20 branches left by the hand-made version of this convention still need one sweep.
… not The Playwright suite has run nowhere since 2026-04-27: not in CI, and not locally without a secret nobody has. Eight of its eighteen specs contained no assertion at all — they opened a page and took a screenshot. The shots pipeline does that at four widths, pixel-stable and diffed, so those were already replaced. Of the rest, most asserted something the code cannot break. One checked that the URL contained the step it had just typed into the URL. One checked a back button that an eslint rule and a hook test already guard. Four earned their place: icons keep their inline fill and the stroke their viewBox asks for, the claim and send-link pages render without a wagmi provider crash, a stale cookie does not bounce /setup to /home, and the captured share-asset card is not blank. Those four did not actually run, which is how they survived. With no API the app layout holds every screen on the loading mascot, so two failed and three passed against a page that had rendered nothing. `?__fixture=home` gives them the fake session, and they now assert against real content in 11 seconds. The icon check was stale too. It demanded stroke-width 2 from every icon, but Icon.tsx crops seven of them to a 20-unit viewBox and the stroke scales with the crop. It now reads the rule from each icon's own viewBox, so it stays honest without an allowlist of seven names to maintain. Coverage does go with this. No fixture covers the add-money and withdraw country forms, the claim link states, /qr-pay, /send/success, or the public routes outside (mobile-ui). Those need real links, real rails or a route group the fixture registry does not reach, and they belong in the nightly sandbox tier. Screenshot-only coverage of everything else was already dead. Everything the deletions orphaned goes with them: the old config, the global setup, all six utils, both scripts, and five package scripts that pointed at nothing.
Rebasing onto the design-system tip turned the registry test red, which is what it is for. /settings and /notifications are gone — bc084ae removed the orphaned settings root page, 537c2a4 removed the notifications page — so three fixtures pointed at routes that no longer exist. A fixture that opens a 404 is not a broken screenshot. It is a screenshot baseline for a screen that is not in the product, and every later diff compares against it. 30 fixtures. settings-language stays; only the settings root went.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…in CI, fix the frozen clock Three review findings. The fixture cookie writes were origin-wide and unguarded: activate() overwrote a real jwt-token session, and while ?__fixture=off sat in the URL every API call re-deleted a freshly created real login. Both writes now touch the cookie only when its value is the fixture's own. The four kept behaviour specs ran in no CI job — they now run inside ds-shots, which already has the preview build and Chromium (verified: 7 pass in 11.5s). FROZEN_NOW predated the registry's newest timestamp, so relative times rendered as future dates; it now sits after 2026-08-01.
/code-review pass — 2026-08-26Fixed in b70943f (typecheck + 3,632 unit tests + all 7 regression tests + a live capture smoke all verified green):
Found but NOT fixed (advisory, for the author to weigh):
|
|
Review notes. Full digest for the whole batch: https://claude.ai/code/artifact/9d565cfc-146b-44d9-98a6-2d76cd081bad The fixture idea is good and the dev story holds up. This also conflicts with its base right now, so it needs a rebase before it can merge. 1. The visual diff has never run on this PR
The baseline problem stacks on top. So the headline feature cannot work on the branch it ships on, and it has not been seen to work in CI. Worth solving before merge — either compute the baseline in-job from the merge-base, or widen the trigger and the allowlist. 2. The capture spec never checks that fixture mode is on
In CI this is safe — the
Also 3. Path filter blind spotThe To be clear this is not a regression — the old 4. Render-phase side effect in
|
…al-shots # Conflicts: # .github/workflows/tests.yml
…rain, prove fixture mode on camera - tests.yml: feat/design-system joins the push/PR triggers and the baseline-save allowlist, because a DS-based PR can only read caches written on its base branch; remove after the DS branch merges to dev - ds-shots-filter KEEP now lists e2e/flows/ and the regression config, so a PR touching only the behaviour specs still runs them - fixtures.spec.ts asserts the app promoted the URL param into sessionStorage before shooting — a non-preview build otherwise yields 120 screenshots of /setup under fixture filenames; the param comes from FIXTURE_PARAM instead of a hardcoded string - reuseExistingServer only outside CI, so a stale next start is never silently adopted on a runner - peekActiveFixture(): read-only fixture lookup for render bodies; useWallet no longer writes sessionStorage/cookie during render - FixtureBanner: fixed strip naming the active fixture, since a logged-in user opening a ?__fixture= link would otherwise see faked API data with no visible tell; tap exits via ?__fixture=off - fix the fixture arithmetic: 30 fixtures x 4 widths = 120 PNGs
|
Re: your review — rebased on feat/design-system (merge 404bc6a) and all findings addressed in 64bd8db. Every gate re-verified after the merge: typecheck clean, 3,664 unit tests, preview build + 7 regression tests, capture smoke.
Thanks for the refute-pass section — the two dead candidates stayed dead. |
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
🖼 Visual diff — no baseline yetNothing is cached for |
…e, failure artifacts Three CI-only failures from the first run ds-shots ever made on this PR. The ds-lint ratchet counted the fixture banner's inline styles — src/dev/fixtures is dev tooling, same class as the dev/ds and dev/devices dirs already allowlisted. The icon spec waited for 'visible' where its assertions only read attributes — 'attached' with a longer budget survives a slow runner. The regression step moves to the end of the job so a red spec cannot abort the baseline/diff/ comment steps, and on failure the e2e/__results__ error contexts upload as an artifact — the share-builder failure reproduces nowhere locally (passes with CI=1), so the next red run must show what the runner actually rendered.
The artifact from the first upload showed the runner rendering the not-found page: BASE_URL falls back to https://peanut.me when unset, which flips the dev layout's prod gate, and CI has no .env to set it. The icon and share-builder specs drive /dev pages, so both died on a 404 that never reproduces locally. Also removed the swallowed waitForSelector in the button test — it passed green against that same 404 (zero icons found, empty loop).
…al-shots # Conflicts: # e2e/flows/dev-showcase.spec.ts
The second base merge collided on dev-showcase.spec.ts: this branch deleted it with the old harness, the DS branch just rebuilt it as the M2 regression target. Keep the DS spec — it has a live consumer again — and restore the two utils it imports. Its console-error gate now allowlists what the static regression harness cannot provide: SW registration, backend 401/403 resources, and public-RPC CORS refusals from the local origin (external target + 127.0.0.1 origin only, so an app-own CORS bug still fails). All 10 regression tests pass under CI=1.
2 participants
Summary
Verifying a design-system change meant building the base branch, screenshotting by hand, building the head branch, screenshotting again, and pairing the results into a table. PR #2758 shipped 25 of those, at one width, and the table could not say which screens had actually moved.
This makes that automatic, and removes a CI job that had reported success without running a test since April.
Base:
feat/design-system(PR #2813). Ships with the design-system release.What lands
1. Named app states —
?__fixture=<name>30 fixtures.
<route>?__fixture=<name>answers every API call from a named state, so any screen renders with no database, no API, no provider keys and no harness secret. The same URL works in a browser, in/dev/devicesat six widths, on a Vercel preview, and under Playwright.Hugo's question — "I want to see the profile screen with username
bh12ui2buibui52biand 1.2 billion points" — is now a URL./dev/fixtureslists them all.Built on
demo-api, which already answers every route the app calls, so a fixture states only what differs. Most are one to three lines.2. Screenshots —
npm run test:visual30 fixtures × 4 widths (320/375/393/430) = 120 PNGs in about 60 seconds. 320 is an iPhone on Larger Text and finds the most bugs; 430 is a Pro Max.
npm run test:visual:diff a bprints which screens moved, worst first.A diff tool that cries wolf gets ignored in a week, so the run pins everything that moves: frozen clock, no cross-origin requests, no service worker, animations and transitions off, fonts and images awaited, and a wait for two equal text samples because the points total counts up from JavaScript. Two full captures of one build differ in zero pixels. A deliberate margin change on one heading surfaced exactly the three affected fixtures at all four widths, nothing else.
The run fails rather than shoots when a loader is on camera. A green run of 120 spinners looks identical to a green run of 120 screens.
3. CI —
e2eout,ds-shotsinThe
e2ejob has reported success without running a test since271750934(2026-04-27), which demoted it withcontinue-on-errorand the note "TEST_HARNESS_SECRET not in CI env yet". It dies inglobalSetupbefore a spec loads.Adding the secret cannot fix it:
requireTestModeonly accepts a database on localhost, so no hosted API can ever serve/dev/test-session.ds-shotsreplaces it — captures the head, diffs against a baseline captured on pushes todev, and keeps one PR comment up to date with the screens that moved and the images inline. Advisory: not inci-success.needs, so it never blocks a merge. The gate stays at about 110 seconds.4. The old spec suite
18 specs, 2,154 lines, running nowhere. Eight contained no assertion at all. One asserted that the URL contained the step it had just typed into the URL.
Four earned their place and are kept. They did not actually run either — with no API the layout holds every screen on the loading mascot, so two failed and three passed against a page that had rendered nothing.
?__fixture=homegives them a session; they now assert against real content in 11 seconds.Coverage this gives up
Stated plainly. No fixture covers the add-money or withdraw country forms,
/claimlink states,/qr-pay,/send/success, or the public routes outside(mobile-ui). The claim page runs a real crypto check against the link's pubKey, which static fixture responses cannot satisfy — that belongs in the nightly sandbox tier. None of it was running before this PR either.Verified
typecheckclean ·npx jest302 suites, 3632 tests ·prettierclean ·ds-lint --checkno metric increased0 changed, 120 unchangedFollow-ups, not in this PR
error-pointsanderror-userfixtures land after the production-fix PR, or they would capture the bugs those fixes removetest:e2e:regressiontakes 8 seconds and needs no secret — cheap to add/devchunks still ship in the production bundle; the routes are blocked, the JavaScript is not