Add Playwright test suite for class list UI and image regressions - #47
Merged
Conversation
Add a repo-level Playwright dependency and npm script so image verification can be run consistently from the repository root with npm run test:images. Keep this tooling outside the Svelte app package because Playwright is only used for repository-level verification and the app itself does not import it at runtime. Ignore Playwright report and test-results directories across package workspaces so failed local runs do not leave ephemeral artifacts available for accidental commits.
Add a gated SvelteKit /__image-check route that renders every class image through the same enhanced image glob used by the class card UI. The route is available in dev and in CI only when PLAYWRIGHT_IMAGE_CHECK=true, keeping the diagnostic page out of normal production routing. Add Playwright coverage for three image safety checks: required fallback images must exist, changed image filenames must not delete or rename existing lookup-contract paths, and newly added image files must not use generated duplicate-style suffixes such as _2, copy, final, or (1). Add a browser-level decode check that visits the diagnostic route, watches image responses, calls img.decode() for every rendered image, and fails if any image response is not OK or any rendered image has zero natural dimensions. Configure Playwright to run the Svelte app from the app directory, use the dev server locally, use preview in CI after a build, and target Chromium as the minimal browser needed for image decoding coverage.
Add a focused Image Check workflow that runs on pull requests touching class images, image lookup code, the diagnostic image-check route, Playwright tests, Playwright config, or package files that affect the test runner. Use a full checkout so the Playwright spec can compare changed image files against the pull request base with git diff and detect deletions or renames instead of relying on a manually maintained image manifest. Install both the repository-level Playwright package and the Svelte app dependencies, install the Chromium browser with system dependencies, build the app, and then run npm run test:images so CI exercises both build-time image processing and browser image decoding.
Capture the existing app/package-lock.json metadata rewrite separately from the image verification implementation so dependency-lock churn is easy to inspect on its own. The app package declarations are unchanged; this commit only removes stale peer metadata fields from resolved lockfile entries as produced by the npm tooling used during the image verification setup. Keeping this in a separate commit avoids hiding lockfile noise inside the Playwright test or CI commits and makes it straightforward to revert independently if the team chooses to regenerate the app lockfile differently.
Rename Woodshop_Safety.jpg to Woodshop_Safety_2.jpg to preserve the exact manual failure case used to validate the new image verification checks. This mirrors the UI-risk scenario where an existing class image lookup path disappears and a generated duplicate-style filename appears in app/src/lib/images. The Playwright spec intentionally rejects both sides of that change: the deleted lookup-contract filename and the _2 suffix on the new file. Keep this as an isolated commit so the failing example can be reviewed, pushed, and later reverted without touching the reusable Playwright route, tests, workflow, or package tooling.
Add yaml as an explicit app dev dependency so npm ci has a concrete lockfile entry for Tailwind's nested postcss-load-config optional yaml peer. GitHub Actions was resolving yaml@2.8.4 during strict clean-install validation and failing because the app lockfile only contained the older yaml 1.x entry required by the app-level PostCSS loader. Regenerate app/package-lock.json so yaml@2.8.4 is available at the app root while yaml@1.10.3 remains nested under the older postcss-load-config package that still requires the 1.x range. This keeps the lockfile complete for npm ci without changing application runtime code. Generate the Prisma client explicitly in the Image Check workflow after npm ci --prefix app and before npm run build --prefix app. A clean CI install does not leave the generated client in place, and the SvelteKit build imports Prisma during server analysis. Verified locally with npm ci, npm ci --prefix app, npm exec --prefix app -- prisma generate --schema app/prisma/schema.prisma, and npm run build --prefix app.
klgilbert
marked this pull request as draft
May 12, 2026 15:35
Add compose.test.yaml at the repository root, a docker-compose stack that provisions a disposable Postgres instance preloaded with dev/dev-db.sql so the SvelteKit app can be exercised end-to-end against realistic class data without depending on Neon CRM, AWS SSM secrets, or production database access. Use the postgres:17 image because dev/dev-db.sql is produced by pg_dump 18.x, which emits "SET transaction_timeout = 0". That GUC was introduced in Postgres 17, so the older postgres:16 image rejects the init script on first start with "unrecognized configuration parameter" and the container exits with code 3. Mount dev/dev-db.sql read-only at /docker-entrypoint-initdb.d/01-dev-db.sql. The official Postgres image executes every .sql and .sh file in that directory exactly once on the first start of an empty data volume, so the seed loads automatically on `docker compose up` with no shell wrapper. The "01-" prefix reserves alphabetical room for additional ordered init scripts in the future without having to rename this file. Match dev/load-dev-db.sh's existing convention: - container_name: asmbly-dev-db - POSTGRES_PASSWORD: localdev - host port 5432 so DATABASE_URL is identical for both database sources and a developer can switch between the shell script and this compose file without reconfiguring anything. The two cannot run concurrently (same container name, same port); that is intentional. They are alternative database sources, not parallel ones, and forcing the conflict surfaces the choice rather than silently routing traffic to the wrong one. Include a pg_isready healthcheck so `docker compose up -d --wait` blocks until the database is actually accepting connections after the init script has finished applying the dump. Without this, tests that run immediately after bring-up race against the seed load and intermittently fail before the database is ready. This file is consumed by upcoming Playwright UI tests and by the matching CI workflow. It is independent of, and does not modify, dev/load-dev-db.sh; that script remains the recommended local development path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add two inert data attributes to the card root <div> in
app/src/lib/components/classCard.svelte so upcoming Playwright UI
tests can locate class cards without coupling assertions to DaisyUI
class names (.card, .card-side, lg:max-h-72) that may legitimately
change with future styling work.
- data-testid="class-card" gives tests a single selector for
counting visible cards, iterating their contents, and asserting
filter / sort / search behavior end-to-end. Mirrors the
data-testid="class-image" pattern already used by the existing
/__image-check diagnostic route.
- data-category={event.category} lets tests assert category-filter
correctness directly. After checking the "3D Printing" sidebar
box, a test can assert that every visible card carries
data-category="3D Printing" without having to decode the embedded
AsmblyIcon SVG or hand-maintain a class-name-to-category mapping
inside the test.
Both attributes are inert in production: no JavaScript reads them,
no CSS targets them, and they add a few bytes per attribute per
card to the rendered HTML. They cost nothing at runtime and the
test surface is much more stable than locating cards by their
visual styling.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add app/tests/class-list.spec.js, a Playwright suite that drives
the real `/` class list page end-to-end against the compose-backed
Postgres database from compose.test.yaml. Replace the existing
diagnostic-route-based image-check coverage with assertions made
against the actual production rendering path.
Group assertions by concern into five tests so each test name is
a contract statement and a failure points to the affected area:
1. renders correctly — page title, card count > 0, no empty
state, and for every visible card: an <img> whose
naturalWidth > 0 and that successfully passes img.decode(),
a heading, a "View Schedule" anchor whose href matches
/event/<id>?eventId=<id>, and visible "Price:" text. This
test also installs a response listener that fails the test
if any image request returns a non-2xx status.
2. search works — typing a real word taken from the first
card's heading narrows the list to fuzzy-matching cards;
typing nonsense ("zzzqxnonsense") surfaces the "Sorry, no
results found" empty-state card with its Clear Search
button; clicking Clear Search restores the baseline card
count. Exercises both Fuse.js fuzzy match and the URL
-> filters reactivity chain.
3. sort works — switching Sort By to "Name" sorts cards
ascending case-insensitively (matching the comparator in
NeonEventInstance.compare); toggling Sort Order reverses
the order. Verifies both the select-driven sortBy and the
swap-checkbox-driven sortAsc paths through updateSearchParams.
4. filters work — checking a category checkbox restricts
visible cards to that category via data-category; toggling
Group By Class Type off does not decrease the count (each
instance now stands alone instead of being collapsed
under its type); toggling Show Unscheduled Classes off
does not increase the count.
5. navigation works — header links to Mentor Series and
Classes FAQ have the expected external asmbly.org hrefs;
direct navigation to /?sortBy=Name&archCategories=X
restores the corresponding UI state from URL parameters
via the afterNavigate hook; clicking a View Schedule link
lands on /event/<id>?eventId=<id>.
Assertions are shape-/invariant-based and derive class names
from the rendered page where possible. Regenerating dev/dev-db.sql
(adding or removing classes) does not require touching the spec
unless the renderer itself changes.
Run the suite serially via test.describe.configure({ mode: 'serial' })
because all five tests share one vite dev server. Running them in
parallel produced intermittent 30s timeouts on selectOption and
fill calls when multiple workers triggered concurrent SvelteKit
navigations against the same Prisma connection. Serial execution
finishes in ~35s, parallel execution in this configuration was
flaky and not faster.
Modify app/playwright.config.js:
- Inject DATABASE_URL into webServer.env so vite dev /
vite preview reach the compose-backed Postgres without
the caller having to export anything. Mirrors the
DATABASE_URL that dev/load-dev-db.sh exports.
- Drop the PLAYWRIGHT_IMAGE_CHECK env var; no consumer
remains after the route deletion below.
- Replace `url: 'http://127.0.0.1:4173/__image-check'`
with `port: 4173`. The port-based readiness probe is a
plain TCP poll that does not require any route to be
live or the database to be reachable; tests surface
database failures via their own assertions instead.
Delete app/src/routes/__image-check/+page.{server.js,svelte}.
The diagnostic route was originally added to render every
globbed image in a single grid for browser-level decode
coverage. Two reasons to remove it:
- Browser-level decode coverage now lives in the
"renders correctly" test above, which calls img.decode()
on every <img> actually shown by `/`. enhanced:img
processes the same glob in both routes, so any decode
regression that would have surfaced on /__image-check
surfaces here too for every image used by a visible
class. Images not referenced by any class would no
longer be exercised, but the CI build step still fails
if enhanced:img cannot process any file in the glob,
so that gap is covered earlier in the pipeline.
- Gating a route on an env var (PLAYWRIGHT_IMAGE_CHECK)
is fragile: a misconfiguration in production could leak
the diagnostic page. The only zero-risk option is to
not ship the route at all.
Rewrite app/tests/images.spec.js to remove the browser-based
"class images load and decode in the browser" test that
depended on the deleted route. Preserve the three node-only
hygiene checks unchanged because each addresses a regression
the UI suite cannot catch on its own:
- required fallback class images exist — pure readdir
check against app/src/lib/images. Catches accidental
deletion of a category default that the lookup logic
would silently fall back to.
- class image changes do not remove or rename existing
filenames — runs `git diff --name-status` against
origin/<base> in CI or `git status --porcelain`
locally. Catches rename-into-fallback regressions
(e.g., Woodshop_Safety.jpg -> Woodshop_Safety_2.jpg)
where the page still renders, just with the wrong
image. The UI suite cannot detect this because the
fallback default decodes successfully.
- new class image filenames do not look like generated
duplicate variants — flags additions like
"Copy of …", "…_2", "… (1)" before they're merged.
These three run without a browser or a database; they are
plain node tests that happen to use Playwright's runner.
Replace the test:images and test:ui scripts in package.json
with a single `test` script:
"test": "playwright test -c app/playwright.config.js"
It runs every spec in app/tests/. Add test:db:up and
test:db:down helpers wrapping the compose lifecycle so the
local workflow is two single commands:
npm run test:db:up # provision the test DB
npm test # run all 8 tests (3 hygiene + 5 UI)
npm run test:db:down # tear down volume (optional)
The 8-test suite passes locally in ~36s on a Mac M-series:
3 hygiene tests in <50 ms total (parallel, no browser) and
5 UI tests serially against the dev server.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rename .github/workflows/image-check.yml to ui-tests.yml and
rewrite it to run the full consolidated test suite (3 hygiene
+ 5 UI tests) instead of just the image-only spec. Git detects
the rename, so existing references to image-check.yml in
history remain navigable.
Broaden the trigger to anything that can affect the rendered
class list. The previous workflow ran only on changes to the
image directory, classCard.svelte, the deleted __image-check
route, and a handful of build files; the new workflow runs on:
- app/** (any source change that can break
the page render — routes, components,
models, image files)
- compose.test.yaml (database provisioning changes)
- dev/dev-db.sql (test fixture changes that can
legitimately move card counts and
sort orders)
- package.json,
package-lock.json (test runner or dependency changes)
- .github/workflows/
ui-tests.yml (the workflow itself)
Restore `fetch-depth: 0` on the checkout step. The hygiene
tests in app/tests/images.spec.js run `git diff --name-status
origin/$GITHUB_BASE_REF...HEAD` to find image renames and
deletions; a shallow clone (the default) does not contain the
base ref and would either fail the diff or silently produce
empty change sets, masking exactly the rename-into-fallback
regressions the hygiene tests are designed to catch.
Switch from `npm run test:ui` (and the previously separate
`npm run test:images`) to `npm test`, which now runs all
specs under app/tests/. The Postgres test database is
provisioned via:
docker compose -f compose.test.yaml up -d --wait
The remaining steps mirror what a local contributor would
run:
npm ci && npm ci --prefix app
npx playwright install --with-deps chromium
npm exec --prefix app -- prisma generate --schema app/prisma/schema.prisma
npm run build --prefix app
npm test
The build step is required because CI uses `npm run preview`
rather than `npm run dev` (selected by the `process.env.CI`
branch in app/playwright.config.js). It also serves as an
implicit "no broken images" gate: enhanced:img processes
every image in $lib/images/* during the build, so a corrupt
or unprocessable file fails the workflow before tests even
start.
Naming the workflow ui-tests.yml (rather than tests.yml)
leaves room for a future unit-tests.yml that would run vitest
against the app package without provisioning a database or a
browser. The two runners would then sit side by side under
distinct workflow names rather than being conflated in one
file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit e51e4bd.
klgilbert
changed the title
klgilbert
marked this pull request as ready for review
May 14, 2026 20:20
klgilbert
force-pushed
the
image-verification
branch
from
May 14, 2026 20:30
9e82c74 to
550c61a
Compare
|
This looks good to me. I'm less familiar with the classes repo, so I'll leave it to @robz for the formal approval, but the only two things I see that could potentially cause issues are:
Other than those two things, things look good to me! Thanks so much for participating in the volunteer day and for writing these tests. 🙇 Like I said above, I'll leave this to @robz for the final approval, but it looks good to me so far. |
Contributor
|
I reverted the test case about files being moved since that's unnecessary. The event.category change looks fine (safe due to the event guard). I agree it's weird to have a separate package.json, but I guess it's because this relies on dev at the top level. Let's consolidate them and with the new unit tests in a followup. |
Contributor
|
fixes #39 |
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
3 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
Adds an end-to-end Playwright test suite that exercises the
/class list page against a real Postgres database loaded fromdev/dev-db.sql, plus retains the filesystem/git-diff image hygiene checks. Removes the env-gated/__image-checkdiagnostic route in favor of asserting against the real page. Onenpm testcommand runs everything in ~36s.What this catches
class-list.spec.jsclass-list.spec.jsrenders correctlyper-cardimg.decode()images.spec.jsgit-diff check_2,(1),Copy of …)images.spec.jsfilename-pattern checkimages.spec.jsfilesystem check$lib/images/*npm run buildstepWhat's in the diff
compose.test.yaml(new)dev/dev-db.sqlas an init script. Matchesdev/load-dev-db.shconventions (containerasmbly-dev-db, passwordlocaldev, port 5432).app/tests/class-list.spec.js(new)dev-db.sqlregeneration doesn't require test edits.app/tests/images.spec.js(new)app/src/lib/components/classCard.sveltedata-testid="class-card"anddata-category={event.category}to the card root (inert in production).app/playwright.config.js(new)DATABASE_URLinto the webServer env; port-based readiness probe (no test-only endpoint needed). Usesvite devlocally,vite previewin CI.package.json(root, new)test,test:db:up,test:db:downscripts..github/workflows/ui-tests.yml(new)npm test. Triggers onapp/**,compose.test.yaml,dev/dev-db.sql, and package files.fetch-depth: 0so the hygiene git-diff works against the merge base.app/src/routes/__image-check/(deleted)How to run
Locally — two single commands:
To watch the browser run instead of headless:
npm test -- --headedTo debug interactively in the Playwright UI:
npx playwright test -c app/playwright.config.js --uiIn CI: the
UI Testsworkflow runs the same steps automatically on any PR that touchesapp/**,compose.test.yaml,dev/dev-db.sql, the rootpackage.json, or.github/workflows/ui-tests.yml.Test plan
npm run test:db:up && npm testpasses locally (8/8) after revertinge51e4bde51e4bdfailuree51e4bd, CI re-runs and reports 8/8 greendev/load-dev-db.shstill works for the existing local-dev flow (compose file shares container name + port, but they're alternative database sources, not parallel ones)