Skip to content

Commit 12edea0

Browse files
authored
Drop jsdom and testing library by switching to vitest browser mode (#3355)
I'd been meaning to do this for a while. Nearly all the changes are to the tests. ### 🤖 summary The unit suite is now pure logic running in node; anything that touches the DOM is a vitest browser mode test. That lets us remove jsdom, `@testing-library/*`, and msw/node entirely, along with the unit setup file, whose three stubs (canvas, matchMedia, ResizeObserver) existed only to fake DOM APIs jsdom lacks — those tests now run against the real things. The non-mechanical bits: * The API client tests run against an MSW worker in the browser (the same mechanism the dev server uses) instead of msw/node's patched fetch. * `TimeSeriesChart` created a canvas at module scope, which made it un-importable without a DOM — this is what the setup file's canvas stub was for. The measuring context is now created lazily. * The `TimeSeriesChart` spec no longer mocks anything: it renders real uPlot and spies on the instance's `redraw`/`setData` via a test-only `onCreate` prop. An earlier version of this PR kept the jsdom-era `vi.mock` of uplot-react, which flaked on webkit in CI — with a cold dep optimizer cache, the mock can attach to a different copy of the module than the one under test. Instance spying is the fallback because uPlot assigns its methods per instance; there's no prototype to patch. The old data-reference-stability test is reframed behaviorally: rerenders with equivalent data must not call `setData`, changed data must. * The `loginUrl` unit test only exercised string concatenation, so it's replaced with an e2e test covering the actual 401 → `/login?redirect_uri=...` flow, using a new `error-401` sentinel project in the mock API. With that test gone, nothing mocks `nav-to-login` anymore, and mockability was the module's stated reason for existing, so it's folded into `client.ts`. Note for #3312: `TimeSeriesChart.spec.tsx` moved to `TimeSeriesChart.browser.spec.tsx` and was rewritten around real uPlot instance spies, so the spec edits there will need porting.
1 parent 21d7c52 commit 12edea0

28 files changed

Lines changed: 353 additions & 1359 deletions

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
- Cover role-gated flows by logging in with `getPageAsUser`; exercise negative paths (e.g., forbidden actions) alongside happy paths as shown in `test/e2e/system-update.e2e.ts`.
3030
- Consider `expectVisible` and `expectNotVisible` deprecated: prefer `expect().toBeVisible()` and `toBeHidden()` in new code.
3131
- When UI needs new mock behavior, extend the MSW handlers/db minimally so E2E tests stay deterministic; prefer storing full API responses so subsequent calls see the updated state (`mock-api/msw/db.ts`, `mock-api/msw/handlers.ts`).
32-
- Co-locate Vitest specs next to the code they cover; use Testing Library utilities (`render`, `renderHook`, `fireEvent`, fake timers) to assert observable output rather than implementation details (`app/ui/lib/FileInput.spec.tsx`, `app/hooks/use-pagination.spec.ts`).
32+
- Co-locate Vitest specs next to the code they cover. Plain `.spec.ts` files run in node with no DOM, so keep them to pure logic. Anything that renders a component or touches a browser API goes in a `.browser.spec.tsx` file, which runs in real browsers via Vitest Browser Mode — use `vitest-browser-react`'s async `render`/`renderHook` (`app/ui/lib/FileInput.browser.spec.tsx`, `app/hooks/use-pagination.browser.spec.ts`).
3333
- Treat Vitest browser specs as small e2e tests: query by accessible role, label, or visible text and use retrying browser matchers. Avoid selectors coupled to CSS classes or internal DOM structure; inspect layout or computed styles only when the behavior has no semantic representation.
3434
- For sweeping styling changes, coordinate with the visual regression harness and follow `test/visual/README.md` for the workflow.
3535
- Fix root causes of flaky timing rather than adding `sleep()` workarounds in tests.

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ The web console has no special privileges as an API consumer. Logging in sets a
4141
- [oxide.ts](https://github.com/oxidecomputer/oxide.ts) generates an API client from [Nexus's OpenAPI spec](https://github.com/oxidecomputer/omicron/blob/main/openapi/nexus.json)
4242
- Testing
4343
- [Mock Service Worker](https://mswjs.io/) for mock API server
44-
- [Vitest](https://vitest.dev/) for unit tests
44+
- [Vitest](https://vitest.dev/) for unit tests, with [Browser Mode](https://vitest.dev/guide/browser/) for component tests
4545
- [Playwright](https://playwright.dev/) for E2E browser tests
4646

4747
## Directory structure
Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,54 @@
55
*
66
* Copyright Oxide Computer Company
77
*/
8+
import { http, HttpResponse } from 'msw'
9+
import { setupWorker } from 'msw/browser'
810
import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'
911

1012
import { project } from '@oxide/api-mocks'
1113

1214
import { api, q } from '..'
1315
import { resetDb } from '../../../mock-api/msw/db'
14-
import { overrideOnce, server } from '../../../test/unit/server'
16+
import { handlers } from '../../../mock-api/msw/handlers'
1517
import { processServerError } from '../errors'
1618

17-
// These are the only unit tests that make requests, so the MSW server
19+
const worker = setupWorker(
20+
...handlers.map((handler) => {
21+
// Browser Mode runs with NODE_ENV=test, so the API client uses a full URL.
22+
handler.info.path = 'http://testhost' + handler.info.path
23+
return handler
24+
})
25+
)
26+
27+
// These are the only browser tests that make requests, so the MSW worker
1828
// lifecycle lives here rather than in the global setup file — resetDb clones
1929
// the whole mock db, which is too slow to run after every test suite-wide.
20-
beforeAll(() => server.listen())
30+
beforeAll(() => worker.start({ quiet: true, onUnhandledRequest: 'error' }))
2131
afterEach(() => {
2232
resetDb()
23-
server.resetHandlers()
33+
worker.resetHandlers()
2434
})
25-
afterAll(() => server.close())
35+
afterAll(() => worker.stop())
36+
37+
// Override request handlers in order to test special cases
38+
function overrideOnce(
39+
method: keyof typeof http,
40+
path: string,
41+
status: number,
42+
body: string | Record<string, unknown>
43+
) {
44+
worker.use(
45+
http[method](
46+
path,
47+
() =>
48+
// https://mswjs.io/docs/api/response/once
49+
typeof body === 'string'
50+
? new HttpResponse(body, { status })
51+
: HttpResponse.json(body, { status }),
52+
{ once: true }
53+
)
54+
)
55+
}
2656

2757
// useApiQuery and useApiMutation are almost entirely typed wrappers around React
2858
// Query's useQuery and useMutation, so they're exercised end-to-end by the

app/api/__tests__/nav-to-login.spec.ts

Lines changed: 0 additions & 38 deletions
This file was deleted.

app/api/__tests__/safety.spec.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ it('mock-api is only referenced in test files', () => {
5151
expect(grepFiles('api-mocks')).toMatchInlineSnapshot(`
5252
[
5353
"AGENTS.md",
54-
"app/api/__tests__/client.spec.ts",
54+
"app/api/__tests__/client.browser.spec.ts",
5555
"mock-api/msw/db.ts",
5656
"test/e2e/fleet-access.e2e.ts",
5757
"test/e2e/instance-create.e2e.ts",
@@ -67,13 +67,12 @@ it('mock-api is only referenced in test files', () => {
6767
[
6868
"AGENTS.md",
6969
"README.md",
70-
"app/api/__tests__/client.spec.ts",
70+
"app/api/__tests__/client.browser.spec.ts",
7171
"app/main.tsx",
7272
"app/msw-mock-api.ts",
7373
"docs/mock-api-differences.md",
7474
"package.json",
7575
"test/e2e/utils.ts",
76-
"test/unit/server.ts",
7776
"tools/start_mock_api.ts",
7877
"tsconfig.json",
7978
]

app/api/client.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import { invariant } from '~/util/invariant'
2424
import { Api, type ApiResult } from './__generated__/Api'
2525
import type { FetchParams } from './__generated__/http-client'
2626
import { processServerError, type ApiError } from './errors'
27-
import { navToLogin } from './nav-to-login'
2827

2928
const _api = new Api({
3029
// unit tests run in Node, whose fetch implementation requires a full URL
@@ -96,6 +95,18 @@ type ExpectedError = {
9695

9796
const expectedErrorLabel = ({ statusCode }: ExpectedError) => `status ${statusCode}`
9897

98+
function loginUrl(opts: { includeCurrent: boolean }) {
99+
const { pathname, search } = window.location
100+
return opts.includeCurrent
101+
? // TODO: include query args too?
102+
`/login?redirect_uri=${encodeURIComponent(pathname + search)}`
103+
: '/login'
104+
}
105+
106+
export function navToLogin(opts: { includeCurrent: boolean }) {
107+
window.location.assign(loginUrl(opts))
108+
}
109+
99110
// method: keyof Api would be strictly more correct, but making it a string
100111
// means we can call this directly in all the spots below instead of having to
101112
// make it generic over Api, which requires passing it as an argument to

app/api/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,4 @@ export * from './__generated__/Api'
1919
export type { ApiTypes }
2020

2121
export type { ApiError } from './errors'
22-
export { navToLogin } from './nav-to-login'
22+
export { navToLogin } from './client'

app/api/nav-to-login.ts

Lines changed: 0 additions & 22 deletions
This file was deleted.

app/components/ErrorPage.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,7 @@ import { Link } from 'react-router'
1010

1111
import { Error12Icon, PrevArrow12Icon } from '@oxide/design-system/icons/react'
1212

13-
import { api, useApiMutation } from '~/api/client'
14-
import { navToLogin } from '~/api/nav-to-login'
13+
import { api, navToLogin, useApiMutation } from '~/api/client'
1514
import { Button } from '~/ui/lib/Button'
1615

1716
const GradientBackground = () => (
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
/*
2+
* This Source Code Form is subject to the terms of the Mozilla Public
3+
* License, v. 2.0. If a copy of the MPL was not distributed with this
4+
* file, you can obtain one at https://mozilla.org/MPL/2.0/.
5+
*
6+
* Copyright Oxide Computer Company
7+
*/
8+
import type uPlot from 'uplot'
9+
import { describe, expect, test, vi, type MockInstance } from 'vitest'
10+
import { render } from 'vitest-browser-react'
11+
12+
import { TimeSeriesChart } from './TimeSeriesChart'
13+
14+
const defaultData = [
15+
{ timestamp: 0, value: 10 },
16+
{ timestamp: 1000, value: 20 },
17+
]
18+
19+
const props = (yAxisTickFormatter: (v: number) => string, data = defaultData) => ({
20+
data,
21+
title: 'CPU',
22+
startTime: new Date(0),
23+
endTime: new Date(3_600_000),
24+
yAxisTickFormatter,
25+
loading: false,
26+
})
27+
28+
type Spies = {
29+
redraw: MockInstance<uPlot['redraw']>
30+
setData: MockInstance<uPlot['setData']>
31+
}
32+
33+
/**
34+
* Render the chart and wait for the uPlot instance to be created. The chart
35+
* mounts a beat after render: it waits for a real container size measurement
36+
* to arrive through ResizeObserver.
37+
*/
38+
async function renderChart(formatter: (v: number) => string) {
39+
let spies: Spies | undefined
40+
const onCreate = (u: uPlot) => {
41+
spies = { redraw: vi.spyOn(u, 'redraw'), setData: vi.spyOn(u, 'setData') }
42+
}
43+
const { rerender } = await render(
44+
<TimeSeriesChart {...props(formatter)} onCreate={onCreate} />
45+
)
46+
await vi.waitFor(() => expect(spies).toBeDefined())
47+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
48+
return { rerender, onCreate, ...spies! } // waitFor above guarantees spies is set
49+
}
50+
51+
describe('safe redrawing', () => {
52+
/*
53+
* TimeSeriesChart uses uPlot's `redraw` method to repaint when `yAxisTickFormatter` changes. This
54+
* is perfectly fine as long as it's called "the right way". Calling redraw "the wrong way" can
55+
* cause uPlot to get stuck with bad settings; in this case, that would be an x range of `null` to
56+
* `null`. That leaves the series basically unplottable, and the visible effect is a blank chart.
57+
*
58+
* This is only visible in production builds because StrictMode incidentally forces a re-create
59+
* AFTER the issue, hiding it, but these tests are fine either way, because they simply prohibit
60+
* "wrong" calls to redraw.
61+
*/
62+
const expectAllRedrawsSafe = (redraw: Spies['redraw']) => {
63+
for (const [rebuildPaths, recalcAxes] of redraw.mock.calls) {
64+
expect(rebuildPaths).toBe(false) // the important part
65+
expect(recalcAxes).toBe(true)
66+
}
67+
}
68+
69+
test('mounting never triggers an unsafe redraw', async () => {
70+
const { redraw } = await renderChart((v) => `${v}%`)
71+
expectAllRedrawsSafe(redraw)
72+
})
73+
74+
test('a new formatter triggers a safe redraw', async () => {
75+
const { rerender, onCreate, redraw } = await renderChart((v) => `${v}%`)
76+
redraw.mockClear()
77+
await rerender(<TimeSeriesChart {...props((v) => `${v} pct`)} onCreate={onCreate} />)
78+
expect(redraw).toHaveBeenCalled()
79+
expectAllRedrawsSafe(redraw)
80+
})
81+
})
82+
83+
test('rerenders only call setData when the data actually changes', async () => {
84+
const { rerender, onCreate, setData } = await renderChart((v) => `${v}%`)
85+
setData.mockClear()
86+
87+
// same data reference, new formatter: nothing for uPlot to update
88+
await rerender(<TimeSeriesChart {...props((v) => `${v} pct`)} onCreate={onCreate} />)
89+
// new reference with equal contents: uplot-react's deep compare skips the update
90+
await rerender(
91+
<TimeSeriesChart {...props((v) => `${v}%`, [...defaultData])} onCreate={onCreate} />
92+
)
93+
expect(setData).not.toHaveBeenCalled()
94+
95+
const newData = [...defaultData, { timestamp: 2000, value: 30 }]
96+
await rerender(
97+
<TimeSeriesChart {...props((v) => `${v}%`, newData)} onCreate={onCreate} />
98+
)
99+
expect(setData).toHaveBeenCalledTimes(1)
100+
})

0 commit comments

Comments
 (0)