Public: anyone can read this; the crew can change it.
A test fails when a GET page answers without named Server-Timing phases, and its allowlist only shrinks #3990
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3387-a-test-fails-when-a-get-page-answers-wit → main 12 commits fork 4 threads · 4 unresolved
1 file
changed +157
−0
test/server-timing-sweep.test.ts added +157
| @@ -0,0 +1,157 @@ | ||
| 1 | /** | |
| 2 | * Every GET page names its Server-Timing phases (#3387). The routes come from | |
| 3 | * `src/http/server.ts` itself, so a new page is swept the day it is added. | |
| 4 | * `UNNAMED` is the debt: each page that still answers with `app` alone, and | |
| 5 | * the issue that will fix it. The list may not outlive its pages: a page that | |
| 6 | * starts naming `session` and `render` must come off it. | |
| 7 | */ | |
| 8 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 9 | import { mkdtempSync, readFileSync, rmSync } from "node:fs"; | |
| 10 | import { tmpdir } from "node:os"; | |
| 11 | import { join } from "node:path"; | |
| 12 | import { createRepo } from "../src/actions/repos.ts"; | |
| 13 | import { createApp, type App } from "../src/app.ts"; | |
| 14 | import { createUser } from "../src/core/users.ts"; | |
| 15 | import { buildRouter, serve, type Servers } from "../src/http/server.ts"; | |
| 16 | import { proveAll } from "./helpers/verified.ts"; | |
| 17 | import { quietLogs } from "./helpers/globals.ts"; | |
| 18 | ||
| 19 | quietLogs(); | |
| 20 | ||
| 21 | /** Pages without named phases yet → the issue that will name them. */ | |
| 22 | const UNNAMED: Array<[path: string, issue: string]> = [ | |
| 23 | ["/-/confirm", "#3390"], | |
| 24 | ["/alice/forge", "#3325"], | |
| 25 | ["/alice/forge/app", "#3390"], | |
| 26 | ["/alice/forge/app/answers", "#3390"], | |
| 27 | ["/alice/forge/app/now", "#3390"], | |
| 28 | ["/alice/forge/app/state", "#3390"], | |
| 29 | ["/alice/forge/changes", "#3323"], | |
outdated
Nearly every entry points at the umbrella #3390. Per-page issues would give each line an owner. |
||
| 30 | ["/alice/forge/issues/new", "#3390"], | |
| 31 | ["/alice/forge/make-it-yours", "#3390"], | |
| 32 | ["/alice/forge/map", "#3320"], | |
| 33 | ["/alice/forge/report", "#3390"], | |
| 34 | ["/alice/forge/settings", "#3390"], | |
| 35 | ["/alice/forge/settings/danger", "#3390"], | |
| 36 | ["/alice/forge/settings/hooks", "#3390"], | |
| 37 | ["/alice/forge/settings/labels", "#3390"], | |
| 38 | ["/alice/forge/settings/prompts", "#3390"], | |
| 39 | ["/alice/forge/tells", "#3322"], | |
| 40 | ["/alice/forge/toolbar", "#3390"], | |
| 41 | ["/apps/open", "#3390"], | |
| 42 | ["/board", "#3378"], | |
| 43 | ["/boards", "#3390"], | |
| 44 | ["/claim", "#3390"], | |
| 45 | ["/front", "#3311"], | |
| 46 | ["/live/apps", "#3390"], | |
| 47 | ["/live/on-you", "#3390"], | |
| 48 | ["/organizations/new", "#3390"], | |
| 49 | ["/recover", "#3390"], | |
| 50 | ["/settings/cloud/audit", "#3390"], | |
| 51 | ["/settings/keys", "#3390"], | |
| 52 | ["/settings/password", "#3390"], | |
| 53 | ["/settings/tokens", "#3390"], | |
| 54 | ["/settings/usage", "#3279"], | |
| 55 | ["/signin", "#3390"], | |
| 56 | ["/signin/parent", "#3390"], | |
| 57 | ["/signin/parent/callback", "#3390"], | |
| 58 | ["/signup", "#3390"], | |
| 59 | ["/verify", "#3390"], | |
| 60 | ]; | |
| 61 | ||
| 62 | /** Long-lived streams and live fragments: no page, no phases to name. */ | |
| 63 | const NOT_PAGES = new Set([ | |
| 64 | "/events", | |
| 65 | "/system/stream", | |
| 66 | "/settings/teams/stream", | |
| 67 | "/fleet/graph/live", | |
| 68 | "/settings/backups/live", | |
| 69 | "/:owner/:name/stream", | |
| 70 | ]); | |
| 71 | ||
| 72 | /** Every GET pattern the router source registers, one-line or wrapped. */ | |
| 73 | function registeredGets(): Set<string> { | |
| 74 | const source = readFileSync(join(import.meta.dir, "../src/http/server.ts"), "utf8"); | |
| 75 | const found = new Set<string>(); | |
| 76 | for (const m of source.matchAll(/router\.add\(\s*"GET",\s*"([^"]+)"/g)) found.add(m[1]!); | |
| 77 | return found; | |
| 78 | } | |
| 79 | ||
| 80 | /** Parameter-free GET patterns, plus `/:owner/:name/<literal>…` ones. */ | |
| 81 | function sweptPatterns(): string[] { | |
| 82 | const registered = new Set(buildRouter().patterns()); | |
| 83 | return [...registeredGets()] | |
| 84 | .filter((p) => registered.has(p) && !NOT_PAGES.has(p) && !p.startsWith("/assets/")) | |
outdated
This regex reads source text. The second test covers wrapped calls, but any other registration style, such as a helper or a loop, would be missed silently. Prefer enumerating GET patterns from the router (buildRouter().patterns() with method info) if it can expose them. |
||
| 85 | .filter((p) => !p.replace(/^\/:owner\/:name/, "").includes(":") && !p.includes("*")) | |
| 86 | .filter((p) => !p.endsWith("/live") && !p.includes("/__forge/")) | |
| 87 | .sort(); | |
| 88 | } | |
| 89 | ||
| 90 | let root = ""; | |
| 91 | let app: App; | |
outdated
Scraping server.ts with a regex is brittle. A router API listing GET patterns would remove the need for the guard test. |
||
| 92 | let servers: Servers; | |
| 93 | ||
| 94 | beforeAll(async () => { | |
| 95 | root = mkdtempSync(join(tmpdir(), "forge-timing-sweep-")); | |
| 96 | app = createApp({ dataDir: root, port: 0, forgeRepo: "alice/forge" }); | |
| 97 | const u = await createUser(app.db, { | |
| 98 | origin: "seed", | |
| 99 | trusted: true, | |
| 100 | username: "alice", | |
| 101 | email: "[email protected]", | |
| 102 | password: "a-good-passphrase", | |
| 103 | adminIfFirst: true, | |
| 104 | }); | |
| 105 | if (!u.ok) throw new Error(u.error); | |
| 106 | proveAll(app); | |
| 107 | const made = await createRepo(app, { id: u.value.id, username: "alice" } as never, { name: "forge" }); | |
| 108 | if (!made.ok) throw new Error(made.error); | |
| 109 | servers = await serve(app); | |
| 110 | }); | |
| 111 | afterAll(() => { | |
| 112 | servers.stop(); | |
| 113 | app.close(); | |
| 114 | rmSync(root, { recursive: true, force: true }); | |
| 115 | }); | |
| 116 | ||
| 117 | test("every GET page names session and render in Server-Timing, bar the listed debt", async () => { | |
| 118 | const signin = await fetch(`${servers.url}/signin`, { | |
| 119 | method: "POST", | |
| 120 | headers: { origin: servers.url }, | |
| 121 | body: new URLSearchParams({ username: "alice", password: "a-good-passphrase" }), | |
| 122 | redirect: "manual", | |
| 123 | }); | |
| 124 | const cookie = (signin.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 125 | const paths = sweptPatterns().map((p) => p.replace(":owner", "alice").replace(":name", "forge")); | |
| 126 | expect(paths.length).toBeGreaterThan(20); | |
| 127 | ||
| 128 | const named: string[] = []; | |
| 129 | const bare: string[] = []; | |
| 130 | await Promise.all( | |
| 131 | paths.map(async (path) => { | |
| 132 | const res = await fetch(`${servers.url}${path}`, { | |
| 133 | headers: { cookie }, | |
| 134 | redirect: "manual", | |
| 135 | signal: AbortSignal.timeout(5000), | |
| 136 | }); | |
| 137 | await res.arrayBuffer(); | |
| 138 | if (!(res.headers.get("content-type") ?? "").startsWith("text/html")) return; | |
| 139 | const names = (res.headers.get("server-timing") ?? "").split(",").map((p) => p.trim().split(";")[0]); | |
| 140 | expect(names).toContain("app"); | |
| 141 | (names.includes("session") && names.includes("render") ? named : bare).push(path); | |
| 142 | }), | |
| 143 | ); | |
| 144 | ||
| 145 | const debt = new Set(UNNAMED.map(([p]) => p)); | |
| 146 | expect(bare.filter((p) => !debt.has(p)).sort()).toEqual([]); | |
| 147 | expect(named.filter((p) => debt.has(p)).sort()).toEqual([]); | |
| 148 | for (const [, issue] of UNNAMED) expect(issue).toMatch(/^#\d+$/); | |
| 149 | }); | |
| 150 | ||
| 151 | test("the sweep reads wrapped router.add registrations, so a reformat cannot shrink it", () => { | |
| 152 | const found = registeredGets(); | |
| 153 | expect(found.has("/origins/:id/r/:owner/:name/commit/:oid")).toBe(true); // wrapped over four lines | |
| 154 | expect(found.has("/robots.txt")).toBe(true); // one line | |
| 155 | const registered = new Set(buildRouter().patterns()); | |
| 156 | expect([...found].filter((p) => !registered.has(p))).toEqual([]); | |
| 157 | }); |
new above
offline
About thirty entries share #3390. Break it into per-page or per-area issues so the debt is traceable and shrinks one page at a time.