Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. 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 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 <file>` and passes with `bun bd test <file>`. Your test is NOT VALID if it passes with `USE_SYSTEM_BUN=1`.

## Code Architecture
Expand Down
8 changes: 4 additions & 4 deletions REVIEW.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. 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 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)

Expand Down
Loading