Desktop E2E: Playwright suite with visual regression diffs - #65805
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (read-only token — formal approval requires write access)
Adds a Desktop E2E Playwright test suite with visual regression diffs. New GitHub Actions workflow e2e-desktop.yml for running the tests in CI.
- Clean test infrastructure addition
- Visual regression testing for desktop UI
- Proper CI integration
- No security concerns
Reviewed by Hermes Agent
fd104cf to
03f0d2a
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Adds Desktop E2E Playwright suite with visual regression diffs. Very large diff (3183 additions, 155KB). The GH_TOKEN and OPENROUTER_API_KEY references in the diff are CI workflow configuration (secrets injected at runtime, not hardcoded). Given the size and scope of the test suite additions, a human review is warranted.
Reviewed by Hermes Agent
529c383 to
e7b6077
Compare
Adds a Windows E2E workflow that downloads the built installer, runs it via AutoHotkey automation (install-hermes-desktop.ahk), and launches the installed app. Includes button reference screenshots for the AHK image matching.
- Type gatewayState in session store - Electron main.ts: force-show window for e2e test workers - tsconfig: include e2e test types - nix/devShell.nix: add cage for headless visual testing on tiling WMs
Adds a full desktop Playwright E2E suite that launches the Electron app against a mock inference server, exercising the full boot chain: electron -> hermes serve -> mock provider -> renderer Includes: - Mock OpenAI-compatible inference server (mock-server.ts) - Shared fixtures with sandbox isolation (credentials, HERMES_HOME, userData, fixed window-state.json for reproducible screenshots) - Test specs: boot, boot-failure, onboarding, mock-backend-setup, chat, and packaged-app launch - Visual regression: expectVisualSnapshot() wraps toHaveScreenshot in try/catch so diffs are reported without failing the test suite - CI workflow: xvfb at 1280x1024, baseline cache from main (--update-snapshots on main, compare on PRs), step summary table with diff/actual/expected image links, dedicated visual-diffs artifact - dev:mock script for local fake-provider development - test:e2e:visual + test:e2e:update-snapshots scripts using cage - .gitignore: *-snapshots/ (baselines cached in CI, not committed)
The visual diff summary told reviewers to 'download and open to compare' instead of linking to the actual run's artifacts page. Now links directly to the run's /artifacts page and lists both artifacts with descriptions. Also, screenshots that matched their baselines were never written to test-results/, so the artifact only contained screenshots that diffed. Now the actual screenshot is always written to the output dir regardless of match/diff, so CI artifacts include every screenshot.
The check script ran: typecheck && test && test:desktop:all && build test:desktop:all calls ensurePackagedApp() → npm run pack, which itself runs npm run build (vite + electron-main + preload + stage-native-deps) before electron-builder --dir. The trailing npm run build was rebuilding the exact same dist/ output a second time. electron-builder --dir reads dist/ as input and doesn't mutate it, so nothing between the two builds invalidates the first one. On the CI linux runner this saves the vite+bundle build (~5s) on every js-tests run. The postbuild assert-dist-built.mjs still runs as part of pack's build step.
The summary step previously linked to the run's /artifacts page generically. Now it links the specific artifact download URLs from each upload step's artifact-url output. Reordered the steps so uploads run before the summary (since the summary needs their outputs), and added id: to each upload step. Each artifact gets its own clickable link: - playwright-test-results (all screenshots + traces) - playwright-report (interactive HTML report) - visual-diffs (just the diffed screenshots, PR-only)
Screenshots were catching the CONNECTING overlay and onboarding Preparing loading bar mid-transition because the wait helpers fire on text content while visual state lags behind. Fix at the source via reduced motion: - playwright.config.ts: emulate prefers-reduced-motion: reduce - styles.css: blanket reduced-motion rule kills all CSS animations/transitions - gateway-connecting-overlay.tsx: skip JS setTimeout exit choreography (text-out 360ms + hold 300ms + overlay fade 520ms) — jump straight to gone - decode-text.tsx: skip scramble interval, render resolved text immediately
Screenshots were catching the app mid-boot at ~92% with the onboarding Preparing progress bar still visible. waitForAppReady checked for the composer (textarea/contenteditable) with state:'visible', but Playwright considers an element visible even when a z-1300+ fixed overlay covers it (non-zero bounding box, not display:none). Now waits for the composer to be attached, then polls document.elementFromPoint at viewport center — if the topmost element is inside a position:fixed inset:0 overlay, the app isn't ready yet.
The boot-failure screenshot showed a progress bar because of two bugs:
1. waitForBootFailure matched on "Let's get you setup" (the onboarding
header that mounts from frame 1 during normal boot), so the screenshot
fired at ~86% progress while the Preparing component's progress bar was
still painted.
2. The Preparing component kept rendering the progress bar even after
boot.error was set — it just turned the bar red and appended the error
text below it.
Fixes:
- Preparing bails out (returns null) when boot.error is set, so
BootFailureOverlay (z-1400) owns the screen exclusively.
- applyDesktopBootProgress no longer clobbers a previously-set boot.error
when a late progress event arrives with error: null — failDesktopBoot is
terminal for the boot cycle.
- waitForBootFailure guards against progress bars being visible and matches
on actual failure signals (error toast, Retry/Repair buttons), not the
onboarding header.
- setupDeadBackend now accepts { fakeError: true } which injects
HERMES_DESKTOP_BOOT_FAKE_ERROR to trigger a real boot failure — the
previous dead-provider fixture never actually caused a boot failure
(hermes serve starts fine; the dead endpoint only matters at chat time).
- boot-failure.spec.ts updated to use { fakeError: true }.
Verified: e2e test passes with 0 progress bars in the DOM at screenshot
time (confirmed via DOM inspection), 16/16 vitest tests pass, typecheck
clean.
windows e2e ain't ready yet
ReviewSolid, genuinely useful test infra — the fixtures/mock-server/tracing layer is well-crafted. Three things hold it back: one prod UX regression, unrelated installer scope, and the promotion of a brand-new Electron suite to a required merge gate. Blocking1. if (phase !== 'live') return
if(reduce) {
setPhase('gone') // runs for ANY reduced-motion user, no return
}
if (previewing) { // 6-space misindent; brace closes at 4
if(reduce) { setPhase('gone'); return }
const id = window.setTimeout(() => setPhase('text-out'), PREVIEW_CONNECT_MS)
return () => window.clearTimeout(id)
}
if (gatewayState === 'open' && shownRef.current) {
setPhase(reduce ? 'gone' : 'text-out')
}
The other prod tweaks are good and should stay: the Should split / reconsider2. Installer rewrite is unrelated scope with its validation disabled. 3. Nits
|
Reverts the Preparing component changes from b285711 so the progress bar turns red (bg-destructive) and the error text shows below it when boot.error is set, instead of bailing out with an early return null. The corresponding e2e guard in waitForBootFailure (e2e/fixtures.ts) that rejected any progress bar in the DOM is dropped — it now waits for the failure dialog (Retry/Repair/Use local gateway/Connection settings) or the "Desktop boot failed" toast. The boot-failure.spec.ts header comment is updated to match. Verified: tsc clean, vitest boot-failure-reauth (21/21) + boot-failure-overlay (3/3) pass, npm run build clean, playwright e2e/boot-failure.spec.ts 2/2 pass.
Pulls commit 3dab86a out of this branch per review — the install.ps1 rewrite (swapping the astral install-script for a direct GitHub-zip download) is a real Windows-installer behavior change that belongs in its own installer PR, not riding along in a Desktop E2E PR. e2e-windows.yml is `if: false` on both jobs and can't run on Linux CI, so the rewrite lands here with no coverage. The deleted install tests (test_install_ps1_native_stderr_eap.py, test_install_ps1_uv_powershell_host.py) are restored — they'll be dropped alongside the installer change in its own PR. Original commit 3dab86a will be cherry-picked onto a dedicated installer PR.
Blocking #1 — gateway-connecting-overlay.tsx reduced-motion regression: the top `if (reduce) setPhase('gone')` fired unconditionally on mount whenever reduce-motion was on, so every OS reduced-motion user lost the CONNECTING overlay during cold boot entirely (jumped to 'gone' before the gateway was even open). The intent was to skip the exit *choreography*, not to skip showing the overlay. Removed the unconditional top block and the redundant nested preview block; kept only the third branch (`gatewayState === 'open' && shownRef.current` → `reduce ? 'gone' : 'text-out'`) which correctly gates the short-circuit on connect. Also fixed `if(reduce)` missing-space, 6-space misindent, and the same 3-line comment pasted three times. Nit #1 — tsconfig excludes e2e, so specs were never typechecked in CI. Added tsconfig.e2e.json (extends base, includes e2e/ + playwright.config.ts, adds @playwright/test types) and wired it into the typecheck script. This surfaced three latent type errors that are fixed in the same commit: - fix-electron-tracing.ts: `app._context` and `electron._playwright` are private APIs — added `as any` on the access before the existing cast. - playwright.config.ts: `reducedMotion: 'reduce'` directly under `use:` is not a valid UseOptions property in playwright 1.58; it's a BrowserContextOption accessed via `contextOptions: { reducedMotion: 'reduce' }`. The old form was silently ignored at runtime, so reduced-motion emulation wasn't actually active — screenshots could catch overlays mid-fade (exactly what the comment warned about). Nit #2 — fix-electron-tracing.ts reaches into Playwright internals (_playwright, _allContexts, _context) with no public contract. Added a header comment calling out the `@playwright/test` exact pin (=1.58.2) so a future bump knows to re-verify the private symbols still exist. Nit #3 — main.ts TEST_WORKER_INDEX block had stray 6-space indentation. Verified: tsc -p . && tsconfig.electron && tsconfig.e2e → 0 errors; vitest boot-failure-overlay (3/3) + boot-failure-reauth (21/21) pass; npm run build clean; playwright e2e/boot-failure.spec.ts 2/2 pass.
The Windows installer E2E scaffolding (e2e-windows.yml + AutoHotkey helper + button screenshots) lands in its own draft PR (ethie/windows-e2e) targeting this branch, so it can be reviewed + iterated independently of the desktop E2E suite. Both jobs remain `if: false` until the installer E2E is ready to run.
these are not flaking in any of my runs, and i think it's important to actually excercise these pathways!! catching and fixing any flakes will fix real bugs IMO. the runtime is short - 1:41 on the last run - and it runs in parallel to other tests, so no extra time. nits:
|
OutThisLife
left a comment
There was a problem hiding this comment.
Approving — all review items addressed:
- Overlay a11y (was blocking): exit short-circuit now lives only in the
gatewayState === 'open' && shownRef.currentbranch (setPhase(reduce ? 'gone' : 'text-out')). Connecting/appearance path is intact, redundant blocks + duplicated comments gone. Reduced-motion users keep the boot overlay. - Installer scope:
install.ps1rewrite + test deletions dropped;e2e-windows.ymlpulled to its own branch (33→26 files). - e2e typecheck:
tsconfig.e2e.jsonadded so specs are covered.
Remaining prod changes are sound (boot.ts error-latch, reduced-motion decode-text/styles, session typing, env-gated main.ts hooks). Desktop E2E green (<2min).
Only conscious call left is e2e-desktop in the required all-checks-pass gate — acceptable given visual diffs are advisory, skip→success covers unrelated PRs, and retries: 1. Fine to own it; revisit if flake shows up.
Summary
Adds a full desktop Playwright E2E suite that launches the Electron app against a mock inference server, exercising the complete boot chain (
electron → hermes serve → mock provider → renderer) without any real LLM.Also includes a Windows installer E2E workflow (AutoHotkey automation), installer script fixes, and minor desktop type fixes.
Commits
managed_uv.pypath fixes for winget/uv installs,install.ps1updates, removes 2 stale install testsgatewayStatein session store, force-shows window for e2e test workers in electron main, addscageto devShell for headless visual testing on tiling WMsE2E Suite Details
mock-server.ts) — streaming SSE + non-streaming, just enough forhermes serveto resolve a provider and replyfixtures.ts) — sandbox isolation (credential stripping, isolatedHERMES_HOME+ userData, unique app name), fixedwindow-state.jsonfor reproducible screenshotsexpectVisualSnapshot()wrapstoHaveScreenshotin try/catch so visual diffs are reported without failing the test suite — diffs surface in the CI step summary + as downloadable artifactse2e-desktop.yml): xvfb at 1280x1024, baseline cache from main (--update-snapshotson main, compare on PRs), step summary table with diff/actual/expected image links, dedicated visual-diffs artifacttest:e2e:visualandtest:e2e:update-snapshotsusingcage(kiosk wayland compositor) so tiling WMs don't interfere with screenshotsdev:mockscript for local fake-provider developmentVerified
tsc -p . --noEmit— 0 errors in e2e fileseslint e2e/— 0 errorsnpm run build— builds cleannix build .#desktop— builds clean