test: add production coverage for global-error compat#556
Conversation
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Good direction — locking in passing behavior with production preview coverage is worthwhile. Two issues to address before merging.
Summary tables not updated
The Chunk 3 detailed table (lines 78-92) now correctly shows 6/6 pass, 0 skip. But there are two other summary tables in the same file that still say 3 pass, 3 skip for chunk 3:
- "Early snapshot" summary (line 67):
| 3. global-error | 11 | 3 | 3 | 5 | 0 | Done | - "Overall Summary" table (line 358):
| 3. global-error | 11 | 3 | 3 | 5 | 0 | Done | - Overall total (line 374): shows
6 skiptotal — should now be3since 3 global-error skips are resolved
These need to be updated to match the detailed table.
Missing production preview tests for routes #3 and #4
The tracker now says tests #3 (global-error-rsc) and #4 (global-error-ssr) are "Verified in dev" and marked PASS, but the production preview block only covers 4 of the 6 routes. The two global-error.tsx fallback routes (/nextjs-compat/global-error-rsc and /nextjs-compat/global-error-ssr) are absent from the production preview describe. Either add them there too for consistency, or change the tracker notes for #3 and #4 to say "Verified in dev only" to be precise about coverage scope.
Otherwise the test code itself looks correct and follows the existing createBuilder + preview pattern from app-router.test.ts.
tests/nextjs-compat/TRACKING.md
Outdated
| | --- | --------------------------------------------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------------------------- | | ||
| | 1 | error-server-test: server component throw caught by error.tsx | PASS | Verified in dev + production preview: vinext renders the local `error.tsx` boundary with HTTP 200. | | ||
| | 2 | error-nested-test: nested error caught by inner error.tsx | PASS | Verified in dev + production preview: vinext resolves to the nearest matching `error.tsx`, not the parent boundary. | | ||
| | 3 | Server component throw without local error.tsx returns a response | PASS | Verified in dev: vinext renders `global-error.tsx` with HTTP 200 when no local boundary exists. | |
There was a problem hiding this comment.
Tests #3 and #4 say "Verified in dev" but neither /nextjs-compat/global-error-rsc nor /nextjs-compat/global-error-ssr appears in the new production preview describe block. The production tests only cover the 4 local-boundary routes (/error-server-test, /error-nested-test/child, /nextjs-compat/metadata-error-with-boundary, /nextjs-compat/metadata-error-without-boundary).
Consider either:
- Adding production preview assertions for these two routes as well (for parity with the claim), or
- Updating the notes here to say "Verified in dev only" so the tracker accurately reflects what's tested.
| | 10 | Client-side notFound() trigger from button (root) | N/A | Requires Playwright | | ||
| | 11 | Client-side notFound() trigger from button (nested) | N/A | Requires Playwright | | ||
|
|
||
| **Result: 6/6 pass (HTTP-level), 0 skip, 5 N/A (browser-only, dev overlay)** |
There was a problem hiding this comment.
This line correctly shows 6/6 pass, 0 skip. But the two summary tables elsewhere in this file were not updated:
- Line 67 ("early snapshot" summary): still says
3 pass, 3 skip - Line 358 ("Overall Summary"): still says
3 pass, 3 skip - Line 374 (total row): still says
6 skip— should be3
Please update all three for consistency.
|
Review submitted successfully. Here's a summary of the feedback: Requested changes on PR #556:
The test code itself is well-structured and follows the existing |
|
Addressed the requested changes in
Verification:
|
|
/bigbonk re-review |
There was a problem hiding this comment.
Good PR — the production preview coverage is well-structured and the tracker updates are mostly correct. One minor inconsistency remains from the previous review round.
The test code itself is solid: assertions are grounded in real implementation strings (the sanitized error message from app-rsc-entry.ts:527), negative assertions prevent false positives (e.g., not.toContain("server page error") to verify message sanitization in prod), and the createBuilder + preview pattern matches the existing convention in app-router.test.ts.
tests/nextjs-compat/TRACKING.md
Outdated
| | --- | --------------------------------------------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------------------------- | | ||
| | 1 | error-server-test: server component throw caught by error.tsx | PASS | Verified in dev + production preview: vinext renders the local `error.tsx` boundary with HTTP 200. | | ||
| | 2 | error-nested-test: nested error caught by inner error.tsx | PASS | Verified in dev + production preview: vinext resolves to the nearest matching `error.tsx`, not the parent boundary. | | ||
| | 3 | Server component throw without local error.tsx returns a response | PASS | Verified in dev: vinext renders `global-error.tsx` with HTTP 200 when no local boundary exists. | |
There was a problem hiding this comment.
The tracker notes for #3 and #4 still say "Verified in dev" but the test file now includes production preview assertions for both /nextjs-compat/global-error-rsc (line 212) and /nextjs-compat/global-error-ssr (line 220). These should say "Verified in dev + production preview" to match rows #1, #2, #5, and #6.
| | 3 | Server component throw without local error.tsx returns a response | PASS | Verified in dev: vinext renders `global-error.tsx` with HTTP 200 when no local boundary exists. | | |
| | 3 | Server component throw without local error.tsx returns a response | PASS | Verified in dev + production preview: vinext renders `global-error.tsx` with HTTP 200 when no local boundary exists. | |
tests/nextjs-compat/TRACKING.md
Outdated
| | 1 | error-server-test: server component throw caught by error.tsx | PASS | Verified in dev + production preview: vinext renders the local `error.tsx` boundary with HTTP 200. | | ||
| | 2 | error-nested-test: nested error caught by inner error.tsx | PASS | Verified in dev + production preview: vinext resolves to the nearest matching `error.tsx`, not the parent boundary. | | ||
| | 3 | Server component throw without local error.tsx returns a response | PASS | Verified in dev: vinext renders `global-error.tsx` with HTTP 200 when no local boundary exists. | | ||
| | 4 | Client component SSR throw without local error.tsx returns a response | PASS | Verified in dev: vinext renders `global-error.tsx` with HTTP 200 for SSR-time client component throws. | |
There was a problem hiding this comment.
Same as above — production preview coverage exists in the test file but the note doesn't reflect it.
| | 4 | Client component SSR throw without local error.tsx returns a response | PASS | Verified in dev: vinext renders `global-error.tsx` with HTTP 200 for SSR-time client component throws. | | |
| | 4 | Client component SSR throw without local error.tsx returns a response | PASS | Verified in dev + production preview: vinext renders `global-error.tsx` with HTTP 200 for SSR-time client component throws. | |
|
Review submitted. Here's a summary: Overall: The PR is in good shape. The production preview tests are well-written — assertions use real implementation strings from One minor issue: The tracker notes for tests #3 and #4 ( |
… reflect production preview coverage
|
/bigbonk re-review |
There was a problem hiding this comment.
Looks good. The production preview tests are well-written and follow the established createBuilder + preview pattern from app-router.test.ts. Assertions are grounded in real implementation behavior — the sanitized error message from app-rsc-entry.ts:527, negative assertions against raw error messages leaking in prod, and correct boundary resolution. Tracker math is correct (362 → 365 pass, 6 → 3 skip). All CI checks pass.
One minor nit (not blocking): The "Combined Key Metrics" section at TRACKING.md:396-397 still says "400 tests passing (362 Vitest + 38 Playwright)" and "11 tests skipped (6 Vitest + 5 Playwright)". With the changes in this PR, these should be "403 tests passing (365 Vitest + 38 Playwright)" and "8 tests skipped (3 Vitest + 5 Playwright)". The actual summary tables are correct — this is just the prose section further down that's out of sync.
|
Review submitted. Here's a summary of my review: Approved the PR. The changes are solid:
One minor nit: The "Combined Key Metrics" prose section at |
Summary
This updates the App Router
global-errorcompat coverage to reflect current vinext behavior and adds production-preview assertions for the HTTP-level cases that were previously tracked as skipped.What Changed
tests/nextjs-compat/global-error.test.tsfor:error.tsxon server component errorserror.tsxerror.tsxforgenerateMetadata()errorsglobal-error.tsxfallback when metadata has no local boundarytests/nextjs-compat/TRACKING.mdto mark those cases as passing instead of skippedupstream/mainWhy It Matters
The tracker was claiming a meaningful Next.js compatibility bug around handled errors returning
500instead of200. The current implementation already matches Next.js for the server-side HTTP semantics here, so the right fix is to lock that behavior in with direct coverage and stop advertising a stale regression.Risks Or Limits
global-errorinteractions from the upstream Next.js suiteVerification
vp test tests/nextjs-compat/global-error.test.ts