test(visual): add visual regression testing across all routes and themes - #3161
Open
afonsojramos wants to merge 9 commits into
Open
test(visual): add visual regression testing across all routes and themes#3161afonsojramos wants to merge 9 commits into
afonsojramos wants to merge 9 commits into
Conversation
|
setchy
approved these changes
Aug 7, 2026
Member
|
Awesome addition, @afonsojramos 👏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Adds pixel-level visual regression testing. The existing 54
.snapfiles serialize DOM structure, which is structurally blind to the class of change the recent theming work has been making: colour tokens, spacing, and CSS variables all move without altering markup.This screenshots every route in a real browser and commits the baselines.
36 baselines, ~1.5 MB:
Built on Vitest 4 browser mode rather than a standalone Playwright suite, so it reuses the existing helpers, mocks, and config. Playwright is present only as the browser driver; there is no second test runner, no
playwright.config.ts, and no separate spec format.How it works
Screenshots only match when the browser build and font stack are identical, so both commands run the suite inside a pinned Playwright container, and CI verifies against that same container. Contributors do not need Playwright's browsers installed locally.
.gitignorepermits only*-chromium-linux.png, so a screenshot taken on a contributor's own OS can never become a baseline.pnpm testis unchanged in scope and still runs the DOM and node projects only.scripts/visual.shreads the image tag from theplaywrightdevDependency, so there is nothing to keep in sync there. The CI job'scontainer:is a literal tag because zizmor'sunpinned-imagesrule rejects an expression it cannot evaluate, and suppressing a security rule to save one string is a poor trade; a Renovate group keeps it moving in lockstep with the package instead.Determinism
Baselines are worthless if they drift, so each source of nondeterminism is pinned explicitly:
Dateonly, so React's scheduler keeps real timers)img.emojiis sized purely by CSS so layout is unchangedrandomElementstubbedTwo of these were found the hard way. Avatars were leaking to githubusercontent because
GitifyUserspells itavatarwhile notifications useavatarUrl. And the login route flaked 4 runs in 5: its tab indicator measures positions withgetBoundingClientRectin a layout effect, and Playwright's built-in animation freezing happens at screenshot time, far too late for that measurement.Guard against false greens
Vitest namespaces baselines per platform and creates missing ones automatically. Run bare on macOS, the first invocation therefore writes a fresh
-darwinset from the current working tree and the second passes against it — reporting green on a branch carrying a real regression, without ever consulting the committed Linux baselines. This was hit in practice during review.A
globalSetupguard now fails the run outside Linux with an actionable message, and the npm scripts route through the container, so the trap is unreachable. Verified: with an 8px sidebar regression present, both the first and second run now fail.Verification
allowedMismatchedPixels: 0is deliberate. Rendering here is fully deterministic, and a ratio-based tolerance of even 0.01 would permit ~2000 pixels on a 500x400 viewport, which is larger than an entire icon and enough to hide a completely swapped glyph.Included fix
fix(mocks)corrects two notification fixtures whose hardcodeddisplay.iconvalues production can never emit:FeedPullRequestOpenIconGitPullRequestIconIssueOpenedIconTagIconFeedPullRequestOpenIconappears nowhere insrcoutside that mock. This caused zero snapshot churn, which is precisely why it went unnoticed: no existing test ever rendered those two icons.Tradeoffs worth reviewing
The job runs on
ubuntu-24.04-arm, notubuntu-latest. Chromium segfaults under amd64 emulation on Apple Silicon, which makes local runs impossible on the machines maintainers actually use. Pinning both sides to arm64 keeps generation and verification on identical hardware. The cost is that running the suite locally needs an arm64 host with Docker; everyone else relies on CI and can read the diff from thevisual-regression-diffsartifact.Glass coverage is partial. These capture the
backdrop-filterfallback that Linux and Windows receive. The macOS native vibrancy material is a window property Chromium cannot render, so it still needs a real-desktop capture.Baselines are binary blobs. Git stores them without useful deltas, so every intentional theme change rewrites them and grows history permanently. The matrix is deliberately scoped to keep that bounded rather than screenshotting every route against all 8 themes.
Follow-up, not included
Size.LARGEis 18, and octicons round down to their 16px variant, soNotificationRow.tsx:102,RepositoryNotifications.tsx:90, andAccounts.tsx:159all upscale a 16px-hinted glyph by 1.125-1.25x and render slightly soft. Left alone here because changing icon sizes is a design call rather than a bug fix.Test plan
pnpm test— 163 files, 1276 tests passingpnpm test:visual— 36/36, verified six consecutive runspnpm tsc --noEmitcleanpnpm checkcleanubuntu-24.04-arm