From c25dafaa97bb3703b685fa102cd6fa4e492fccb3 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 19 Jul 2026 03:42:12 +0000 Subject: [PATCH 1/3] docs: add test reliability and speed guidance to CLAUDE.md and REVIEW.md CI analysis of recent PR builds showed recurring test anti-patterns that the contributor docs do not warn against: large setDefaultTimeout values, fixed sleep() waits, tests hitting the public registry, and leak tests that assert absolute RSS deltas. CLAUDE.md gains four bullets in the Writing Tests section (speed budget, no public internet, setDefaultTimeout as ceiling, leak-test methodology) and the existing no-setTimeout rule now covers await sleep(N) explicitly. REVIEW.md extends four existing bullets in Tests reviewers reject rather than adding new ones: literal sleep needs a justifying comment, external- service tests skipIf and deadline their setup, RSS checks compare growth rate not absolute deltas, and slow new files get scrutinized for test.concurrent. --- CLAUDE.md | 6 +++++- REVIEW.md | 8 ++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4bb81422e8c9..12ebe6eda819 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,7 +102,11 @@ test("(multi-file test) my feature", async () => { - NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI. - Use `tempDir` from `"harness"` to create a temporary directory. **Do not** use `tmpdirSync` or `fs.mkdtempSync` to create temporary directories. - When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure. -- **CRITICAL**: Do not write flaky tests. Do not use `setTimeout` in tests. Instead, `await` the condition to be met. You are not testing the TIME PASSING, you are testing the CONDITION. +- Keep tests fast: budget roughly 1s per test and 10s per file. Debug+ASAN builds run 10-100x slower than release, so a 1s local test can take a minute in CI. +- Never contact the public internet (registry.npmjs.org, github.com, CDNs). Use `VerdaccioRegistry` from `"harness"` for package installs and a local `Bun.serve({ port: 0 })` for HTTP. +- `setDefaultTimeout` is a ceiling, not a target. Leave the default and pass a per-test timeout only for the rare outlier; a 5-minute file default multiplies across retries when one test hangs. +- Leak tests compare growth rate across two equal-sized runs, not an absolute RSS delta. ASAN quarantine and GC jitter make absolute thresholds flaky. +- **CRITICAL**: Do not write flaky tests. Do not use `setTimeout` or `await sleep(N)` to wait for a condition; poll with a deadline or `await` the event itself. You are not testing the TIME PASSING, you are testing the CONDITION. - **CRITICAL**: Verify your test fails with `USE_SYSTEM_BUN=1 bun test ` and passes with `bun bd test `. Your test is NOT VALID if it passes with `USE_SYSTEM_BUN=1`. ## Code Architecture diff --git a/REVIEW.md b/REVIEW.md index 069fb0eea051..b3ef35e5a6a5 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -6,15 +6,15 @@ Several situational sections live in `.claude/docs/landing-prs.md` — read the ## Tests reviewers reject -- **Await conditions, wire failures to reject.** Await the actual observable condition (promise resolved from the event handler, ack handshake, readiness line from child stdout). Wire EVERY failure event (`error`, `close`, `abort`, process exit) to reject the awaited promise — never throw inside event callbacks. Don't raise per-test timeouts to make a slow test pass; shrink the workload. Buffer raw socket/stdout chunks to the protocol's framing before asserting. For "X does not happen", poll a bounded window rather than sleep-then-check. +- **Await conditions, wire failures to reject.** Await the actual observable condition (promise resolved from the event handler, ack handshake, readiness line from child stdout). Wire EVERY failure event (`error`, `close`, `abort`, process exit) to reject the awaited promise — never throw inside event callbacks. Don't raise per-test timeouts to make a slow test pass; shrink the workload. Buffer raw socket/stdout chunks to the protocol's framing before asserting. For "X does not happen", poll a bounded window rather than sleep-then-check. A literal `sleep`/`setTimeout` of 50ms or more outside a bounded poll loop needs a comment naming why no observable signal exists. - **Prove the test fails for the RIGHT reason.** Beyond `USE_SYSTEM_BUN=1`: trace the fixture through every earlier guard/threshold using real constants from source; check that OS fast paths (clonefile, sendfile), error-swallowing APIs (`existsSync`), and build-time fast paths can't satisfy the test without running your code; confirm env knobs the test sets are actually read by `src/`; assert that setup created the precondition. Hang-guard tests assert the process exited on its own (`signalCode === null`). Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test. - **Every assertion must be able to fail, and assert the strongest invariant.** Hunt vacuous patterns: un-awaited `.rejects`/`.resolves`, expects inside catch blocks or callbacks that may never fire, async arrows passed to `toThrow()`, loops over possibly-empty collections, conditional assertions. Assert exact values on normalized output: specific error class/code/message (never bare `toThrow()`), `toBe` over `toContain`, actual bytes not lengths. Read snapshot contents before committing — a snapshot captured from buggy code certifies the bug. Never combine `--update` with a name filter. - **Cover the variant matrix, not just the repro.** Every sibling entry point receiving the same fix (CLI flag AND JS API), both states of every flag, exact limit boundaries (at the limit succeeds, one past fails), every overload, ESM and CJS, alternate modes (`--compile`, `--bytecode`, watch), error paths, the negative contract (sibling files unmodified, callbacks NOT fired), adversarial inputs for anything parsing user data. Add new variants ALONGSIDE existing tests — never mutate an existing test's input to the new case. - **Subprocess tests: drain pipes concurrently.** `Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])` — an unread pipe fills the ~64KB OS buffer and deadlocks the child. Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined `{ stdout, stderr, exitCode }` object. In multi-stage tests, assert each stage's output in order. -- **Tests must be hermetic and leave nothing behind.** Never contact external network hosts or live registries — reproduce the condition with a local in-process server or the container harness. Isolate process-global flags by running each case in a fresh subprocess. Release every resource via `using`/`await using` or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners); no manual close alongside `using`; `server.close()` does not terminate live connections; restore mutated globals in finally. -- **Every behavioral change ships an automated test in the same PR.** "Verified manually", unnamed "existing tests", and benchmarks don't count, even for one-liners. Crash fixes need the crashing input as a spawned fixture; UAF/leak fixes need an ASan repro on the unfixed build or a leak regression test (`Bun.gc(true)` + `heapStats` objectTypeCounts; RSS thresholds need ~2x headroom). Include every reproduction from the linked issue. Never add production code solely to make a test writable — use `bun:internal-for-testing` or externally observable behavior. +- **Tests must be hermetic and leave nothing behind.** Never contact external network hosts or live registries — reproduce the condition with a local in-process server or the container harness. External-service tests (verdaccio, headless chrome, docker, node-gyp) `skipIf` when the dependency is unavailable and hard-deadline their `beforeAll` setup. Isolate process-global flags by running each case in a fresh subprocess. Release every resource via `using`/`await using` or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners); no manual close alongside `using`; `server.close()` does not terminate live connections; restore mutated globals in finally. +- **Every behavioral change ships an automated test in the same PR.** "Verified manually", unnamed "existing tests", and benchmarks don't count, even for one-liners. Crash fixes need the crashing input as a spawned fixture; UAF/leak fixes need an ASan repro on the unfixed build or a leak regression test (`Bun.gc(true)` + `heapStats` objectTypeCounts; RSS-based checks compare growth rate across two equal-sized runs with ~2x headroom, never an absolute delta that ASAN quarantine will jitter past). Include every reproduction from the linked issue. Never add production code solely to make a test writable — use `bun:internal-for-testing` or externally observable behavior. - **Never silently weaken, skip, or delete an existing test or safety net.** Every deletion needs a stated reason or replacement; every skip/todo needs a comment with the observed failure. When de-flaking, keep asserting the property the original assertion protected — branch per-platform rather than dropping precision. Never disable sanitizers or weaken CI verification to get green. When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR. Un-skip `.todo` tests your fix makes pass. Never edit a test to route around a runtime bug it exposed. -- **Copy harness conventions exactly.** Spread `bunEnv` when modifying it (`{...bunEnv, KEY: undefined}`). `Buffer.alloc(n, fill).toString()` instead of `"x".repeat(n)` (slow in debug JSC). `test.each` for matrices; `test.concurrent` for independent subprocess suites; `jest.fn()` over boolean flags; async spawns over `spawnSync`. Use harness skip mechanisms with a reason — a bare top-level `return` reports PASSED. Check `harness.ts` for platform helpers before writing your own. Keep tests fast (~1s budget; debug+ASAN runs 10-100x slower) — a correct but slow test still gets changes-requested. +- **Copy harness conventions exactly.** Spread `bunEnv` when modifying it (`{...bunEnv, KEY: undefined}`). `Buffer.alloc(n, fill).toString()` instead of `"x".repeat(n)` (slow in debug JSC). `test.each` for matrices; `test.concurrent` for independent subprocess suites; `jest.fn()` over boolean flags; async spawns over `spawnSync`. Use harness skip mechanisms with a reason — a bare top-level `return` reports PASSED. Check `harness.ts` for platform helpers before writing your own. Keep tests fast (~1s per test; debug+ASAN runs 10-100x slower); a new file over ~10s on the default lane gets scrutinized for `test.concurrent` and staying serial needs a stated reason. A correct but slow test still gets changes-requested. ## Native code: memory safety (the most-blocked category) From 2d0e3f3c09906a3e4a1a9892f9c512e650f2f65f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 19 Jul 2026 03:43:25 +0000 Subject: [PATCH 2/3] docs: mention test.concurrent in the CLAUDE.md speed budget bullet --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 12ebe6eda819..0a1912a91cf3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,7 +102,7 @@ test("(multi-file test) my feature", async () => { - NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI. - Use `tempDir` from `"harness"` to create a temporary directory. **Do not** use `tmpdirSync` or `fs.mkdtempSync` to create temporary directories. - When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure. -- Keep tests fast: budget roughly 1s per test and 10s per file. Debug+ASAN builds run 10-100x slower than release, so a 1s local test can take a minute in CI. +- Keep tests fast: budget roughly 1s per test and 10s per file. Debug+ASAN builds run 10-100x slower than release, so a 1s local test can take a minute in CI. Use `test.concurrent` for independent subprocess-spawning tests. - Never contact the public internet (registry.npmjs.org, github.com, CDNs). Use `VerdaccioRegistry` from `"harness"` for package installs and a local `Bun.serve({ port: 0 })` for HTTP. - `setDefaultTimeout` is a ceiling, not a target. Leave the default and pass a per-test timeout only for the rare outlier; a 5-minute file default multiplies across retries when one test hangs. - Leak tests compare growth rate across two equal-sized runs, not an absolute RSS delta. ASAN quarantine and GC jitter make absolute thresholds flaky. From 032ffbeb1312bba4e885f99c3cfeb50b6d9ca0b7 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sun, 19 Jul 2026 04:13:05 +0000 Subject: [PATCH 3/3] docs: align leak-test and external-service guidance with tree conventions Verdaccio is a bundled test/package.json dependency started from node_modules, not an out-of-repo system binary. It has no availability probe in harness.ts and 14/15 callers start it unconditionally, so it does not belong in the skipIf-when-unavailable list alongside docker, chrome, and node-gyp. The useful advice for forked helper servers (deadline the startup poll) is kept as a separate clause. The RSS leak-test guidance now codifies what ~50 in-tree tests actually do: branch the threshold on isASAN/isDebug with the bound well below the unfixed leak. The previous wording mandated a two-run growth-rate shape that has no in-tree examples and outlawed the working pattern. --- CLAUDE.md | 2 +- REVIEW.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0a1912a91cf3..aeef626a9688 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -105,7 +105,7 @@ test("(multi-file test) my feature", async () => { - Keep tests fast: budget roughly 1s per test and 10s per file. Debug+ASAN builds run 10-100x slower than release, so a 1s local test can take a minute in CI. Use `test.concurrent` for independent subprocess-spawning tests. - Never contact the public internet (registry.npmjs.org, github.com, CDNs). Use `VerdaccioRegistry` from `"harness"` for package installs and a local `Bun.serve({ port: 0 })` for HTTP. - `setDefaultTimeout` is a ceiling, not a target. Leave the default and pass a per-test timeout only for the rare outlier; a 5-minute file default multiplies across retries when one test hangs. -- Leak tests compare growth rate across two equal-sized runs, not an absolute RSS delta. ASAN quarantine and GC jitter make absolute thresholds flaky. +- Leak tests branch their RSS threshold on `isASAN`/`isDebug` and keep the bound well below what the unfixed leak produces. An un-branched absolute delta flakes under ASAN quarantine and GC jitter. - **CRITICAL**: Do not write flaky tests. Do not use `setTimeout` or `await sleep(N)` to wait for a condition; poll with a deadline or `await` the event itself. You are not testing the TIME PASSING, you are testing the CONDITION. - **CRITICAL**: Verify your test fails with `USE_SYSTEM_BUN=1 bun test ` and passes with `bun bd test `. Your test is NOT VALID if it passes with `USE_SYSTEM_BUN=1`. diff --git a/REVIEW.md b/REVIEW.md index b3ef35e5a6a5..4d78d676b9e9 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -11,8 +11,8 @@ Several situational sections live in `.claude/docs/landing-prs.md` — read the - **Every assertion must be able to fail, and assert the strongest invariant.** Hunt vacuous patterns: un-awaited `.rejects`/`.resolves`, expects inside catch blocks or callbacks that may never fire, async arrows passed to `toThrow()`, loops over possibly-empty collections, conditional assertions. Assert exact values on normalized output: specific error class/code/message (never bare `toThrow()`), `toBe` over `toContain`, actual bytes not lengths. Read snapshot contents before committing — a snapshot captured from buggy code certifies the bug. Never combine `--update` with a name filter. - **Cover the variant matrix, not just the repro.** Every sibling entry point receiving the same fix (CLI flag AND JS API), both states of every flag, exact limit boundaries (at the limit succeeds, one past fails), every overload, ESM and CJS, alternate modes (`--compile`, `--bytecode`, watch), error paths, the negative contract (sibling files unmodified, callbacks NOT fired), adversarial inputs for anything parsing user data. Add new variants ALONGSIDE existing tests — never mutate an existing test's input to the new case. - **Subprocess tests: drain pipes concurrently.** `Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])` — an unread pipe fills the ~64KB OS buffer and deadlocks the child. Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings); assert a combined `{ stdout, stderr, exitCode }` object. In multi-stage tests, assert each stage's output in order. -- **Tests must be hermetic and leave nothing behind.** Never contact external network hosts or live registries — reproduce the condition with a local in-process server or the container harness. External-service tests (verdaccio, headless chrome, docker, node-gyp) `skipIf` when the dependency is unavailable and hard-deadline their `beforeAll` setup. Isolate process-global flags by running each case in a fresh subprocess. Release every resource via `using`/`await using` or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners); no manual close alongside `using`; `server.close()` does not terminate live connections; restore mutated globals in finally. -- **Every behavioral change ships an automated test in the same PR.** "Verified manually", unnamed "existing tests", and benchmarks don't count, even for one-liners. Crash fixes need the crashing input as a spawned fixture; UAF/leak fixes need an ASan repro on the unfixed build or a leak regression test (`Bun.gc(true)` + `heapStats` objectTypeCounts; RSS-based checks compare growth rate across two equal-sized runs with ~2x headroom, never an absolute delta that ASAN quarantine will jitter past). Include every reproduction from the linked issue. Never add production code solely to make a test writable — use `bun:internal-for-testing` or externally observable behavior. +- **Tests must be hermetic and leave nothing behind.** Never contact external network hosts or live registries — reproduce the condition with a local in-process server or the container harness. Tests that need a system binary (headless chrome, docker, node-gyp) `skipIf` when the dependency is unavailable; forked helper servers hard-deadline their `beforeAll` startup. Isolate process-global flags by running each case in a fresh subprocess. Release every resource via `using`/`await using` or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners); no manual close alongside `using`; `server.close()` does not terminate live connections; restore mutated globals in finally. +- **Every behavioral change ships an automated test in the same PR.** "Verified manually", unnamed "existing tests", and benchmarks don't count, even for one-liners. Crash fixes need the crashing input as a spawned fixture; UAF/leak fixes need an ASan repro on the unfixed build or a leak regression test (`Bun.gc(true)` + `heapStats` objectTypeCounts; RSS thresholds branch on `isASAN`/`isDebug` with the bound well below the unfixed leak, and measure after a warmup window when signal is tight). Include every reproduction from the linked issue. Never add production code solely to make a test writable — use `bun:internal-for-testing` or externally observable behavior. - **Never silently weaken, skip, or delete an existing test or safety net.** Every deletion needs a stated reason or replacement; every skip/todo needs a comment with the observed failure. When de-flaking, keep asserting the property the original assertion protected — branch per-platform rather than dropping precision. Never disable sanitizers or weaken CI verification to get green. When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR. Un-skip `.todo` tests your fix makes pass. Never edit a test to route around a runtime bug it exposed. - **Copy harness conventions exactly.** Spread `bunEnv` when modifying it (`{...bunEnv, KEY: undefined}`). `Buffer.alloc(n, fill).toString()` instead of `"x".repeat(n)` (slow in debug JSC). `test.each` for matrices; `test.concurrent` for independent subprocess suites; `jest.fn()` over boolean flags; async spawns over `spawnSync`. Use harness skip mechanisms with a reason — a bare top-level `return` reports PASSED. Check `harness.ts` for platform helpers before writing your own. Keep tests fast (~1s per test; debug+ASAN runs 10-100x slower); a new file over ~10s on the default lane gets scrutinized for `test.concurrent` and staying serial needs a stated reason. A correct but slow test still gets changes-requested.