Skip to content

[1.1] test: add Playwright e2e regression net for current app#34

Open
zphillips wants to merge 1 commit into
PADAS:developfrom
zphillips:contrib/e2e-tests
Open

[1.1] test: add Playwright e2e regression net for current app#34
zphillips wants to merge 1 commit into
PADAS:developfrom
zphillips:contrib/e2e-tests

Conversation

@zphillips

@zphillips zphillips commented Jun 4, 2026

Copy link
Copy Markdown
Contributor
Field Value
Change type ♻️ Same behavior (test)
Risk severity S4 – Low

Summary

Adds a 7-journey Playwright regression net for the current app (network mocked). Acts as a safety net before the React 19 / Vite / MapLibre modernization. No app code changes.

Decisions & alternatives

  • e2e-first (UI-level): assertions go through the UI, so the suite survives internal refactors — unlike unit tests bound to the current single large App component.
  • Considered unit-tests-first; rejected — they'd be largely rewritten by the refactor.
  • Trade-off: relies on the legacy dev server via --openssl-legacy-provider (Node 20 + webpack 4), tagged REMOVE-AFTER-VITE.

Links

Tests — how do we know it's safe? (check all used)

  • Unit
  • Integration
  • Contract
  • Synthetic / e2e
  • Manual
  • None

Suite runs 7/7 green (npx playwright test, self-contained, mocks config + ER API for determinism).

Incident detection — if this breaks in prod, how will we know?

N/A — test-only, never shipped to users; breakage shows in CI / local runs, not production.

Incident impact analysis — can we measure the blast radius?

N/A — S4, no production surface.

Rollback & mitigation — if it goes wrong, how do we fix it fast?

  • Safe to plain-revert? Yes — purely additive test files, no runtime/state/behavior touched.
  • Rollout: direct. If revert isn't enough: n/a.

Seven UI-level journeys (load, legend, story panel, track toggle, locate,
help tips, reset hotkey) with mocked config + ER API for determinism. These
assert behavior through the UI so they survive the planned Vite/React 19/
MapLibre overhaul.

webpack-dev-server runs under --openssl-legacy-provider (Node 20 + webpack 4);
tagged REMOVE-AFTER-VITE for removal in Pass 1.2.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@zphillips zphillips changed the title test: add Playwright e2e regression net for current app [1.1] test: add Playwright e2e regression net for current app Jun 4, 2026
@zphillips zphillips marked this pull request as ready for review June 4, 2026 03:01

@chrisj-er chrisj-er left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Playwright e2e regression net

Verdict: Approve with minor follow-ups. Well-scoped, low-risk, test-only. I traced each journey to the real source and the selectors and behaviors all check out. UI-level e2e as a safety net before the Vite/React 19/MapLibre overhaul is the right call.

Correctness — verified against source ✅

  • Selectors (#legend-open-button, #tips-button-container, #subject-track-button, #subject-location-button, #close-icon, #map-container) all exist.
  • Test 5 asserts flyTo.center === [0.01, 0.01]LocButton passes subject.last_position.geometry.coordinates, matching the sub-1 fixture exactly.
  • Test 7 Alt+R → logKey checks keyCode 18 & 82 then resetMap()flyTo({ bearing: 0 }). The native-keydown-with-defined-keyCode trick correctly works around the app reading e.keyCode.
  • Test 4 mock route **/tracks matches the real URL …/subject/sub-1/tracks, and waitForRequest is correctly registered before the click.
  • The flyTo spy reassigns the method on the same window.GlobalMap object the app calls — wrapping is sound.

Issues worth raising

1. The "regression net" isn't wired into CI (main gap). The description says "breakage shows in CI / local runs," but .circleci/config.yml only has deploy_testER (build + S3 deploy on develop) — no lint, no tests. As-is this suite only catches regressions when someone remembers to run yarn test:e2e locally. For a safety net guarding a big refactor, that's a real hole. Suggest a CircleCI job (Node 20 image) running npx playwright install --with-deps chromium && yarn test:e2e on PRs.

2. No setup docs. Running fresh requires npx playwright install chromium. Worth a README line or a pretest:e2e hook so it doesn't fail mysteriously for the next person.

3. Deprecated assert import syntax. import config from './fixtures/config.json' assert { type: 'json' }assert was replaced by with and is removed from modern V8 (Node 22+). Playwright's transpiler currently handles it, but prefer with { type: 'json' } for future-proofing.

4. Design-doc link points to a fork. It references github.com/zphillips/er-map/... rather than PADAS/er-map, likely inaccessible to the team. Same path appears in the REMOVE-AFTER-VITE comment.

Nits

  • fullyParallel: true boots a full Mapbox WebGL map per test concurrently — fine at 7, but a likely flakiness source as the suite grows.
  • reuseExistingServer: !CI reuses a locally-running real (non-mocked) dev server on :3000 — mostly harmless but can confuse local runs.

Done well

  • The REMOVE-AFTER-VITE comment is excellent — root cause, the crutch, and the removal trigger all documented.
  • Asserting through behavior (flyTo spy, request interception) instead of WebGL pixels is what keeps these tests alive across the MapLibre swap.
  • Deterministic data-URI fixtures avoid unmocked network entirely.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants