From 75f514c3f8f6fa1f5ffc7052e450a634085cb6f6 Mon Sep 17 00:00:00 2001 From: TurtleWolfe Date: Thu, 20 Aug 2026 04:25:24 +0000 Subject: [PATCH] fix(#396): an accessibility spec proved a setting survives navigating to the 404 page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `colorblind-toggle.spec.ts` navigated to `/about` to prove a colour-vision setting survives a page change. **This app has never had an `/about` route.** So "navigate to another page" navigated to `not-found.tsx`, and the persistence the test claims to prove was only ever proven across the error page. It passed 7/7 either way, because the chrome it asserts on renders there too. That is #396's "an E2E test that lands on the 404 route" class: nothing errors, the spec keeps running, and a green result means something weaker than it reads. Pointed at `/blog/`, which exists. Still 7/7 against production — the fix is a strict improvement, not a repair of a broken test. THE GUARD is static, and that is a deliberate scope choice. #396 asks to "fail loudly when an E2E test lands on the 404 route". A runtime fixture wrapping every navigation would catch more — including computed paths like `goto(\`/blog/${slug}\`)` — but it changes behaviour for every spec on the REQUIRED lane, and sweeping the suite found exactly ONE real instance. The zero-risk half is worth having now; the runtime half stays recorded in #396 rather than being smuggled in behind a one-line bug fix. So `e2e-gotos-resolve.test.js` enumerates the app router's real routes (handling `(groups)`, `[dynamic]` and `[...catchall]`) and asserts every literal `goto('/...')` in the suite resolves to one. Two paths are allowlisted, each with a reason: `/non-existent-page` (the test IS "404 page handles non-existent routes") and color-contrast's `TEMPLATE_PROBES` path, which must reach not-found.tsx deliberately because a template has no URL of its own (#425). The allowlist requires a reason string longer than a token, asserted by the test, so it cannot rot into a place where real mistakes get parked. Stated in the file so a green run is not over-read: this cannot see computed paths, redirects that end on a 404, or routes that exist as files but fail to build. Mutation-verified, mutant confirmed present first, two ways: restoring the `/about` navigation (caught, naming `colorblind-toggle.spec.ts:174 -> /about`), and emptying the allowlist (caught, proving the allowlist is load-bearing rather than decorative). test:scripts 424/424; type-check and lint clean. Refs #396 --- scripts/__tests__/e2e-gotos-resolve.test.js | 183 ++++++++++++++++++ .../accessibility/colorblind-toggle.spec.ts | 7 +- 2 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 scripts/__tests__/e2e-gotos-resolve.test.js diff --git a/scripts/__tests__/e2e-gotos-resolve.test.js b/scripts/__tests__/e2e-gotos-resolve.test.js new file mode 100644 index 00000000..cb7e5ec9 --- /dev/null +++ b/scripts/__tests__/e2e-gotos-resolve.test.js @@ -0,0 +1,183 @@ +/** + * Every literal `page.goto('/path')` in the E2E suite must name a real route (#396). + * + * WHY. A spec that navigates to a path the app does not have lands on `not-found.tsx` + * and keeps going. Nothing errors: assertions about chrome (nav, footer, theme, a + * persisted setting) still pass there, so the spec stays green while measuring the + * error page instead of the subject it names. + * + * THIS IS NOT HYPOTHETICAL, THREE TIMES OVER: + * + * - `colorblind-toggle.spec.ts` navigated to `/about` — a route this app has never + * had — to prove a colour-vision setting survives navigation. It proved it + * survives navigating to the 404 page. Found by this check. + * - #425: PR #420 added `/docs/[slug]` with no INSTANCES entry, so the contrast + * sweep visited the literal path, 404'd, and reported a violation against + * `/docs/[slug]` when the real subject was `not-found.tsx`. + * - While investigating #842, three consecutive measurements were of the 404 page + * because `goto('/')` under a basePath BASE_URL resolves to the server root. A + * plausible `footer a = 3` was the 404 page's own footer. + * + * WHY STATIC AND NOT A RUNTIME FIXTURE. #396 asks for "fail loudly when an E2E test + * lands on the 404 route". A fixture wrapping every navigation would catch more — + * including computed paths this cannot see — but it changes behaviour for every spec + * on the REQUIRED lane, and this sweep found only one real instance. The cheap, + * zero-risk half is worth having first; the runtime half is recorded in #396. + * + * WHAT THIS CANNOT CHECK, so a green run is not over-read: computed paths + * (`goto(\`/blog/${slug}\`)`), redirects that land on a 404, and routes that exist as + * files but fail to build. It checks literal paths against the route table. + */ +'use strict'; + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..', '..'); +const APP = path.join(ROOT, 'src', 'app'); +const E2E = path.join(ROOT, 'tests', 'e2e'); + +/** + * Paths a spec navigates to ON PURPOSE to exercise the 404 template. Each needs a + * reason, so the list cannot quietly become a dumping ground for real mistakes. + */ +const DELIBERATE_404 = { + '/non-existent-page': + 'cross-page-navigation.spec.ts — the test IS "404 page handles non-existent routes"', + '/__contrast-probe-unmatched-route__/': + 'color-contrast.spec.ts TEMPLATE_PROBES — not-found.tsx has no URL of its own, so the sweep must reach it deliberately (#425)', +}; + +/** Routes the app router actually serves, from `page.tsx` files. */ +function routes(dir = APP, prefix = '') { + const out = []; + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { + if (e.isDirectory()) { + // `(group)` segments do not appear in the URL. + const seg = /^\(.*\)$/.test(e.name) ? '' : `/${e.name}`; + out.push(...routes(path.join(dir, e.name), prefix + seg)); + } else if (e.name === 'page.tsx') { + out.push(prefix || '/'); + } + } + return out; +} + +const norm = (p) => p.split('?')[0].split('#')[0].replace(/\/+$/, '') || '/'; + +function resolves(p, table) { + const t = norm(p); + return table.some((r) => { + const rr = norm(r); + if (rr === t) return true; + if (!rr.includes('[')) return false; + const pat = + '^' + + rr + .split('/') + .map((s) => + /^\[\.\.\..+\]$/.test(s) + ? '.+' + : /^\[.+\]$/.test(s) + ? '[^/]+' + : s.replace(/[.*+?^${}()|\\]/g, '\\$&') + ) + .join('/') + + '$'; + return new RegExp(pat).test(t); + }); +} + +/** Every literal `.goto('/...')` in the suite. Template literals are skipped. */ +function gotos() { + const out = []; + const walk = (dir) => { + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { + const p = path.join(dir, e.name); + if (e.isDirectory()) walk(p); + else if (/\.ts$/.test(e.name)) { + fs.readFileSync(p, 'utf8') + .split('\n') + .forEach((line, i) => { + for (const m of line.matchAll(/\.goto\(\s*['"](\/[^'"$`]*)['"]/g)) { + out.push({ + file: path.relative(ROOT, p), + line: i + 1, + target: m[1], + }); + } + }); + } + } + }; + walk(E2E); + return out; +} + +describe('E2E navigations name real routes (#396)', () => { + it('finds a real route table and real navigations', () => { + // Non-vacuity, both sides. "Nothing 404s" is trivially true of an empty route + // table (everything fails) or an empty goto list (nothing is checked) — and the + // second would pass silently, which is the shape this file exists to catch. + assert.ok( + routes().length >= 20, + `only ${routes().length} routes enumerated` + ); + assert.ok( + gotos().length >= 30, + `only ${gotos().length} literal goto() calls found` + ); + }); + + it('no spec navigates to a path the app does not serve', () => { + const table = routes(); + const bad = gotos() + .filter((g) => !resolves(g.target, table)) + .filter( + (g) => !(norm(g.target) in DELIBERATE_404 || g.target in DELIBERATE_404) + ); + + assert.deepEqual( + bad.map((g) => `${g.file}:${g.line} -> ${g.target}`), + [], + 'these navigations land on not-found.tsx. The spec keeps running and its ' + + 'assertions about chrome still pass, so it stays green while measuring the ' + + 'error page instead of its subject (#396). Point it at a real route, or add ' + + 'it to DELIBERATE_404 with a reason if the 404 IS the subject.' + ); + }); + + it('the detector can actually fail', () => { + const table = ['/', '/blog', '/blog/[slug]', '/docs']; + assert.equal(resolves('/blog', table), true); + assert.equal( + resolves('/blog/', table), + true, + 'a trailing slash is the same route' + ); + assert.equal( + resolves('/blog/anything', table), + true, + 'dynamic segment matches' + ); + assert.equal(resolves('/about', table), false, 'the real bug this found'); + assert.equal( + resolves('/blog/a/b', table), + false, + 'one dynamic segment is not two' + ); + assert.equal( + resolves('/?x=1', table), + true, + 'a query string is not part of the route' + ); + + // The allowlist must be reasons, not bare strings — otherwise it rots into a + // place where real mistakes get parked. + for (const [p, why] of Object.entries(DELIBERATE_404)) { + assert.ok(why.length > 30, `DELIBERATE_404['${p}'] needs a real reason`); + } + }); +}); diff --git a/tests/e2e/accessibility/colorblind-toggle.spec.ts b/tests/e2e/accessibility/colorblind-toggle.spec.ts index a8d74631..4d8960c5 100644 --- a/tests/e2e/accessibility/colorblind-toggle.spec.ts +++ b/tests/e2e/accessibility/colorblind-toggle.spec.ts @@ -167,8 +167,11 @@ test.describe('Colour vision controls (Display popover) - Accessibility', () => }); await page.waitForTimeout(300); - // Navigate to another page - await page.goto('/about'); + // `/blog/`, a route that exists. This said `/about`, which this app has never + // had — so "navigate to another page" navigated to the 404 template, and the + // persistence this test claims to prove was only ever proven across an error + // page (#396's "an E2E test that lands on the 404 route" class). + await page.goto('/blog/'); await page.waitForLoadState('networkidle'); // Return to home