mirror of
https://github.com/coder/coder.git
synced 2026-09-21 12:44:32 +08:00
Jeremy RuppelandSamuel Volin
46ec620767
## Summary Deflakes the `adjust user theme preference` Playwright test (`site/e2e/tests/users/userSettings.spec.ts`), tracked in DEVEX-415. The test selected the Light theme and then hard-navigated with `page.goto` before the optimistic appearance update was persisted. The navigation could cancel the in-flight `PUT /api/v2/users/me/appearance`, so the reloaded document embedded the stale `dark` preference and the final assertion flaked. `toPass` retries could not help because retrying the reload only re-reads the still-stale persisted state. ## Fix Wait for the appearance form's save spinner to clear before the hard reload, mirroring how other settings tests wait for a visible save confirmation. Asserting the optimistic light class first guarantees the spinner is already showing if a save started; a repeat run that is already light never shows it, so the test is idempotent and there are no direct API calls. To give the test a UI signal, `Spinner` gets an opt-in `label` prop that exposes it as a `role="status"` live region with an `aria-label` (decorative otherwise). `Loader` moves its label onto its own status container so it keeps a single status region. ## Changes - `site/src/components/Spinner/Spinner.tsx`: opt-in `label` prop. - `site/src/components/Loader/Loader.tsx`: label on its own status region. - `site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx`: label both appearance save spinners. - `site/e2e/tests/users/userSettings.spec.ts`: wait for the save spinner. ## Validation - `biome check` and `tsc -p .` pass. - Loader + AppearancePage unit tests and the affected storybook tests pass. - The e2e test passed 20/20 under `pnpm playwright:test -g "adjust user theme preference" --repeat-each 20`. <details> <summary>Implementation plan & decision log</summary> # DEVEX-415: Fix flake in "adjust user theme preference" e2e test ## Problem Playwright test `site/e2e/tests/users/userSettings.spec.ts` → `adjust user theme preference` flakes. After selecting the Light theme and hard-navigating to `/`, the reloaded page sometimes stays `dark`, failing the final assertion. CI Flake Bot has recorded repeated recurrences on `main` (latest 2026-08-17, runs `32004239187`, `31745261984`) even after PR #25183 added `toPass` retries. ## Root cause (confirmed by reading the code) The appearance update is optimistic and its persistence is not awaited before navigation: - `updateAppearanceSettings` (`site/src/api/queries/users.ts`) has an `onMutate` that optimistically writes the new theme into the React Query cache. The `<html>` class flips to `light` immediately, before the `PUT /api/v2/users/me/appearance` completes. - `useQueuedAppearanceSubmit` (`site/src/pages/UserSettingsPage/AppearancePage/AppearancePage.tsx`) serializes submits: if a request is in flight, the next is queued and only fires after the first settles. - A fresh `member` user has **empty** appearance settings. `migrateLegacyPreference` (`site/src/theme/themeMode.ts`) maps empty settings to `{ mode: "single", theme: "dark" }` (`DEFAULT_THEME = "dark"`). So the "Theme mode" dropdown already starts on **Single theme** and `<html>` starts `dark`. Selecting "Single theme" in the test is therefore a **no-op** (`onChangeMode` early-returns when `mode === draft.mode`) and fires **no** PUT. The only appearance PUT in the test is the one from clicking "Light default" (`onSelectSingle("light")` → `theme_preference: "light"`). - The test's first `expectLightThemeClasses(page)` passes purely from the optimistic cache. It then calls `page.goto("/")` almost immediately (~20 ms after the click). If PUT #2 (the light one) has not persisted server-side, the new document loads the still-persisted `dark` preference from embedded metadata, and every retry of the post-navigation assertion sees `dark` for the full 10 s window. `toPass` cannot help post-navigation because it only re-reads stale, already persisted state; it cannot make an unfinished/queued PUT complete. ## Implemented fix: wait for the saving spinner (UI signal) before navigation > Iteration history: (1) An earlier attempt waited on the appearance `PUT` via > a `waitForApiCall` helper, but that couples the test to a state transition > and is non-idempotent (a repeat run that is already light fires no PUT, so > the wait times out). CI retries reuse the same ephemeral server, so this is a > real hazard. (2) A second attempt confirmed persistence with > `page.request.get(...)`, but that calls the API directly and stops being a > site test. Both were rejected. The repo's non-flaky settings tests wait for a **visible save confirmation** (e.g. "settings updated successfully" toasts) before trusting the result. The appearance theme form has no toast; its only save-in-progress feedback is the `<Spinner>`. So the fix mirrors that pattern using the spinner: 1. Make the theme section's spinner identifiable via a new opt-in `label` prop on `Spinner` (sets `role="status"` + `aria-label` only when provided; decorative otherwise). `Loader` moves its label onto its own `status` container so it keeps a single status region and its `getByLabelText` queries keep working. 2. In the test, after clicking "Light default", assert the optimistic light class, then wait for that spinner to be hidden before `page.goto("/")`. Why this is correct and idempotent: - React Query sets `isPending` before `onMutate` applies the optimistic cache update, so by the time the optimistic light class is visible the spinner is already showing if a save started. Waiting for it to clear guarantees the PUT settled (and was not canceled by navigation) before the reload. - A repeat run that is already light: clicking "Light default" is a no-op radio change, no PUT fires, the spinner never shows, and `toBeHidden` passes immediately. The reload still shows light. - No direct API calls: the test only observes site UI. ### Implemented changes `Spinner` gains an opt-in `label` prop (`site/src/components/Spinner/Spinner.tsx`): ```tsx role={label ? "status" : undefined} aria-label={label} ``` `Loader` carries the label on its own status container (`site/src/components/Loader/Loader.tsx`), and the appearance save spinners use the new prop (`AppearanceForm.tsx`): ```tsx <Spinner loading={isUpdating} size="sm" label="Saving theme preference" /> <Spinner loading={isUpdating} size="sm" label="Saving terminal font" /> ``` `site/e2e/tests/users/userSettings.spec.ts`: ```ts await expect( page.getByRole("combobox", { name: /theme mode/i }), ).toContainText("Single theme"); // precondition: single mode const singleThemeGroup = page.getByRole("group", { name: "Theme" }); await expect(singleThemeGroup).toBeVisible(); await singleThemeGroup.getByText("Light default", { exact: true }).click(); await expectLightThemeClasses(page); // optimistic DOM => spinner showing if saving await expect( page.getByRole("status", { name: "Saving theme preference" }), ).toBeHidden(); // save settled before the hard reload await page.goto("/", { waitUntil: "domcontentloaded" }); await expectLightThemeClasses(page); ``` Validation: `biome check`, `tsc -p .` pass; Loader + AppearancePage unit tests and the affected storybook tests pass; the e2e test passed 20/20 under `pnpm playwright:test -g "adjust user theme preference" --repeat-each 20` (idempotent). ## Alternatives considered - **Wait on the appearance `PUT` via a `waitForApiCall` helper**: rejected. It couples the test to a state transition and is non-idempotent, a repeat run that is already light fires no PUT so the wait times out (14/15 repeats failed). CI retries reuse the same ephemeral server, so this is a real hazard, not just a local-repeat artifact. - **Confirm persistence with `page.request.get(...)`**: rejected. It calls the API directly and stops being a site test. - **Default `role="status"` on the shared `Spinner`**: rejected. `Loader` wraps `Spinner` in its own `status` div, so a default would nest two status regions and break `Loader.test.tsx`. The opt-in `label` prop avoids this. - **Add a networkidle wait or sleep before navigation**: rejected. Violates the repo guidance against `time.Sleep`-style timing hacks and is inherently racy. - **Fix product behavior instead of the test**: out of scope. Related bug DEVEX-94 ("Light Theme setting not respected until Appearance page opened") tracks product-side persistence/embedding behavior; this task is scoped to stabilizing the e2e test. ## Files touched - `site/src/components/Spinner/Spinner.tsx` (new opt-in `label` prop). - `site/src/components/Loader/Loader.tsx` (label on its own status region). - `site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx` (use the `label` prop on both appearance save spinners). - `site/e2e/tests/users/userSettings.spec.ts` (wait for the spinner). </details> --- This PR was created by Coder Agents on behalf of @jeremyruppel. --------- Co-authored-by: Samuel Volin <sam.volin@coder.com>
e2e
The structure of the end-to-end tests is optimized for speed and reliability. Not all tests require setting up a new PostgreSQL instance or using the Terraform provisioner. Deciding when to trade time for robustness rests with the developers; the framework's role is to facilitate this process.
Take a look at prior art in tests/ for inspiration. To run a test:
cd site
# Build the frontend assets. If you are actively changing
# the site to debug an issue, add `--watch`.
pnpm build
# Alternatively, build with debug info and source maps:
NODE_ENV=development pnpm vite build --mode=development
# Install the browsers to `~/.cache/ms-playwright`.
pnpm playwright:install
# Run E2E tests. You can see the configuration of the server
# in `playwright.config.ts`. This builds and runs `site/e2e/bin/coder`.
pnpm playwright:test
# Run a specific test (`-g` stands for grep. It accepts regex).
pnpm playwright:test -g '<your test here>'
Using nix
If this breaks, it is likely because the flake chromium version and playwright are no longer compatible. To fix this, update the flake to get the latest chromium version, and adjust the playwright version in the package.json.
You can see the playwright version here: https://search.nixos.org/packages?channel=unstable&show=playwright-driver&from=0&size=50&sort=relevance&type=packages&query=playwright-driver
# Optionally add '--command zsh' to choose your shell.
nix develop
cd site
pnpm install
pnpm build
pnpm playwright:test
To run the playwright debugger from VSCode, just launch VSCode from the nix environment and have the extension installed.
# Optionally add '--command zsh' to choose your shell.
nix develop
code .
Enterprise tests
Enterprise tests require a license key to run.
export CODER_E2E_LICENSE=<license key>
Debugging tests
To debug a test, it is more helpful to run it in ui mode.
pnpm playwright:test-ui