writing-visual-tests

# Writing Visual Tests This skill adds a screenshot-comparison test to Opik's visual regression suite. Unlike the functional E2E suite (`tests_end_to_end/e2e/`, see `writing-e2e-tests`), this suite's assertion *is* the screenshot — so seeding must produce deterministic pixels, not just correct data. **Announce at start:** "I'm using the writing-visual-tests skill to add a visual test for X." **Plan before implementing.** Before writing or editing any file, work through step 1 ("Scope") of "The loop" below and present the user a short plan: which page/panel and states you'll cover, what new seed route/client method (if any) you'll add, which spec file and project the new test(s) will live in (and why, per "One project per screenshot group"), and the resulting screenshot name(s). Wait for the user's explicit approval before starting step 2 (seeding) or any other implementation work. Only skip this pause if the user's request already specifies all of these choices unambiguously. ## Where tests live The suite is at `tests_end_to_end/visual-tests/`: - **Specs:** `tests/<area>.spec.ts` — e.g. `visual-comparison.spec.ts` (happy-path pages), `empty-states.spec.ts`, `trace-sidebar.spec.ts`. One `test.describe` per spec, one `test.beforeAll` that seeds data and resolves the project ID. - **Page objects:** `page-objects/<page>.page.ts` — see "Page object pattern" below; **always follow it**, don't freehand a different shape. - **Seeding:** `helpers/test-helper-client.ts` — a typed HTTP client over the Flask `test-helper-service` (`../test-helper-service/routes/*.py`), which wraps the real Python SDK. Add a new route + a new client method together; never seed by clicking through the UI. - **Screenshot util:** `tests/utils/screenshot.ts` — `screenshot(page, name, extraMasks?)`. Read this whole file before writing a new spec. - **Global setup/teardown:** `global-setup.ts` / `global-teardown.ts` — create/delete the fixed-name projects (`visual-project`, `visual-empty-project`, `visual-sidebar-project`, …) used across specs. Fixed names, no timestamp suffix, so screenshots are identical across runs. **Give each new portion of screenshots its own project** — see "One project per screenshot group" below. - **Baselines:** `screenshots/baseline/*.png` — see "Baselines are local and gitignored" below before you assume one exists. ## Page object pattern Every existing page object (`projects.page.ts`, `experiments.page.ts`, `datasets.page.ts`, `test-suites.page.ts`, `logs.page.ts`, …) follows the same shape. New ones must match it exactly — don't introduce `test.step()` wrapping (that's the `e2e/` suite's convention, not this one), don't return locators from `goto()`, don't add a constructor signature that differs from the others. ```ts import { Page } from '@playwright/test'; import { BasePage } from './base.page'; export class WidgetsPage extends BasePage { constructor(page: Page, baseUrl: string, workspace: string) { super(page, baseUrl, workspace); } async goto(projectId: string): Promise<void> { await this.page.goto(this.url(`projects/${projectId}/widgets`)); await this.page.waitForLoadState('load'); await this.dismissWelcomeDialogIfPresent(); } // Races the populated state against the empty state so the same method // works whether the seeded project has data or not — never a bare // unconditional wait, which hangs forever if the match text is wrong. async waitForReady(expectedCellText: string): Promise<void> { await this.page.getByRole('heading', { name: 'Widgets', exact: true }).waitFor({ state: 'visible', timeout: 10000 }); await Promise.race([ this.page.locator('tbody tr td').filter({ hasText: expectedCellText }).first().waitFor({ state: 'visible', timeout: 10000 }), this.page.getByRole('heading', { name: /no widgets yet/i }).waitFor({ state: 'visible', timeout: 8000 }), ]); } async waitForEmpty(): Promise<void> { await this.page.getByRole('heading', { name: 'Widgets', exact: true }).waitFor({ state: 'visible', timeout: 10000 }); await this.page.getByRole('heading', { name: /no widgets yet/i }).waitFor({ state: 'visible', timeout: 20000 }); } } ``` Rules that fall out of this: - **Extend `BasePage`** and take the exact `(page, baseUrl, workspace)` constructor, even if a given page doesn't need `workspace` yet — consistency lets every spec construct page objects identically. - **`goto()`** always: navigate via `this.url(...)` (never a raw string), `waitForLoadState('load')`, then `dismissWelcomeDialogIfPresent()`. - **`waitForReady(expectedCellText)`** takes the text to match, doesn't hardcode it — callers pass whatever they actually seeded. See "Waiting: for content, not just structure" below for what's safe to pass. - **`waitForEmpty()`** is a separate method, reused by `empty-states.spec.ts` — don't fold empty-state handling into `waitForReady` as a flag. - Add new interaction methods (click, open, switch-tab) alongside `goto`/`waitForReady`, not in the spec file. ### Exception: panels and non-route components A side panel (e.g. the trace/span detail panel) isn't its own route — it opens over an existing page. Don't force it into `BasePage`. Give it just a `Page` and a `root` locator scoped to the panel's own testid, then scope every other locator/action under `root`: ```ts import { Page, Locator } from '@playwright/test'; export class TraceDetailsPanelPage { constructor(private page: Page) {} get root(): Locator { return this.page.getByTestId('traces'); } async waitForLoaded(): Promise<void> { await this.root.getByRole('button', { name: 'Close' }).waitFor({ state: 'visible', timeout: 20000 }); // ... } } ``` Construct it from the *page* the panel opens over, in the spec: `new TraceDetailsPanelPage(page)`, right after the page object that opened it. ## Safety: verify local config before seeding The Python SDK behind `test-helper-service` reads `~/.opik.config`. If it points at a cloud environment, seeding creates real data there. ```bash grep url_override ~/.opik.config ``` If `url_override` is anything other than `http://localhost:5173/api/`, back it up and point it local: ```bash cp ~/.opik.config ~/.opik.config.bak-visual-test cat > ~/.opik.config << 'EOF' [opik] url_override = http://localhost:5173/api/ workspace = default EOF ``` When the work is done, restore it and stop `test-helper-service` — see the "Cleanup checklist" below before doing either: both are machine-wide, and blindly restoring/killing can break a concurrent run. ## Local environment Use `./opik.sh` (repo root) for the Docker stack — **check what's already running before touching it**: ```bash docker ps --format '{{.Names}}\t{{.Status}}' # look for opik-opik-frontend-1, opik-opik-backend-1, etc. ./opik.sh --verify # only if you're unsure containers are healthy ``` If containers are already up (healthy, running), leave them alone — don't `--stop`/`--clean`/restart something a teammate or another task left running. Only run `./opik.sh` (no flags) to start what's missing. **A rebuilt stack is not a fresh one.** `./opik.sh --build` rebuilds images but reuses existing Docker volumes (MySQL, ClickHouse, …) unless you explicitly wipe them — so a container that just restarted can still be serving months of accumulated local data: projects, feedback definitions, environments, AI provider keys, whatever previous sessions (yours or a teammate's) left behind. If your test's scope includes a **default** or **empty** state — anything you'd normally get "for free" on a brand-new workspace — don't trust what a long-lived instance currently shows. See "Know your defaults before you seed or delete" below before writing any seed/cleanup code against it. If a genuinely fresh stack (fresh volumes) is feasible and not disruptive to concurrent work, prefer it for this kind of test; if not, fall back to reasoning from the Liquibase migrations directly rather than from the current DB contents. Start the test helper service (check port 5555 is free first): ```bash lsof -ti:5555 # empty output = free, safe to start cd tests_end_to_end/test-helper-service OPIK_BASE_URL=http://localhost:5173 OPIK_WORKSPACE=default TEST_HELPER_PORT=5555 \ nohup .venv/bin/python app.py > /tmp/test-helper-service.log 2>&1 & disown curl -s http://localhost:5555/health ``` Restart it (`kill $(lsof -ti:5555)`, relaunch) whenever you change a route file — Flask's dev server doesn't hot-reload here. Same caution as everywhere else this port comes up: confirm the PID is actually yours before killing it (see "Cleanup checklist" below). ## Cleanup checklist Everything this suite generates is gitignored (root `.gitignore` entries: `tests_end_to_end/visual-tests/test-results/`, `visual-report/`, `.auth/`, `.test-state.json`, `screenshots/`; repo-wide: `allure-results/`) — nothing here risks a bad commit, but it does accumulate on disk across sessions. Work through this list before ending a visual-testing session, not just the config restore. **`~/.opik.config` and the `test-helper-service` process are machine-wide, not scoped to your run.** If another agent, terminal, or teammate could be running visual/E2E tests concurrently on the same machine, check before you touch either — restoring the config or killing the process out from under a run in progress breaks it: ```bash lsof -ti:5555 # a PID here isn't necessarily "your" leftover — it may be someone else's active run ps -p <pid> -o lstart,command # when did it start, relative to when you started your own work? ``` If in doubt, ask rather than kill. This isn't hypothetical — it happened during this skill's own authoring session: a `test-helper-service` instance was found still listening after "cleanup," and it turned out to belong to a different run in progress, not a leftover. Also note: `playwright.config.ts`'s `webServer` directive auto-spawns `test-helper-service` on port 5555 if nothing answers `/health` when a test starts (`reuseExistingServer: !process.env.CI`) — so an instance can appear that you never manually started, including right after your own "final run." Don't assume every process on that port traces back to a command you ran. | Artifact | What it is | Action | |---|---|---| | `~/.opik.config` | Points the SDK at an environment | Confirm no concurrent run needs it local (see above), then **restore from your backup** (`cp ~/.opik.config.bak-visual-test ~/.opik.config`) and delete the backup file. | | `test-helper-service` process (port 5555) | Flask bridge — may be one you started, or one `webServer` auto-spawned, or someone else's | Confirm it's actually idle/yours (see above) before `kill $(lsof -ti:5555)`. | | Docker containers (`opik-opik-*`) | The stack under test | **Leave alone** if they were already running when you started (the common case) — don't `--stop`/`--clean` a stack you didn't start. Only stop containers you personally started for this session. | | `test-results/` | Playwright's per-test output (failure screenshots, videos, traces) | Regenerated every run — safe to delete: `rm -rf test-results`. | | `screenshots/comparison/` | Side-by-side copy written on every comparison run (i.e. whenever `SKIP_TEARDOWN` is not `1`) | Regenerated every run — safe to delete: `rm -rf screenshots/comparison`. | | `visual-report/` | The HTML report (`npm run test:report`) | Safe to delete: `rm -rf visual-report`. | | `allure-results/` | Allure reporter output | Safe to delete: `rm -rf allure-results`. | | `.test-state.json` | Experiment-ID bridge for non-`SKIP_TEARDOWN` runs | Removed automatically by `global-teardown.ts` on a normal run; delete by hand only if you aborted mid-run without ever finishing a non-`SKIP_TEARDOWN` pass. | | `.auth/` | Playwright storage state written by `global-setup.ts` | **Not cleaned up by `global-teardown.ts`** — it only removes `.test-state.json` and server-side data. Delete by hand: `rm -rf .auth`. | | `screenshots/baseline/*.png` | Your actual baselines | **Not cruft — don't reflexively delete.** Gitignored and per-machine by design (see "Baselines are local and gitignored"); leave them for the next person/session to reuse, unless you specifically want to force a clean re-baseline. | | Seeded project (`visual-project` / `visual-empty-project` / `visual-sidebar-project`) in the running Opik instance | Server-side data, not a local file | Deleted by `global-teardown.ts` on a plain `npx playwright test` run (not `SKIP_TEARDOWN=1`). Do a final run *without* `SKIP_TEARDOWN` before you finish (see step 7) so this actually happens — don't leave it to the next person. If teardown never ran (e.g. an interrupted `SKIP_TEARDOWN=1` session), delete any of the three by hand via the running instance's UI/API. | One-shot cleanup for the local-only report/log artifacts (keeps baselines): ```bash cd tests_end_to_end/visual-tests rm -rf test-results screenshots/comparison visual-report allure-results ``` ## Baselines are local and gitignored `screenshots/` is gitignored (see `.gitignore`) — baseline PNGs are **never committed**. There is no CI job for this suite; it's a local, on-demand check. Two consequences: - On a fresh clone, or after `git clean`, **no baselines exist at all** — every screenshot name in every spec needs generating before you can run a compare pass. - Adding a *new* screenshot name always needs a fresh baseline, regardless of whether other baselines in the same directory already exist from a previous session. **Always (re)generate baselines for the whole suite**, not just the spec you touched, before judging a compare run — via `--update-snapshots` (see the loop below). Don't assume "the baseline is already there"; check, or just regenerate, it's cheap: ```bash ls tests_end_to_end/visual-tests/screenshots/baseline/ | grep <your-prefix> ``` Regenerate whenever any of these change, not just on first add: the screenshot name, the masks passed to it, a wait added/removed before it, or the seeded data shape. ## The loop 1. **Scope.** Which page/panel, which states (tabs, empty vs. populated, with/without optional data). Check whether an existing spec's `beforeAll` already seeds a project/entity you can extend, or whether this needs its own. 2. **Seed data.** Add a `test-helper-service` route if no existing one produces the shape you need (see "Adding a seed endpoint" below), plus a `TestHelperClient` method. 3. **Page object(s).** Follow the pattern above exactly; reuse an existing page object if the page already has one. 4. **Spec.** One `test()` per screenshot, unique names (see "Unique screenshot names"). 5. **Generate/refresh baselines for the whole suite** — every time, whether or not baselines already exist (see "Baselines are local and gitignored" above). Run **all** spec files, not just the one you touched: adding or changing a test can shift shared page chrome, and stale baselines in untouched files are otherwise only caught by accident. ```bash cd tests_end_to_end/visual-tests SKIP_TEARDOWN=1 OPIK_BASE_URL=http://localhost:5173 npx playwright test --update-snapshots --reporter=list ``` `SKIP_TEARDOWN=1` keeps the seeded projects alive for a fast next run; the plain `npx playwright test` invocation still always cleans and recreates them at the *start* (global-setup), so each run starts from a known state. If the full-suite run surfaces failures in files you didn't touch, don't reflexively blame your change — check whether those baselines simply predate a recent, unrelated app change (compare `ls -la screenshots/baseline/` timestamps against recent frontend commits) before deciding whether to regenerate them or investigate further; confirm with the user before regenerating baselines outside the scope of your own change. 6. **Stress-test for stability — do not skip this.** Run the full-suite compare pass (no `--update-snapshots`) 3 times in a row: ```bash for i in 1 2 3; do echo "=== Run $i ===" SKIP_TEARDOWN=1 OPIK_BASE_URL=http://localhost:5173 npx playwright test --reporter=list done ```