Add browser coverage for human-readable routes - #252
Conversation
MattIPv4
left a comment
MattIPv4
left a comment
There was a problem hiding this comment.
Thanks for working on this! Instead of unstable_dev, I wonder if we can use the new createTestHarness designed for use w/ Playwright etc.? https://developers.cloudflare.com/changelog/post/2026-07-21-integration-test-harness/
|
Addressed both suggestions: the browser suite now uses Fresh current-base checks pass: Chromium 2/2 twice, unit tests 485/485, types, lint, focused formatting, a Wrangler dry run, and |
There was a problem hiding this comment.
Pull request overview
Adds real-browser coverage for human-readable routes through a local Worker and Vite proxy, plus cross-platform island bundling support.
Changes:
- Adds Playwright browser tests for API documentation and library filtering.
- Configures local Worker lifecycle, proxying, and dynamic ports.
- Normalizes Vite paths and runs browser tests in CI.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vitest.config.ts |
Excludes browser specs from unit tests. |
vitest.browser.setup.ts |
Manages the browser-test Worker lifecycle. |
vitest.browser.config.ts |
Configures Playwright, proxying, and ports. |
vite.client.config.ts |
Normalizes island module paths. |
src/utils/spec/browser.ts |
Provides the iframe route helper. |
src/routes/library.browser.spec.ts |
Tests library-page hydration and filtering. |
src/routes/api.browser.spec.ts |
Tests hydrated API documentation. |
package.json |
Adds browser tooling and test command. |
package-lock.json |
Locks new browser dependencies. |
.github/workflows/ci.yml |
Installs Chromium and runs browser tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (externalApiUrl) { | ||
| process.env.VITEST_BROWSER_WORKER_URL = externalApiUrl; | ||
| return; |
MattIPv4
left a comment
MattIPv4
left a comment
There was a problem hiding this comment.
👋 Thanks for getting this updated. Having explored this more locally, and how the proxy that Vite provides works, I'm a bit concerned by the security model here as we do testing of remote origins in our CI -- it seems it would be possible for a remote origin we're testing to use the Vite proxy to access files on disk etc.
I wonder if we'd be better off using regular ol' Playwright? I think the setup would be similar using createTestHarness, but wouldn't need the proxy setup nor the iframe, as we'd be able to hit the local/remote worker URL via direct page navigation?
| try { | ||
| const { url } = await server.listen(); | ||
| if (!url.origin.startsWith(websiteBase)) { | ||
| throw new Error( | ||
| `Local Worker origin ${url.origin} does not match website base ${websiteBase}.`, | ||
| ); | ||
| } | ||
|
|
||
| const workerEnv = await server | ||
| .getWorker<{ WEBSITE_BASE: string }>() | ||
| .getEnv(); | ||
| if (workerEnv.WEBSITE_BASE !== websiteBase) { | ||
| throw new Error( | ||
| `Local Worker WEBSITE_BASE is ${workerEnv.WEBSITE_BASE}, expected ${websiteBase}.`, | ||
| ); | ||
| } | ||
|
|
||
| const response = await fetch(new URL('/health', url)); | ||
| const body = await response.text(); | ||
| if (!response.ok || body !== 'OK') { | ||
| throw new Error( | ||
| `Local website health check failed with status ${String(response.status)} and body ${JSON.stringify(body)}.`, | ||
| ); | ||
| } | ||
|
|
||
| await use(server); | ||
| } finally { | ||
| await server.close(); | ||
| } |
There was a problem hiding this comment.
I don't think we need to spin up the server and do a health check here; the individual tests will naturally fail if the server is unhealthy.
Type of Change
What issue does this relate to?
Fixes #188
What should this PR do?
@playwright/testserver fixture using Wrangler'screateTestHarness(), and reset the harness after each test.baseURLand navigate directly to/apiand/libraries/backbone.js/1.1.0.WEBSITE_BASEfor the dynamic local origin according to the Worker's prefix-based website detection.What are the acceptance criteria?
npm testnpm run test:browsernpm run lintnpm run typesnpm run formatnpx wrangler deploy --dry-runThis changes test infrastructure and coverage only; it does not change production behavior or the rendered UI, so before/after screenshots are not applicable.