test: add snapshot tests for entry template generators#345
test: add snapshot tests for entry template generators#345james-elicx merged 13 commits intocloudflare:mainfrom
Conversation
commit: |
Add 7 snapshot tests covering all 5 virtual entry module generators: App Router (imported directly — already exported standalone): - generateRscEntry: minimal routes, with middleware, with config - generateSsrEntry - generateBrowserEntry Pages Router (tested via pluginContainer.load on virtual modules): - virtual:vinext-server-entry - virtual:vinext-client-entry These snapshots lock down the generated code so subsequent extraction refactors (moving generators into entries/ modules) can be verified against a known baseline. Ref cloudflare#253
yunus25jmi1
force-pushed
the
test/entry-template-snapshots
branch
from
450a7df to
1d6c140
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Snapshot tests for entry template generators
This is a solid foundation for the incremental extraction work planned in #253. The approach is well-structured: App Router generators are tested directly as exported functions, and Pages Router generators are tested via pluginContainer.load() since they're closures inside the plugin. The stabilize() helper for machine-independent snapshots is a good call.
I verified all 7 tests pass and the snapshots match current output. A few observations below.
Fragility concern with Pages Router snapshots
The Pages Router snapshots depend on the tests/fixtures/pages-basic/ directory contents. Any PR that adds/removes/renames a test page in that fixture will break these snapshots (since the generated import lists and route tables change). This is technically by design for snapshot tests, but worth being aware of — it creates an indirect coupling. A comment in the test file noting this would help future contributors understand why their unrelated fixture change broke this test.
Missing instrumentationPath coverage
The generateRscEntry function accepts an instrumentationPath parameter (9th argument) that conditionally emits instrumentation import and registration code. None of the three RSC entry test cases exercise this path. Since this is a snapshot suite aimed at catching regressions during refactoring, it would be worth adding a test case with instrumentationPath set to capture that conditional code generation branch.
Minor: the AppRouterConfig test case doesn't cover allowedOrigins / allowedDevOrigins
The "with config" test case covers redirects, rewrites, and headers, but not allowedOrigins or allowedDevOrigins from the AppRouterConfig interface. These affect the generated CSRF validation and dev origin checking code. Low priority since the default empty arrays are covered by the other snapshots, but noting for completeness.
Overall
Good prerequisite for the extraction work. The snapshot file is large (9k+ lines) but that's inherent to the approach — these are full code-generated module outputs. The test structure is clean and the three RSC entry variants (minimal, middleware, config) provide reasonable axis coverage.
| const PAGES_FIXTURE_DIR = path.resolve( | ||
| import.meta.dirname, | ||
| "./fixtures/pages-basic", | ||
| ); |
There was a problem hiding this comment.
Consider noting the coupling between this fixture and the snapshots:
| ); | |
| // NOTE: Adding, removing, or renaming pages in this fixture will break the | |
| // Pages Router snapshots below. Run `pnpm test tests/entry-templates.test.ts -u` | |
| // to update them after intentional fixture changes. | |
| const PAGES_FIXTURE_DIR = path.resolve( | |
| import.meta.dirname, | |
| "./fixtures/pages-basic", | |
| ); |
| false, // no trailingSlash | ||
| ); | ||
| expect(code).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
Missing coverage for the instrumentationPath parameter — the 9th argument to generateRscEntry. When provided, it emits an import * as _instrumentation statement and the register() call block. Since the goal is regression confidence during extraction, it'd be good to have a snapshot that covers this branch.
Something like:
it("generateRscEntry snapshot (with instrumentation)", () => {
const code = generateRscEntry(
"/tmp/test/app",
minimalAppRoutes,
null,
[],
null,
"",
false,
undefined,
"/tmp/test/instrumentation.ts",
);
expect(code).toMatchSnapshot();
});
|
Review submitted. Here's a summary of my findings: All 7 tests pass and snapshots match the current generator output. Key feedback posted:
Overall the PR is a solid prerequisite for the extraction work in #253. |
- Add comment on PAGES_FIXTURE_DIR warning about snapshot coupling - Add generateRscEntry snapshot test covering instrumentationPath arg
|
/bigbonk re-review this pr please |
There was a problem hiding this comment.
Review: Snapshot tests for entry template generators
Solid prerequisite for the extraction work in #253. The test structure is well-designed: App Router generators tested directly as exported functions with synthetic fixtures, Pages Router generators tested via pluginContainer.load() since they're closures. The stabilize() helper for machine-independent snapshots is the right approach.
I see the instrumentation coverage feedback from the previous review was already incorporated — nice. 8 tests, 8 snapshots, all matching.
A few observations:
Missing globalErrorPath coverage
The generateRscEntry function has a conditional code path for globalErrorPath (the 5th parameter) that emits an import + a GlobalErrorComponent wrapping the entire page element tree inside buildPageElement() (line 966–976 of app-dev-server.ts). None of the test cases exercise this — all pass null. Since this affects the structure of buildPageElement() (it wraps the outermost element with an additional ErrorBoundary), it's worth having a snapshot to catch regressions during extraction.
Missing dynamic route coverage
All minimalAppRoutes entries are static (isDynamic: false, params: []). Dynamic routes produce different matchPattern usage at runtime, but more importantly they affect the generated route table entries (e.g., params: ["id"], isDynamic: true). A route like /blog/:slug would increase confidence that the route table serialization is stable.
The with config test doesn't exercise allowedOrigins / allowedDevOrigins
The config object omits allowedOrigins and allowedDevOrigins, so they default to [] — the same as every other snapshot. This means the generated CSRF validation and dev origin checking code paths for non-empty origin lists are uncovered. The previous review noted this as low priority, and I agree — the logic itself is tested by other test suites — but mentioning it for completeness since the AppRouterConfig interface has 5 fields and the test only exercises 3.
Minor: PR description says 7 snapshots but there are 8
The PR body says "7 snapshots" but the instrumentation test brings it to 8. A trivial nit — just updating the description would keep it accurate.
Snapshot file size
11,464 lines is large but inherent to the approach — these are full generated modules. The value justifies the cost: any refactoring that changes the generated output will immediately show up as a snapshot diff. Worth noting that snapshot updates will be required for any change to the generators, which is exactly the point.
| null, // no global error | ||
| "", // no basePath | ||
| false, // no trailingSlash | ||
| ); |
There was a problem hiding this comment.
Missing coverage for the globalErrorPath parameter (5th argument). When provided, generateRscEntry emits an import for the global error module and wraps the outermost element in buildPageElement() with an additional ErrorBoundary (lines 966–976 of app-dev-server.ts). Since this structurally changes the generated buildPageElement() function, a snapshot would catch regressions during extraction.
Something like:
| ); | |
| ); | |
| expect(code).toMatchSnapshot(); | |
| }); | |
| it("generateRscEntry snapshot (with global error)", () => { | |
| const code = generateRscEntry( | |
| "/tmp/test/app", | |
| minimalAppRoutes, | |
| null, | |
| [], | |
| "/tmp/test/app/global-error.tsx", | |
| "", | |
| false, | |
| ); | |
| expect(code).toMatchSnapshot(); | |
| }); |
| layoutTreePositions: [0], | ||
| isDynamic: false, | ||
| params: [], | ||
| }, |
There was a problem hiding this comment.
All routes in minimalAppRoutes are static (isDynamic: false, params: []). Consider adding a dynamic route fixture (e.g., /blog/:slug with isDynamic: true, params: ["slug"]) to verify that dynamic segment serialization in the generated route table is stable across refactorings. This is optional — the current coverage is reasonable for a first pass.
|
Review submitted successfully. Here's a summary of what I posted: Review submitted on PR #345 with 2 inline comments and a detailed summary. Key findings:
|
- Add dynamic route fixture (/blog/:slug) to minimalAppRoutes to cover dynamic segment serialization in the generated route table - Add generateRscEntry snapshot test covering globalErrorPath arg
|
/bigbonk re-review please! |
There was a problem hiding this comment.
Review: Snapshot tests for entry template generators
I verified the tests locally — all 9 pass and snapshots match. The test structure is well-designed: App Router generators tested as direct function calls with synthetic fixtures, Pages Router generators tested via pluginContainer.load() since they're closures. The stabilize() helper for machine-independent snapshots is the right approach.
Overall this is solid work and addresses the prerequisites for #253. A few observations:
PR description is stale
The description says "7 snapshots" but there are 9 test cases / 9 snapshots — the with instrumentation, with global error, and dynamic route (/blog/:slug) additions from previous review feedback are included in the code but the description wasn't updated. Minor, but worth fixing for anyone reading the PR later.
Snapshot file size (14,255 lines) is a maintenance consideration
Each generateRscEntry variant produces ~2,400 lines, and the 5 variants account for ~12,000 lines of the snapshot. Since the vast majority of each snapshot is identical boilerplate (the entire RSC request handler, route matching, error handling, etc.), a change to any shared code path in generateRscEntry will produce a diff touching all 5 snapshots simultaneously. This is by design for regression detection, but reviewers should be aware that legitimate one-line generator changes will produce ~12,000-line snapshot diffs.
This isn't a blocker — the value for the extraction work in #253 justifies the cost. But it's worth noting that after the extraction is complete and the generators stabilize, it may be worth considering whether all 5 RSC variants remain necessary or whether some could be consolidated.
Coverage gaps (noted by previous reviews, confirming)
The previous reviews correctly identified the missing globalErrorPath and instrumentationPath coverage — both were added and I confirmed the snapshots capture the conditional code generation properly:
- The instrumentation snapshot includes the
import * as _instrumentationandregister()call block - The global error snapshot includes the
GlobalErrorComponentErrorBoundarywrapper in bothbuildPageElement()andrenderHTTPAccessFallbackPage() - The dynamic route
/blog/:slugwithisDynamic: true, params: ["slug"]is correctly serialized
The allowedOrigins/allowedDevOrigins gap in the config test remains (3 of 5 AppRouterConfig fields exercised). I agree with the previous assessment that this is low priority since the default empty arrays are captured in every other snapshot.
One item I'd flag for follow-up
The Pages Router snapshots are tightly coupled to tests/fixtures/pages-basic/ — which is shared by 6 other test files. The comment on line 96-98 documenting this coupling was a good addition. However, since any fixture change requires updating snapshots, and pages-basic is actively used for feature development (new test pages get added there), expect this snapshot to need frequent -u updates. Consider whether the Pages Router snapshot tests should use a dedicated minimal fixture instead of the shared one, to reduce churn. Not a blocker for this PR.
| }); | ||
|
|
||
| it("generateRscEntry snapshot (with config)", () => { | ||
| const config: AppRouterConfig = { |
There was a problem hiding this comment.
The with config test exercises redirects, rewrites, and headers but omits allowedOrigins and allowedDevOrigins. Since both of these produce distinct generated code (CSRF validation logic and dev origin checking respectively), it would improve coverage to include them:
| const config: AppRouterConfig = { | |
| it("generateRscEntry snapshot (with config)", () => { | |
| const config: AppRouterConfig = { | |
| redirects: [ | |
| { source: "/old", destination: "/new", permanent: true }, | |
| ], | |
| rewrites: { | |
| beforeFiles: [ | |
| { source: "/api/:path*", destination: "/backend/:path*" }, | |
| ], | |
| afterFiles: [], | |
| fallback: [], | |
| }, | |
| headers: [ | |
| { | |
| source: "/api/:path*", | |
| headers: [{ key: "X-Custom", value: "test" }], | |
| }, | |
| ], | |
| allowedOrigins: ["https://example.com"], | |
| allowedDevOrigins: ["localhost:3001"], | |
| }; |
Low priority — the default empty arrays are captured in the other snapshots, so the risk is limited to the non-empty origin code paths.
| const resolved = await server.pluginContainer.resolveId(moduleId); | ||
| expect(resolved).toBeTruthy(); | ||
| const loaded = await server.pluginContainer.load(resolved!.id); | ||
| expect(loaded).toBeTruthy(); |
There was a problem hiding this comment.
Nit: the as any cast on the loaded result could be tightened. Vite's PluginContainer.load() returns LoadResult | null, where LoadResult is string | { code: string; ... } | null. A more precise type guard would be:
| expect(loaded).toBeTruthy(); | |
| const loaded = await server.pluginContainer.load(resolved!.id); | |
| expect(loaded).toBeTruthy(); | |
| const code = typeof loaded === "string" | |
| ? loaded | |
| : typeof loaded === "object" && loaded !== null && "code" in loaded | |
| ? loaded.code | |
| : ""; | |
| return code; |
Very minor — the as any works fine here since the expect(loaded).toBeTruthy() guard above ensures it's non-null.
|
Review submitted successfully. Here's a summary of what I posted: Review submitted on PR #345 with 2 inline comments and a detailed summary. Key findings:
|
- Add allowedOrigins and allowedDevOrigins to the 'with config' test case to cover CSRF validation and dev origin checking code paths - Replace as any cast with proper type guard for pluginContainer.load() result
Merge upstream/main which includes f1e98b9 (don't clear RSC context before lazy stream is consumed). Update 5 snapshots to reflect the replaced setHeadersContext/setNavigationContext(null) calls with comments explaining lazy stream context lifecycle.
|
/bigbonk re-review please! |
There was a problem hiding this comment.
Review: Snapshot tests for entry template generators
All 9 tests pass locally, snapshots match. The test structure is well-designed: App Router generators tested as direct function calls with synthetic fixtures (machine-independent /tmp/test/... paths), Pages Router generators tested via pluginContainer.load() since they're closures. The stabilize() helper for replacing the workspace root is the right approach.
This is a solid prerequisite for the extraction work in #253. Two substantive pieces of feedback below, and a few minor notes.
1. metadataRoutes is the only generateRscEntry parameter with zero non-default coverage
Every other parameter has at least one test exercising a non-default value, but metadataRoutes is always [] across all 5 RSC entry tests. When non-empty, this parameter triggers:
- A conditional import of
sitemapToXml, robotsToText, manifestToJsonfrommetadata-routes.js(line 226 of app-dev-server.ts) - Dynamic metadata module imports
- Static metadata file reading with
fs.readFileSync+ base64 embedding - A metadata route table with structurally different entries for dynamic vs static routes
None of this is captured by any snapshot. Since the goal is regression confidence during extraction, this is a meaningful gap — the metadata route code generation is non-trivial and would silently break during refactoring.
2. Route feature coverage in minimalAppRoutes is genuinely minimal
Every AppRoute in the fixture has these fields set to null/empty:
routePath(route handlers /route.ts)templatesparallelSlotsloadingPatherrorPath/layoutErrorPathsnotFoundPath/notFoundPathsforbiddenPath/unauthorizedPath
This means the route table serialization code for all of these features (lines 102–149 of app-dev-server.ts) only ever generates null values in the snapshot. A single route with a few of these populated (e.g., a loadingPath, an errorPath, and a notFoundPath) would capture the import generation and route table wiring for these features, which is where extraction regressions are most likely.
This isn't a blocker — the current coverage is reasonable for a first pass, and the bulk of the generated code (request handling, middleware, CSRF, RSC rendering, etc.) is well-covered. But if there's appetite for one more test case, a "kitchen sink" route fixture would close the gap cheaply.
Minor notes
- PR description says "7 snapshots" but there are 9 tests and 9 snapshots (instrumentation + global error were added from review feedback). Worth updating.
- Snapshot file is 14,315 lines. Each RSC entry variant is ~2,400 lines, and the 5 variants produce ~12,000 lines of shared boilerplate. Legitimate one-line generator changes will produce ~12,000-line snapshot diffs. This is by design and justified for the extraction work, but worth noting for reviewers.
- Pages Router fixture coupling is well-documented by the comment on lines 96–98. Good.
Overall this is ready to land with the understanding that the metadata route and rich route feature gaps are known and can be addressed incrementally.
tests/entry-templates.test.ts
Outdated
| "", // no basePath | ||
| false, // no trailingSlash | ||
| ); | ||
| expect(code).toMatchSnapshot(); |
There was a problem hiding this comment.
metadataRoutes is the only generateRscEntry parameter that isn't exercised with a non-default value in any test. When non-empty, it triggers a conditional import (sitemapToXml, robotsToText, manifestToJson), dynamic metadata module imports, and static file base64 embedding — all structurally different generated code.
Consider adding a test case with a synthetic MetadataFileRoute[]. The static variant would need a real file for fs.readFileSync (could use any fixture file), or you could test just the dynamic variant which only needs a file path for import generation:
| expect(code).toMatchSnapshot(); | |
| expect(code).toMatchSnapshot(); | |
| }); | |
| it("generateRscEntry snapshot (with metadata routes)", () => { | |
| const code = generateRscEntry( | |
| "/tmp/test/app", | |
| minimalAppRoutes, | |
| null, | |
| [ | |
| { | |
| type: "sitemap", | |
| isDynamic: true, | |
| filePath: "/tmp/test/app/sitemap.ts", | |
| servedUrl: "/sitemap.xml", | |
| contentType: "application/xml", | |
| }, | |
| ], | |
| null, | |
| "", | |
| false, | |
| ); | |
| expect(code).toMatchSnapshot(); | |
| }); |
Not a blocker — just the most impactful single addition for extraction confidence.
| layoutTreePositions: [0], | ||
| isDynamic: false, | ||
| params: [], | ||
| }, |
There was a problem hiding this comment.
The route fixtures are effective for testing the conditional code paths at the function level (middlewarePath, globalErrorPath, instrumentationPath, config), but the route table serialization itself only generates null values for every optional route feature (routePath, templates, parallelSlots, loadingPath, errorPath, notFoundPath, forbiddenPath, unauthorizedPath).
A single additional route with a few of these populated would capture the import generation + route table wiring:
{
pattern: "/dashboard",
pagePath: "/tmp/test/app/dashboard/page.tsx",
routePath: null,
layouts: ["/tmp/test/app/layout.tsx", "/tmp/test/app/dashboard/layout.tsx"],
templates: ["/tmp/test/app/dashboard/template.tsx"],
parallelSlots: [],
loadingPath: "/tmp/test/app/dashboard/loading.tsx",
errorPath: "/tmp/test/app/dashboard/error.tsx",
layoutErrorPaths: [null, "/tmp/test/app/dashboard/error.tsx"],
notFoundPath: "/tmp/test/app/dashboard/not-found.tsx",
notFoundPaths: [null, "/tmp/test/app/dashboard/not-found.tsx"],
forbiddenPath: null,
unauthorizedPath: null,
routeSegments: ["dashboard"],
layoutTreePositions: [0, 1],
isDynamic: false,
params: [],
}This is optional — the current fixture is fine for a first pass. But since every RSC entry variant reuses the same minimalAppRoutes, enriching the fixture gives coverage across all 5 snapshots at once.
|
Review submitted successfully. Here's a summary of what I posted: Review submitted on PR #345 with 2 inline comments and a detailed summary. Key findings:
Overall assessment: solid prerequisite for #253, ready to land with the understanding that the coverage gaps are known and can be addressed incrementally. |
…or/template/notFound
|
Addressed the two substantive items from the latest bonk review:
PR description updated to reflect 10 snapshots (was 7). All 10 tests pass. |
…tests, update snapshots after merge
|
/bigbonk re-review the pr please |
There was a problem hiding this comment.
Review: Snapshot tests for entry template generators
All 10 tests pass, typecheck passes, and the snapshots match current generator output. This is a well-structured prerequisite for the extraction work in #253.
What I verified
-
Test structure is sound. App Router generators tested as direct function calls with synthetic
/tmp/test/...paths; Pages Router generators tested viapluginContainer.load()since they're closures. Thestabilize()helper correctly replaces the workspace root with<ROOT>for machine-independent snapshots. -
Parameter coverage is comprehensive. All conditional code paths in
generateRscEntryare exercised by at least one test case:middlewarePath→ "with middleware" testglobalErrorPath→ "with global error" test (confirmedGlobalErrorComponentwrapper +ErrorBoundaryinbuildPageElement()andrenderHTTPAccessFallbackPage())instrumentationPath→ "with instrumentation" test (confirmedimport * as _instrumentation+register()block)metadataRoutes→ "with metadata routes" test (confirmedsitemapToXml/robotsToText/manifestToJsonimport and route table withtype: "sitemap",isDynamic: true)config→ "with config" test (confirmedallowedOrigins,allowedDevOrigins, redirects, rewrites, headers all serialized)basePath+trailingSlash→ exercised in the config test (/base,true)
-
Route fixture covers the important axes. The
minimalAppRoutesfixture includes:- Static routes (
/,/about,/dashboard) - Dynamic route (
/blog/:slugwithisDynamic: true, params: ["slug"]) — serialization verified in all RSC entry snapshots - Enriched route (
/dashboard) withloadingPath,errorPath,layoutErrorPaths,notFoundPath,notFoundPaths,templates— confirmed these generate actual import statements (e.g.,import * as mod_8 from "/tmp/test/app/dashboard/loading.tsx")
- Static routes (
-
Pages Router snapshots correctly capture the full route table from
pages-basicfixture, including static and dynamic pages. The coupling comment on lines 116-118 is helpful.
Snapshot file size
17,146 lines. Each RSC entry variant is ~2,500 lines, and 6 variants produce ~15,000 lines of mostly shared boilerplate. A one-line generator change will produce ~15,000-line snapshot diffs. This is the expected tradeoff — regression detection during extraction is worth the maintenance cost. Worth consolidating variants after #253 completes if the generators stabilize.
Minor notes (non-blocking)
Two small observations below as inline comments. Neither is a blocker.
| ], | ||
| allowedOrigins: ["https://example.com"], | ||
| allowedDevOrigins: ["localhost:3001"], | ||
| }; |
There was a problem hiding this comment.
Nit: the AppRouterConfig interface also has bodySizeLimit (defaults to 1048576). All snapshots currently show the default var __MAX_ACTION_BODY_SIZE = 1048576. Setting a non-default value here (e.g., bodySizeLimit: 5 * 1024 * 1024) would capture that the config field actually propagates to the generated code. Very low priority since the default is captured everywhere.
| routePath: null, | ||
| layouts: ["/tmp/test/app/layout.tsx", "/tmp/test/app/dashboard/layout.tsx"], | ||
| templates: ["/tmp/test/app/dashboard/template.tsx"], | ||
| parallelSlots: [], |
There was a problem hiding this comment.
Nice — this enriched /dashboard route closes the gap the previous reviews flagged about route feature coverage. The snapshot confirms it generates imports for loading.tsx, error.tsx, not-found.tsx, and template.tsx, plus non-null route table entries for all of these. The only untested AppRoute features now are routePath (route handlers), parallelSlots, forbiddenPath, and unauthorizedPath — all reasonable to defer.
|
Review submitted and approved. Here's a summary: Approved PR #345 — Snapshot tests for entry template generators. All 10 tests pass, typecheck passes, and snapshots match current generator output. The PR is a solid prerequisite for the incremental extraction work in #253. Key findings:
|
2 participants
Summary
Add snapshot tests for all 5 virtual entry module generators as a prerequisite for the incremental extraction in #253.
Per @james-elicx's feedback on #259: get snapshot tests in first, then perform each extraction separately so we have strong regression confidence.
Tests added (10 snapshots)
App Router (functions already exported from
app-dev-server.ts):generateRscEntry— minimal routes, with middleware, with config, with instrumentation, with global error, with metadata routesgenerateSsrEntrygenerateBrowserEntryPages Router (closures inside the plugin, tested via
pluginContainer.load()):virtual:vinext-server-entryvirtual:vinext-client-entryRoute fixture coverage
The
minimalAppRoutesfixture includes:/,/about,/dashboard)/blog/:slugwithisDynamic: true, params: ["slug"])/dashboard) withloadingPath,errorPath,layoutErrorPaths,notFoundPath,notFoundPaths,templatesVerification
Addresses #253
Supersedes #259 (will be broken into incremental PRs after this lands)