From cf4bd43135bd43a948ccfa6cd70b1f371a21570e Mon Sep 17 00:00:00 2001 From: Agent Manager Date: Tue, 18 Aug 2026 16:45:36 +0000 Subject: [PATCH 1/2] Discover test suites instead of listing them by hand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both package.json files carried the suite list on one line — `node a.test.mjs && node b.test.mjs && …` — so every PR that added a test edited the same line and any two of them conflicted by construction; six times in the last few days. The damage is not the conflict, it is the resolution: taking one side drops the other PR's suite from the run, CI stays green, and the missing coverage is invisible. `npm test` in each package now discovers what to run. The rule is `test/*.test.mjs`, then `*.test.mjs` at the package root, alphabetically within each. A suite that must stay out of the default set says so in its own header — a line containing `am-test: manual` and the reason — and every run prints what it skipped and why, so nothing goes quiet again. That covers the five that were already excluded by omission: the four server suites needing Chromium and a full web build (terminal-ui, screenshot-input, reader-info, mobile) and web's statusMark.render, which keep their own scripts. Sequential, deliberately. `node --test` would discover the same files but runs them in parallel, and suites here bind fixed ports (migration 7893, resize 7895, trace-download 7898) and drive Chromium. Those ports are distinct today only because whoever added each picked a free number; nothing enforces it, and the first suite that copies an existing PORT constant would produce a flake that reads as a product bug. The runner spawns one child at a time and stops at the first failure with its exit code, which is what the `&&` chain did. Two server suites start running that never had: test/backup.test.mjs and test/backup-health.test.mjs, added with the bucket-backup work (#26, #34) and never referenced by any script. They are node:test files, they pass, they take 45ms between them, and they touch no ports — 33 assertions that were being carried but not run. --- scripts/run-suites.mjs | 94 +++++++++++++++++++++++++++++ server/mobile.test.mjs | 1 + server/package.json | 2 +- server/reader-info.test.mjs | 1 + server/screenshot-input.test.mjs | 1 + server/terminal-ui.test.mjs | 1 + web/package.json | 2 +- web/test/statusMark.render.test.mjs | 6 +- 8 files changed, 104 insertions(+), 4 deletions(-) create mode 100644 scripts/run-suites.mjs diff --git a/scripts/run-suites.mjs b/scripts/run-suites.mjs new file mode 100644 index 0000000..c61e0d9 --- /dev/null +++ b/scripts/run-suites.mjs @@ -0,0 +1,94 @@ +// Runs a package's test suites: every `*.test.mjs` in `test/`, then every one at +// the package root, in that order, one at a time. +// +// WHY THIS EXISTS. Both package.json files used to carry the suite list by hand +// — `node a.test.mjs && node b.test.mjs && …` on one line. Every PR that added +// a test edited that line, so any two such PRs conflicted by construction (six +// times in the last few days), and the natural resolution — take one side — +// silently drops the other PR's suite from the run. It stays green and nobody +// notices the coverage is gone. Discovery removes the shared line: adding +// `test/foo.test.mjs` is enough to make it run. +// +// SEQUENTIAL, DELIBERATELY. `node --test` would also discover these files, but +// it runs them in PARALLEL by default, and several suites here start a real +// server on a fixed port (migration on 7893, resize on 7895, trace-download on +// 7898) or drive Chromium. Those ports do not collide today, but only because +// whoever added each one picked a free number — nothing enforces it, and the +// first suite that copies an existing PORT constant produces a flake that reads +// as a product bug. This fleet has already lost time to exactly that symptom. +// One at a time costs wall-clock and nothing else. +// +// EXIT SEMANTICS match the `&&` chain it replaces: the first failing suite stops +// the run and its exit code is this process's exit code. That holds for both +// kinds of file here — the standalone scripts that print their own results and +// call process.exit, and the two `node:test` suites, which node exits non-zero +// for when a test fails (verified, not assumed). +// +// OPTING OUT. A suite that must not run in the default set says so in its own +// header, on a line containing `am-test: manual` plus the reason. It is declared +// where a reader will see it rather than by absence from a list somewhere else, +// and every run prints what it skipped and why, so coverage cannot go quiet. +// +// Usage: node ../scripts/run-suites.mjs [substring …] +// (a substring filters to matching suites — for running one by hand) +import fs from 'node:fs'; +import path from 'node:path'; +import { spawnSync } from 'node:child_process'; + +const MANUAL = /am-test:\s*manual\s*[—:-]?\s*(.*)/; +const HEAD_BYTES = 4096; // the marker belongs in the header, not line 900 + +const listDir = (dir) => { + let names = []; + try { names = fs.readdirSync(dir); } catch { return []; } + return names + .filter((n) => n.endsWith('.test.mjs')) + .sort((a, b) => a.localeCompare(b, 'en')) + .map((n) => path.join(dir, n)) + .filter((p) => fs.statSync(p).isFile()); +}; + +// `test/` first, then the package root: the root files are the older ones, and +// keeping them behind the directory keeps the common case (a new suite in +// test/) at the front of the run. +const found = [...listDir('test'), ...listDir('.')]; + +const filters = process.argv.slice(2); +const suites = []; +const skipped = []; +for (const file of found) { + let head = ''; + try { + const fd = fs.openSync(file, 'r'); + const buf = Buffer.alloc(HEAD_BYTES); + head = buf.subarray(0, fs.readSync(fd, buf, 0, HEAD_BYTES, 0)).toString('utf8'); + fs.closeSync(fd); + } catch { /* unreadable: let node report it */ } + const manual = head.match(MANUAL); + if (manual) { skipped.push({ file, why: manual[1].trim() }); continue; } + if (filters.length && !filters.some((f) => file.includes(f))) continue; + suites.push(file); +} + +const pkg = path.basename(process.cwd()); +if (!suites.length) { + console.error(`no suites found in ${pkg}/${filters.length ? ` matching ${filters.join(', ')}` : ''}`); + process.exit(1); +} +const plural = (n) => `${n} suite${n === 1 ? '' : 's'}`; +console.log(`${pkg}: ${plural(suites.length)}\n`); + +for (const [i, file] of suites.entries()) { + console.log(`── [${i + 1}/${suites.length}] ${file}`); + const r = spawnSync(process.execPath, [file], { stdio: 'inherit' }); + const code = r.status === null ? 1 : r.status; + if (code !== 0) { + console.error(`\n${file} FAILED (${r.signal ? `signal ${r.signal}` : `exit ${code}`})`); + console.error(`${plural(i)} had passed before it; the rest were not started.`); + process.exit(code); + } + console.log(''); +} + +console.log(`${pkg}: ${plural(suites.length)} passed`); +for (const { file, why } of skipped) console.log(` skipped ${file} — ${why || 'marked manual'}`); diff --git a/server/mobile.test.mjs b/server/mobile.test.mjs index ec56f15..1b165e0 100644 --- a/server/mobile.test.mjs +++ b/server/mobile.test.mjs @@ -5,6 +5,7 @@ // visual viewport is replaced with a controllable EventTarget so the keyboard // test covers both viewport height and iOS's non-zero offsetTop. // +// am-test: manual — Chromium, a full web build and port 7896; `npm run test:mobile`. // npm run test:mobile import fs from 'node:fs'; import os from 'node:os'; diff --git a/server/package.json b/server/package.json index c483c8c..29628e9 100644 --- a/server/package.json +++ b/server/package.json @@ -16,7 +16,7 @@ "test:ui": "node terminal-ui.test.mjs && node screenshot-input.test.mjs && node reader-info.test.mjs", "test:screenshots": "node screenshot-input.test.mjs", "test:mobile": "node mobile.test.mjs", - "test": "node test/archive.test.mjs && node test/trace-download.test.mjs && node test/attachments.test.mjs && node state-checkpoint.test.mjs && node test/usage.test.mjs && node test/operations.test.mjs && node test/hidden.test.mjs && node test/slowfs.test.mjs && node test/spawn-group.test.mjs && node test/revive.test.mjs && node test/repin.test.mjs && node test/codex-repin.test.mjs && node test/opencode-resume.test.mjs && node test/input-required.test.mjs && node test/terminal-modes.test.mjs && node test/trace-tail.test.mjs && node test/trace-window.test.mjs && node migration.test.mjs && node resize.test.mjs" + "test": "node ../scripts/run-suites.mjs" }, "engines": { "node": ">=20.19" diff --git a/server/reader-info.test.mjs b/server/reader-info.test.mjs index 2b26dba..f3b2315 100644 --- a/server/reader-info.test.mjs +++ b/server/reader-info.test.mjs @@ -11,6 +11,7 @@ * * Set READER_INFO_PUBLIC_DIR to a prebuilt web/dist to skip the build, and * READER_INFO_PORT to move off the default when suites run in parallel. + * am-test: manual — Chromium, a full web build and READER_INFO_PORT; `npm run test:ui`. */ import fs from 'node:fs'; import os from 'node:os'; diff --git a/server/screenshot-input.test.mjs b/server/screenshot-input.test.mjs index fe6b9c6..89783af 100644 --- a/server/screenshot-input.test.mjs +++ b/server/screenshot-input.test.mjs @@ -12,6 +12,7 @@ * - the creation dialog has no redundant file picker. * * Set SCREENSHOT_PUBLIC_DIR to a prebuilt web/dist to skip the build. + * am-test: manual — Chromium, a full web build and SCREENSHOT_PORT; `npm run test:ui`. */ import fs from 'node:fs'; import os from 'node:os'; diff --git a/server/terminal-ui.test.mjs b/server/terminal-ui.test.mjs index 6410122..ba232b5 100644 --- a/server/terminal-ui.test.mjs +++ b/server/terminal-ui.test.mjs @@ -7,6 +7,7 @@ * - ...without letting that lingering selection shadow Ctrl+C's SIGINT * * Set TERMUI_PUBLIC_DIR to a prebuilt web/dist to skip the build. + * am-test: manual — Chromium, a full web build and port 7897; `npm run test:ui`. */ import fs from 'node:fs'; import os from 'node:os'; diff --git a/web/package.json b/web/package.json index 3daa7a7..3bc22a7 100644 --- a/web/package.json +++ b/web/package.json @@ -13,7 +13,7 @@ "dev": "vite", "build": "tsc --noEmit && vite build", "typecheck": "tsc --noEmit", - "test": "node test/stepMarkdown.test.mjs && node test/statusMark.test.mjs && node test/mobileBack.test.mjs && node test/pendingExchange.test.mjs && node test/composerAlign.test.mjs && node test/fileWrapToggle.test.mjs && node test/exchanges.test.mjs && node test/sessionTitle.test.mjs && node test/overviewSort.test.mjs && node test/drafts.test.mjs && node test/settingsMobile.test.mjs && node test/traceWindows.test.mjs && node test/sidebar-dnd.test.mjs", + "test": "node ../scripts/run-suites.mjs", "test:render": "node test/statusMark.render.test.mjs", "preview": "vite preview" }, diff --git a/web/test/statusMark.render.test.mjs b/web/test/statusMark.render.test.mjs index 98a54e2..26fc440 100644 --- a/web/test/statusMark.render.test.mjs +++ b/web/test/statusMark.render.test.mjs @@ -6,8 +6,10 @@ // mark, and a state pseudo-element never paints over the inline provider/CLI // colours carried by bare `.status` dots. // -// Needs Chromium; run with: node test/statusMark.render.test.mjs -// (not in `npm test`, which stays browser-free — see package.json's test:render) +// am-test: manual — needs Chromium; run with `npm run test:render`. +// Kept out of the default suite as its own script, unchanged by the discovery +// change. (The note that used to sit here said `npm test` stays browser-free; +// that stopped being true when traceWindows.test.mjs joined it.) import assert from 'node:assert/strict'; import fs from 'node:fs'; import path from 'node:path'; From dcfa89ee9d399f4c491bdf44289feda6ed5f8a58 Mon Sep 17 00:00:00 2001 From: Agent Manager Date: Tue, 18 Aug 2026 17:24:05 +0000 Subject: [PATCH 2/2] Review: report what discovery skipped, wherever it stopped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four findings on #91, all in the runner. A `*.test.mjs` below `test/` or the package root was ignored in silence — the same coverage-goes-quiet failure this script exists to end, arriving by a different route. Discovery still stops at those two depths (so `test/fixtures/` stays fixtures), but anything suite-shaped underneath is now listed at the end of every run with the three ways out: move it up, mark it manual, or rename it. Verified with the reviewer's own probe — a `process.exit(9)` in `server/test/fixtures/nested.test.mjs` is named now instead of vanishing. The skip list only printed when the run passed: `process.exit(code)` returned before it. Both exits now go through one `report()`, so "here is what did not run" survives the moment someone is actually reading the output. `npm test -- reader-info` said no suites matched, when in fact one matched and was deliberately excluded — the filter is advertised as the run-it-by-hand path and the manual suites are exactly the ones worth running that way. It now names what matched, why it is held back, and offers `--manual`, which lets an explicit filter reach them. A bare filter still cannot drag a Chromium suite in. The fourth finding (the runner missing from the image) does not reproduce: Dockerfile:159 has copied the whole scripts/ directory since #10, and /app/scripts in the running Space holds all eight files. Answered on the PR. --- scripts/run-suites.mjs | 69 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 9 deletions(-) diff --git a/scripts/run-suites.mjs b/scripts/run-suites.mjs index c61e0d9..61cff6a 100644 --- a/scripts/run-suites.mjs +++ b/scripts/run-suites.mjs @@ -27,10 +27,19 @@ // OPTING OUT. A suite that must not run in the default set says so in its own // header, on a line containing `am-test: manual` plus the reason. It is declared // where a reader will see it rather than by absence from a list somewhere else, -// and every run prints what it skipped and why, so coverage cannot go quiet. +// and every run prints what it skipped and why — on the failure path too, since +// that is the moment someone is actually reading this output. // -// Usage: node ../scripts/run-suites.mjs [substring …] -// (a substring filters to matching suites — for running one by hand) +// DISCOVERY IS TWO DEEP, ON PURPOSE: `test/` and the package root, so a +// `test/fixtures/` directory is fixtures rather than a source of surprise runs. +// A `*.test.mjs` anywhere below that is reported at the end of every run instead +// of being ignored — a file that looks like a suite and never runs is the exact +// failure this script exists to end, and it does not matter that the cause is a +// subdirectory rather than a hand-edited list. +// +// Usage: node ../scripts/run-suites.mjs [substring …] [--manual] +// a substring filters to matching suites — for running one by hand; +// --manual lets that filter reach the suites marked manual. import fs from 'node:fs'; import path from 'node:path'; import { spawnSync } from 'node:child_process'; @@ -53,7 +62,10 @@ const listDir = (dir) => { // test/) at the front of the run. const found = [...listDir('test'), ...listDir('.')]; -const filters = process.argv.slice(2); +const args = process.argv.slice(2); +const wantManual = args.includes('--manual'); +const filters = args.filter((a) => !a.startsWith('--')); +const matches = (file) => !filters.length || filters.some((f) => file.includes(f)); const suites = []; const skipped = []; for (const file of found) { @@ -65,17 +77,55 @@ for (const file of found) { fs.closeSync(fd); } catch { /* unreadable: let node report it */ } const manual = head.match(MANUAL); - if (manual) { skipped.push({ file, why: manual[1].trim() }); continue; } - if (filters.length && !filters.some((f) => file.includes(f))) continue; + // Named explicitly with --manual, a manual suite runs: the filter is the + // run-one-by-hand path, and the suites worth running by hand are mostly these. + if (manual && !(wantManual && filters.length && matches(file))) { + if (matches(file)) skipped.push({ file, why: manual[1].trim() }); + continue; + } + if (!matches(file)) continue; suites.push(file); } +// Anything that looks like a suite but sits below the two scanned depths. +const stray = []; +(function walk(dir, depth) { + let entries = []; + try { entries = fs.readdirSync(dir, { withFileTypes: true }); } catch { return; } + for (const e of entries) { + const p = path.join(dir, e.name); + if (e.isDirectory()) { + if (['node_modules', '.git', 'dist', 'coverage'].includes(e.name)) continue; + walk(p, depth + 1); + } else if (e.name.endsWith('.test.mjs') && depth > 0 && path.dirname(p) !== 'test') { + stray.push(p); + } + } +}('.', 0)); + +const plural = (n) => `${n} suite${n === 1 ? '' : 's'}`; +// Printed by BOTH exits. What did not run is most worth saying when something +// failed, and that is exactly when an early `process.exit` used to swallow it. +const report = () => { + for (const { file, why } of skipped) console.log(` skipped ${file} — ${why || 'marked manual'}`); + for (const file of stray) { + console.log(` NOT RUN ${file} — below \`test/\` and the package root, where discovery looks.`); + console.log(' Move it up, or mark it `am-test: manual` with a reason, or rename it.'); + } +}; + const pkg = path.basename(process.cwd()); if (!suites.length) { - console.error(`no suites found in ${pkg}/${filters.length ? ` matching ${filters.join(', ')}` : ''}`); + // Blaming the filter here sends people looking for a typo when the file was + // found and deliberately excluded. + const manualOnly = skipped.length && filters.length; + console.error(manualOnly + ? `${pkg}/: ${plural(skipped.length)} matched ${filters.join(', ')}, all marked manual:` + : `no suites found in ${pkg}/${filters.length ? ` matching ${filters.join(', ')}` : ''}`); + report(); + if (manualOnly) console.error(`Run one anyway with: npm test -- ${filters.join(' ')} --manual`); process.exit(1); } -const plural = (n) => `${n} suite${n === 1 ? '' : 's'}`; console.log(`${pkg}: ${plural(suites.length)}\n`); for (const [i, file] of suites.entries()) { @@ -85,10 +135,11 @@ for (const [i, file] of suites.entries()) { if (code !== 0) { console.error(`\n${file} FAILED (${r.signal ? `signal ${r.signal}` : `exit ${code}`})`); console.error(`${plural(i)} had passed before it; the rest were not started.`); + report(); process.exit(code); } console.log(''); } console.log(`${pkg}: ${plural(suites.length)} passed`); -for (const { file, why } of skipped) console.log(` skipped ${file} — ${why || 'marked manual'}`); +report();