fix: get E2E Tests fully green (wallet-lifecycle, theme-toggle, visual regression baselines) - #338
Merged
prodbycorne merged 5 commits intoSep 7, 2026
Conversation
… dir The failure-artifact upload step pointed at playwright-report/, but e2e/playwright.config.ts doesn't configure the html reporter, so that directory is never written -- every failed E2E run has been uploading an empty artifact. test-results/ is where traces, error-context.md (a DOM/ accessibility snapshot at the moment of failure), and screenshots actually land regardless of which reporter is configured. Needed this to get real diagnostics on the remaining E2E failures.
✅ Deploy Preview for spiffy-melomakarona-eb1e8a ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for smart-drop ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Root-caused both remaining E2E failures from SmartDropLabs#335 using real CI trace/DOM diagnostics (via the test-results upload fix in SmartDropLabs#338), instead of guessing. - wallet-lifecycle.spec.ts: "/" is the only route this suite visits fresh. On a first-ever visit, OnboardingOverlay (src/components/ OnboardingOverlay) opens a Chakra Modal, which marks the rest of the page aria-hidden for focus-trapping. That's correct accessible-modal behavior, not a bug -- but it removed the floating connect button from the accessibility tree entirely, so getByRole couldn't find it at all ("element(s) not found", not just "not visible"). Confirmed via the captured error-context.md DOM snapshot, which showed the onboarding dialog open and blocking. Seeded the same "smartdrop_onboarded" flag the overlay itself writes on dismiss, so this test starts at the returning- user baseline every other spec in the suite implicitly gets by never visiting "/". - visual-regression.spec.ts "theme toggle persists across reloads": traced the actual click+reload sequence via the captured trace.zip. The toggle click worked correctly every time (localStorage read "dark" immediately after, on the first check, no retries needed) -- the bug was in the test itself. It seeded the "light" baseline via page.addInitScript(), which Playwright re-runs on *every* navigation in that page, including the page.reload() the test uses to check persistence. So the seed script was silently overwriting the just-set "dark" value back to "light" right before the app re-hydrated and the persistence check read it. Switched to page.evaluate() + one explicit reload for the one-time seed, which doesn't re-fire on the later reload. - Added the 3 baseline screenshots these tests need (404-page-light, leaderboard-credits, leaderboard-stake) -- this repo has never had any committed, so toHaveScreenshot() always failed on its first run regardless of app correctness. Pulled the actual PNGs Chromium rendered on the real ubuntu-latest CI runner (via the same test-results artifact) rather than generating them locally, since pixel output isn't portable across platforms and this Mac can't run Playwright's Chromium at all (unsupported OS). The 4th (404-page-dark) still has no baseline -- the toggle-persistence test never reached that screenshot line before this fix, so no actual.png existed yet to pull. It'll get generated (and fail once, by Playwright's normal first-run behavior) on this branch's next CI run; will follow up with it once that run lands. Both bugs were latent and pre-existing -- neither test had ever run to completion in CI before SmartDropLabs#335 fixed the Lighthouse crash that was gating this whole job.
The 3 baselines added in the previous commit were pixel-accurate everywhere except one region: Next.js's dev-mode error/issues badge (<nextjs-portal>, bottom-left -- this suite runs against `pnpm dev`, not a production build). Confirmed via a real CI diff (leaderboard-stake-diff.png from run 34143853333): every pixel outside that badge matched the committed baseline exactly; only the badge itself differed between two separate CI runs of the same deterministic page state. The animation-disabling <style> this spec already injects into document.head doesn't reach whatever's animating inside the badge (it likely lives in a shadow root), so its pixels aren't reproducible run to run -- unsuitable for a pixel-diff baseline regardless of how the baseline is generated. Masking it with Playwright's `mask` option (paints over the region before comparing, in both the actual screenshot and the stored baseline) is the standard fix for exactly this class of known- dynamic, out-of-scope-of-the-test UI element. This invalidates the 3 baselines just committed -- they were captured without masking, so the masked region's paint differs from what a masked comparison now expects. Removing them here; a follow-up commit will add freshly-generated masked baselines (all 4 now, including 404-page-dark, which the previous run reached for the first time after the wallet-lifecycle/theme-toggle fixes).
Two follow-ups after the masked run (34144631276) still failed:
1. Playwright's `mask` option didn't visibly cover <nextjs-portal> --
the actual.png still showed the badge unmasked. A custom element's own
layout box doesn't necessarily bound whatever's rendered in its shadow
root, so masking by locator bounding-rect isn't reliable here. Hiding
it outright (display: none via page.evaluate, before each screenshot)
sidesteps that entirely -- confirmed simpler and doesn't depend on
Playwright correctly computing a box for shadow-DOM content.
2. Once tests ran far enough to reach `expect(consoleErrors).toEqual([])`
for the first time (previously blocked earlier by the theme-toggle bug
fixed last commit), it failed for reasons that turned out to be
pre-existing and unrelated to anything under test:
- Next's own dev-only <HotReload> hydration-mismatch warning (from its
dev-mode error-overlay machinery re-hydrating oddly under automation
-- framework-internal, never ships in the production build).
- The browser's own "Failed to load resource: 404" log, logged
automatically for the routes these specs *intentionally* navigate to
because they don't exist -- that's the point of a 404-page test, not
an app defect.
Filtered both out of trackConsoleErrors() by pattern rather than
weakening the assertion generally, so it still catches anything else.
All 4 baselines this suite needs, pulled from a real CI run (34145325691) with the dev-overlay-hide fix applied -- confirmed clean (no dev-only badge) by inspecting each image directly before committing. Same rationale as the earlier baseline commits: this repo has never had any committed, Playwright always fails toHaveScreenshot() on a first run regardless of app correctness, and pixel output isn't portable across platforms so these have to come from the actual ubuntu-latest runner CI uses, not generated locally.
praizehimm
changed the title
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
2 participants
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
#335 fixed the crash that was blocking this job entirely (a broken
next-intl rewrite 404ing every route under
next start), which letPlaywright run to completion for the first time ever on this repo. That
surfaced 4 more pre-existing, dormant failures. This PR root-causes and
fixes all of them using real diagnostics pulled from actual CI runs
(trace.zip, error-context.md, and the real Linux/Chromium screenshots) --
this Mac can't run Playwright at all (
ERROR: Playwright does not support chromium on mac13), so nothing here was guessed blind.Result:
E2E Testsis fully green -- all 15 tests pass. (Run:https://github.com/SmartDropLabs/smartdrop-frontend/actions/runs/34145838859)
1. Diagnostics were silently broken (needed this to see anything)
.github/workflows/e2e.ymluploadedplaywright-report/on failure, bute2e/playwright.config.tsdoesn't configure the html reporter, so thatdirectory was never written -- every failed run uploaded an empty
artifact. Pointed it at
test-results/instead, which is where tracesand DOM snapshots actually land.
2.
wallet-lifecycle.spec.ts: floating connect button not found on "/"error-context.mdshowed the real cause: OnboardingOverlay opens a ChakraModal on a first-ever visit to "/", which
aria-hiddens the rest of thepage for focus-trapping (correct modal behavior) -- that removed the
connect button from the accessibility tree entirely.
/is the onlyroute this suite visits fresh. Seeded the "already onboarded" localStorage
flag before navigating, matching the returning-user baseline every other
spec implicitly gets.
3.
farm.spec.ts: countdown locator strict-mode violationTwo elements legitimately show the same locked position's countdown
(
FarmPoolRowandEarningRoweach render it independently) -- not abug. Scoped the locator with
.first(), matching the pattern alreadyused two lines below for
unlockBtn.4.
visual-regression.spec.ts"theme toggle persists across reloads"Traced the actual click+reload sequence via
trace.zip: the toggleworked correctly every time. The test's own setup was the bug --
page.addInitScript(() => localStorage.setItem(..., "light"))re-runs onevery navigation in that page, including the
page.reload()used totest persistence, silently overwriting the just-set "dark" value back to
"light" right before the check read it. Switched to
page.evaluate()+one explicit reload for the one-time seed.
5.
visual-regression.spec.ts(3 tests): missing baselines + dev-overlay noiseThis repo never had baseline screenshots committed, so
toHaveScreenshot()always failed by design on a first run. Pulled real Chromium renders from
actual CI runs (not generated locally -- pixel output isn't portable
across platforms) for all 4 needed baselines. Along the way found two
more issues these tests never got far enough to hit before:
<nextjs-portal>, this suiteruns
pnpm dev) animates inside what's likely a shadow root theexisting animation-disabling script can't reach, making it
non-deterministic pixel-to-pixel.
maskdidn't visibly cover it (thecustom element's own layout box doesn't necessarily bound its shadow
content), so hidden it outright via
display: nonebefore eachscreenshot instead.
expect(consoleErrors).toEqual([])was tripping on two benign, dev/test-only sources: Next's own
<HotReload>hydration-mismatch warning(framework-internal, never ships to production) and the browser's own
"Failed to load resource: 404" log for the routes these specs
intentionally navigate to because they don't exist. Filtered both by
pattern rather than weakening the check generally.
Verified
pnpm run typecheck-- clean