test: add static export E2E tests (#564)#686
Conversation
Add Playwright E2E tests for `output: "export"` static builds covering both App Router and Pages Router. Includes a lightweight static file server with path traversal protection, proper error handling, and startup validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/bigbonk review |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Static Export E2E Tests
Clean PR — well-structured tests, solid static file server, and proper CI wiring. The test coverage is good for an initial static export E2E suite (both App Router and Pages Router, including dynamic routes, metadata, __NEXT_DATA__, and 404 behavior).
One issue to address in the static file server, and a few minor suggestions below.
| const server = createServer(async (req, res) => { | ||
| try { | ||
| const parsed = new URL(req.url ?? "/", "http://localhost"); | ||
| let pathname = parsed.pathname; |
There was a problem hiding this comment.
Bug: pathname is still percent-encoded here. new URL().pathname returns percent-encoded path components (e.g., /%2e%2e/%2e%2e/etc/passwd stays encoded). However, resolve(join(rootDir, pathname)) does not decode percent-encoding — it operates on the raw string. This means:
- A request for
/blog/hello%20worldwon't match a file namedhello world.html(minor, unlikely to matter for these tests). - More importantly, a request with
%2e%2esegments (e.g.,/%2e%2e/%2e%2e/etc/passwd) would be passed toresolve(join(...))as literal%2e%2e— whichresolvetreats as a normal directory name, not... So the path traversal check still works correctly in practice.
So this is safe as-is, but the intent is clearer if you decode the pathname before joining. Consider:
| let pathname = parsed.pathname; | |
| let pathname = decodeURIComponent(parsed.pathname); |
This also makes the .html extension fallback work correctly for paths with encoded characters (e.g., spaces). Not critical for current test fixtures, but it's the more correct behavior for a file server.
| if (pathname.endsWith("/")) pathname += "index.html"; | ||
|
|
||
| // Resolve file path — try exact match, then .html extension | ||
| let filePath = resolve(join(rootDir, pathname)); |
There was a problem hiding this comment.
Nit: resolve(join(rootDir, pathname)) is redundant since resolve with an absolute first segment in the join result already normalizes. But pathname starts with /, so join(rootDir, pathname) actually works correctly (join strips the leading / from pathname when the first arg is absolute).
Just noting this is fine as-is.
| await expect(nav.locator('a[href="/products/widget"]')).toBeVisible(); | ||
| }); | ||
|
|
||
| test("client-side navigation works between pages", async ({ page }) => { |
There was a problem hiding this comment.
This test verifies client-side navigation works, which is great for a static export. One thing to note: since this is a full page navigation (no vinext runtime for SPA transitions in a static export), page.waitForURL will work fine. But if static export eventually supports client-side hydration with the router, this test's behavior would change. Worth a brief comment.
Also, consider verifying the navigation didn't produce a network error (checking response status on the target page), similar to other tests in this file.
| // lightweight static file server. No vinext runtime is needed — | ||
| // the output is pure pre-rendered HTML files. | ||
| command: | ||
| "npx tsc -p ../../../packages/vinext/tsconfig.json && node ../../../packages/vinext/dist/cli.js build && node ../../../tests/e2e/static-export/serve-static.mjs dist/client 4180", |
There was a problem hiding this comment.
The tsc step compiles the entire vinext package before building — same pattern as pages-router-prod. Just confirming: is this needed because the static-export fixture depends on vinext from source (workspace dependency), and the CLI invocation (dist/cli.js) needs the compiled output? If so, this is correct.
One concern: in CI, the e2e job already runs vp run build (line 146 of ci.yml) before Playwright. So npx tsc -p ../../../packages/vinext/tsconfig.json here is redundant in CI but necessary for local development (where you might not have run the build). The pages-router-prod project has the same pattern, so this is consistent.
| process.exit(1); | ||
| } | ||
|
|
||
| const MIME_TYPES = { |
There was a problem hiding this comment.
Nice set of MIME types. Consider adding .txt (text/plain) and .woff2 (font/woff2) for completeness — static exports sometimes include font files or robots.txt. Not blocking.
|
Review submitted successfully. Here's a summary of the feedback I posted: Overall: Clean PR with well-structured tests and solid static file server implementation. Good coverage across both App Router and Pages Router static export scenarios. Key feedback points:
|
2 participants
Summary
output: "export"static builds covering both App Router and Pages Routerserve-static.mjs) with path traversal protection, proper error handling, and startup validationstatic-exportproject inplaywright.config.tsand the CI matrixDetails
New files
tests/e2e/static-export/app-router.spec.ts— 10 tests covering home, about, blog slugs, navigation, metadata, and 404 handlingtests/e2e/static-export/pages-router.spec.ts— 7 tests covering Pages Router pages,getStaticProps/getStaticPathsdata,__NEXT_DATA__verification, and 404 for non-pre-rendered routestests/e2e/static-export/serve-static.mjs— Zero-dependency static file server with:server.on("error")handler for EADDRINUSEModified files
playwright.config.ts— Newstatic-exportproject entry (port 4180, 60s timeout).github/workflows/ci.yml— Addstatic-exportto E2E matrixTest plan
vp check(formatting + types) passes