From e7c37d76b9da19fe1a0f264fa4678294f718b097 Mon Sep 17 00:00:00 2001 From: Stefan Andrei <89592870+sneakygriff@users.noreply.github.com> Date: Sat, 4 Jul 2026 08:16:43 +0300 Subject: [PATCH] fix(tests): stop browse/design teardowns from killing the whole bun run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several browse and design test files armed `setTimeout(() => process.exit(0), 500)` inside afterAll as a workaround for a browser close() that can hang. That assumes each test file runs in its own process. It doesn't: `bun test` runs EVERY file in one shared process. The 500ms timer therefore fired partway through a LATER test file and terminated the entire run with exit code 0 and no summary. Because the exit code was 0, the suite could not gate — only a fraction of the files ran, and every downstream failure was silently masked. The fix removes the delayed process.exit from teardown and instead time-boxes the resource cleanup: browser teardowns race `close()` against a short timeout and abandon it if it hangs (the child is reaped at real process exit), and the design daemon /shutdown test stubs `process.exit` around the shutdown timers so they fire harmlessly. A new static-guard test, test/no-suicide-exit.test.ts, scans every *.test.ts and fails if any file schedules a delayed process.exit, so the pattern cannot silently return. test/user-slug-fallback.test.ts is made deterministic via HOME isolation. Verified: test/no-suicide-exit.test.ts + test/user-slug-fallback.test.ts pass (15/15); browse/test/commands.test.ts passes standalone (243/243) and now runs to a full summary; design/test/daemon.test.ts passes (34/34) with no early exit. Honest note: with the suite now able to run to completion, it surfaces pre-existing failures that the early exit had been hiding — e.g. design/test/feedback-roundtrip.test.ts fails identically on unmodified origin/main (a `session.clearLoadedHtml` stub gap), independent of this change. These are not introduced here; this change only makes them visible so they can be gated on and fixed. Co-Authored-By: Claude Opus 4.8 --- browse/test/batch.test.ts | 9 +++- browse/test/commands.test.ts | 11 +++-- browse/test/compare-board.test.ts | 9 +++- browse/test/content-security.test.ts | 9 +++- browse/test/handoff.test.ts | 9 +++- browse/test/security-live-playwright.test.ts | 9 +++- browse/test/snapshot.test.ts | 9 +++- design/test/daemon.test.ts | 31 +++++++++---- design/test/feedback-roundtrip.test.ts | 9 +++- test/no-suicide-exit.test.ts | 49 ++++++++++++++++++++ test/user-slug-fallback.test.ts | 10 +++- 11 files changed, 135 insertions(+), 29 deletions(-) create mode 100644 test/no-suicide-exit.test.ts diff --git a/browse/test/batch.test.ts b/browse/test/batch.test.ts index 3d904a1a97..a6ee8a2b0e 100644 --- a/browse/test/batch.test.ts +++ b/browse/test/batch.test.ts @@ -42,9 +42,14 @@ beforeAll(async () => { // The test needs to start a server. Let's use the existing server infrastructure. }); -afterAll(() => { +afterAll(async () => { try { testServer.server.stop(); } catch {} - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test runs + // all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); // We need a running browse server for HTTP tests. diff --git a/browse/test/commands.test.ts b/browse/test/commands.test.ts index 9382cb27ed..506885b564 100644 --- a/browse/test/commands.test.ts +++ b/browse/test/commands.test.ts @@ -94,11 +94,14 @@ beforeAll(async () => { await bm.launch(); }); -afterAll(() => { - // Force kill browser instead of graceful close (avoids hang) +afterAll(async () => { try { testServer.server.stop(); } catch {} - // bm.close() can hang — just let process exit handle it - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test runs + // all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); // ─── Navigation ───────────────────────────────────────────────── diff --git a/browse/test/compare-board.test.ts b/browse/test/compare-board.test.ts index 0a453a43a8..b9007af7e0 100644 --- a/browse/test/compare-board.test.ts +++ b/browse/test/compare-board.test.ts @@ -69,10 +69,15 @@ beforeAll(async () => { await handleWriteCommand('goto', [boardUrl], bm); }); -afterAll(() => { +afterAll(async () => { try { server.stop(); } catch {} fs.rmSync(tmpDir, { recursive: true, force: true }); - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test runs + // all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); // ─── DOM Structure ────────────────────────────────────────────── diff --git a/browse/test/content-security.test.ts b/browse/test/content-security.test.ts index 8c760460dc..51f9e53499 100644 --- a/browse/test/content-security.test.ts +++ b/browse/test/content-security.test.ts @@ -422,9 +422,14 @@ describe('Hidden element stripping', () => { await bm.launch(); }); - afterAll(() => { + afterAll(async () => { try { testServer.server.stop(); } catch {} - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test + // runs all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); test('detects CSS-hidden elements on injection-hidden page', async () => { diff --git a/browse/test/handoff.test.ts b/browse/test/handoff.test.ts index e6754637f5..9708233e1c 100644 --- a/browse/test/handoff.test.ts +++ b/browse/test/handoff.test.ts @@ -26,9 +26,14 @@ beforeAll(async () => { await bm.launch(); }); -afterAll(() => { +afterAll(async () => { try { testServer.server.stop(); } catch {} - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test runs + // all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); // ─── Unit Tests: Failure Tracking (no browser needed) ──────────── diff --git a/browse/test/security-live-playwright.test.ts b/browse/test/security-live-playwright.test.ts index c75a115d30..b46e4b8c9b 100644 --- a/browse/test/security-live-playwright.test.ts +++ b/browse/test/security-live-playwright.test.ts @@ -56,9 +56,14 @@ describe('defense-in-depth — live Playwright fixture', () => { await bm.launch(); }); - afterAll(() => { + afterAll(async () => { try { testServer.server.stop(); } catch {} - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test + // runs all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); test('L2 — content-security.ts hidden-element stripper detects the .sneaky div', async () => { diff --git a/browse/test/snapshot.test.ts b/browse/test/snapshot.test.ts index 17b26c3d4e..d3c012eaff 100644 --- a/browse/test/snapshot.test.ts +++ b/browse/test/snapshot.test.ts @@ -31,9 +31,14 @@ beforeAll(async () => { await bm.launch(); }); -afterAll(() => { +afterAll(async () => { try { testServer.server.stop(); } catch {} - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test runs + // all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); // ─── Snapshot Output ──────────────────────────────────────────── diff --git a/design/test/daemon.test.ts b/design/test/daemon.test.ts index 65c5d7a097..4e1518fcbd 100644 --- a/design/test/daemon.test.ts +++ b/design/test/daemon.test.ts @@ -361,16 +361,27 @@ describe("daemon /shutdown", () => { await fetchHandler( req("POST", `/boards/${board.id}/api/feedback`, { regenerated: false }), ); - // Now non-done count is 0 — handler should return shuttingDown:true. - // We DON'T let the real gracefulShutdown timer fire (it calls process.exit - // after 50ms which would tear down the test runner); instead we just - // observe the immediate response. - const r = await fetchHandler(req("POST", "/shutdown")); - expect(r.status).toBe(200); - const body = (await r.json()) as any; - expect(body.shuttingDown).toBe(true); - // Reset state for subsequent tests; the shutdown timer will be a no-op - // because the next resetForTest flips shuttingDown back to false. + // The handler arms setTimeout(gracefulShutdown, 50), and gracefulShutdown + // arms setTimeout(process.exit, 50). bun test runs ALL files in one + // process, so letting that exit fire would kill the whole suite ~100ms + // later (exit 0, no summary — see test/no-suicide-exit.test.ts). Stub + // process.exit, wait past both timers so they fire harmlessly while + // stubbed, then restore. (resetForTest does NOT defuse the timers: the + // exit callback is unconditional.) + const origExit = process.exit; + (process as any).exit = (() => undefined) as any; + try { + const r = await fetchHandler(req("POST", "/shutdown")); + expect(r.status).toBe(200); + const body = (await r.json()) as any; + expect(body.shuttingDown).toBe(true); + // Let both 50ms timers (gracefulShutdown, then its process.exit) fire + // against the stub before restoring the real process.exit. + await new Promise((resolve) => setTimeout(resolve, 200)); + } finally { + (process as any).exit = origExit; + } + // Reset state for subsequent tests (gracefulShutdown set shuttingDown). resetDaemon(); }); }); diff --git a/design/test/feedback-roundtrip.test.ts b/design/test/feedback-roundtrip.test.ts index e8d63db233..4868f60d4a 100644 --- a/design/test/feedback-roundtrip.test.ts +++ b/design/test/feedback-roundtrip.test.ts @@ -121,10 +121,15 @@ beforeAll(async () => { await bm.launch(); }); -afterAll(() => { +afterAll(async () => { try { server.stop(); } catch {} fs.rmSync(tmpDir, { recursive: true, force: true }); - setTimeout(() => process.exit(0), 500); + // Close only this file's own browser — never process.exit(): bun test runs + // all files in one process, so a delayed exit kills the whole suite + // (see test/no-suicide-exit.test.ts). close() can hang when the browser + // already died, and its internal 5s timeout ties bun's 5s hook timeout — + // so race it at 3s and abandon; the child is reaped at process exit. + try { await Promise.race([bm?.close(), new Promise((resolve) => setTimeout(resolve, 3000))]); } catch {} }); // ─── The critical test: browser click → file on disk ───────────── diff --git a/test/no-suicide-exit.test.ts b/test/no-suicide-exit.test.ts new file mode 100644 index 0000000000..a820daebd5 --- /dev/null +++ b/test/no-suicide-exit.test.ts @@ -0,0 +1,49 @@ +/** + * Guard: no test file may schedule a delayed process.exit(). + * + * `bun test` runs EVERY test file in one process. The pattern of arming a + * 500ms timer in afterAll whose callback calls process.exit(0) — once used + * in several browse/design tests as a "bm.close() can hang" workaround — + * assumes each file gets its own process. It doesn't: the armed timer fires + * 500ms later, mid-way through a LATER test file, and kills the entire + * suite with exit code 0 and no summary. The truncated run silently masks + * every downstream failure (observed: only ~16 of 434 files ran, shell + * exit 0). + * + * This test statically scans every *.test.ts in the repo and fails if any + * schedules process.exit via setTimeout. Teardown must only release the + * file's own resources (e.g. `await bm.close()` — BrowserManager.close() + * is already time-boxed internally) — never terminate the shared runner. + * + * If a future test legitimately needs this pattern inside a child-process + * script (template literal passed to `bun -e`), split the child script + * into a fixture file instead of exempting it here. + */ +import { test, expect } from 'bun:test'; +import fs from 'node:fs'; +import path from 'node:path'; + +const repoRoot = path.resolve(import.meta.dir, '..'); + +// Matches a setTimeout whose arrow callback (with or without an argument) +// immediately calls process.exit. Doesn't match its own escaped source text +// (the backslashes in this regex literal prevent a literal-text match). +const DELAYED_EXIT = /setTimeout\(\s*(?:\(\s*\)|\(?\w+\)?)\s*=>\s*process\.exit\(/; + +test('no test file schedules a delayed process.exit (kills the whole bun test run)', () => { + const glob = new Bun.Glob('**/*.test.ts'); + const violations: string[] = []; + + for (const rel of glob.scanSync({ cwd: repoRoot })) { + if (rel.includes('node_modules/')) continue; + const source = fs.readFileSync(path.join(repoRoot, rel), 'utf-8'); + const lines = source.split('\n'); + for (let i = 0; i < lines.length; i++) { + if (DELAYED_EXIT.test(lines[i])) { + violations.push(`${rel}:${i + 1}: ${lines[i].trim()}`); + } + } + } + + expect(violations).toEqual([]); +}); diff --git a/test/user-slug-fallback.test.ts b/test/user-slug-fallback.test.ts index 1d8c3f9253..c7cc68f48d 100644 --- a/test/user-slug-fallback.test.ts +++ b/test/user-slug-fallback.test.ts @@ -35,6 +35,12 @@ function runConfig(args: string[], extraEnv: Record = {}): { std encoding: 'utf-8', env: { ...process.env, + // HOME isolation: endpoint_hash() reads $HOME/.claude.json for the + // gbrain MCP URL. Pointing HOME at the empty TMP_HOME makes it + // deterministically 'local' regardless of the developer's real + // ~/.claude.json (which would otherwise change the persisted key + // namespace to user_slug_at_). + HOME: TMP_HOME, ...extraEnv, }, timeout: 5000, @@ -92,7 +98,9 @@ describe('resolve-user-slug fallback chain', () => { const configFile = join(TMP_HOME, 'config.yaml'); expect(existsSync(configFile)).toBe(true); const content = readFileSync(configFile, 'utf-8'); - expect(content).toMatch(/^user_slug_at_[a-f0-9]+:\s+persisttest/m); + // HOME is isolated to the empty TMP_HOME, so endpoint_hash() is + // deterministically the literal 'local' on every machine. + expect(content).toMatch(/^user_slug_at_local:\s+persisttest/m); }); test('subsequent calls return same slug (stable across sessions)', () => {