Repository navigation
docs: add test reliability and speed guidance to CLAUDE.md and REVIEW.md - #34679
Conversation
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.
|
Docs-only change (CLAUDE.md + REVIEW.md). No source or test code touched. Self-review addressed three findings:
Accepted as-is:
CI: builds 75614 and 75625 each show a disjoint set of failures, all tagged Ready for review. |
|
Updated 10:27 PM PT - Jul 18th, 2026
❌ @robobun, your commit 032ffbe has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34679That installs a local version of the PR into your bun-34679 --bun |
|
Warning Review limit reached
Next review available in: 2 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — docs-only additions to contributor test guidance.
What was reviewed:
- Verified
VerdaccioRegistryis the actual export name intest/harness.ts(line 1816). - Checked that CLAUDE.md/REVIEW.md are not covered by CODEOWNERS.
- REVIEW.md edits are purely additive to existing bullets — no existing guidance was removed or weakened.
- New bullets match the surrounding voice and don't contradict existing rules.
Extended reasoning...
Overview
This PR adds test-reliability and test-speed guidance to two contributor documentation files: CLAUDE.md (the authoring guide) and REVIEW.md (the reviewer checklist). In CLAUDE.md, four new bullets are added to the "Writing Tests" section and one existing bullet is refined to name await sleep(N) alongside setTimeout. In REVIEW.md, four existing bullets in "Tests reviewers reject" are extended in-place with additional clauses (50ms sleep threshold, external-service skipIf/deadline, RSS growth-rate leak checks, ~10s file budget). No source code, tests, build config, or CI is touched.
Security risks
None. This is prose in Markdown documentation files that are read by contributors and by automated review tooling. Nothing here affects runtime behavior, the build, or any executed code path.
Level of scrutiny
Low. Docs-only changes to contributor guidance carry no correctness or safety risk to the shipped product. The main things worth checking are (a) factual accuracy of referenced helpers, and (b) that no existing guidance was silently deleted. I confirmed VerdaccioRegistry exists as a named export in test/harness.ts:1816, and diffed each REVIEW.md line to verify the changes are strictly additive — every prior clause is preserved verbatim with new sentences appended. Neither file is covered by a CODEOWNERS rule.
Other factors
The additions are consistent in tone and density with the surrounding bullets, and the PR description grounds each rule in a measured CI anti-pattern. The self-review comment already addressed the intentional duplication of the ~1s budget across both files (different audiences). The bug-hunting system found no issues. Given the change is small, self-contained, purely documentation, and factually accurate, this is safe to approve without human review.
…ions 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.
There was a problem hiding this comment.
No issues found — the prose is accurate and the referenced helpers (VerdaccioRegistry, isASAN, isDebug) all exist in test/harness.ts. Deferring to a maintainer only because this adds new normative review-blocking criteria to REVIEW.md (the 50ms sleep-comment rule, the ~10s/file test.concurrent scrutiny threshold), and policy thresholds like those are worth a human sign-off before they start gating other PRs.
What was reviewed
- Verified all named harness exports exist at the cited locations.
- Confirmed the diff is strictly additive/tightening — no existing rule is weakened or removed.
- Checked CODEOWNERS: neither file is owned.
- Docs-only; no source, test, or build-affecting files touched.
Extended reasoning...
Overview
Docs-only PR touching two contributor-guidance files: CLAUDE.md (the agent/contributor instructions) and REVIEW.md (the review-blocking criteria). Adds four new bullets to the "Writing Tests" section of CLAUDE.md and extends four existing bullets in REVIEW.md's "Tests reviewers reject" section. All additions concern test reliability and speed: per-test/per-file time budgets, test.concurrent for independent subprocess suites, no public-internet access in tests, setDefaultTimeout as ceiling not target, ASAN/debug-branched leak thresholds, and a comment requirement for literal sleeps ≥50ms outside poll loops.
Security risks
None. Markdown-only change with no executable content, no CI/workflow modifications, and no source or test code.
Level of scrutiny
Low for correctness (prose, no runtime effect), but moderate for policy. REVIEW.md is the document that defines what blocks merges in this repo, and CLAUDE.md drives agent behavior. The additions introduce new concrete thresholds — a 50ms floor above which uncommented sleep/setTimeout is flagged, and a ~10s/file bar above which serial tests need a stated justification. These are reasonable, derived from a CI audit per the PR description, and match existing practice in the tree (~50 isASAN ?-branched thresholds already exist). But they are still policy choices that will be enforced on every future PR, so a maintainer should confirm the specific numbers.
Other factors
- All harness identifiers referenced in the new prose were spot-checked against
test/harness.tsand exist (VerdaccioRegistryat line 1816,isASANat line 54,isDebugat line 27). - The diff is purely additive — the only modified pre-existing sentence (the CRITICAL no-
setTimeoutrule in CLAUDE.md) is tightened to also nameawait sleep(N)and recommend polling with a deadline; nothing is loosened or removed. - Neither file is covered by
.github/CODEOWNERS. - No prior
claude[bot]review on this PR; only robobun self-review and a rate-limited coderabbit stub in the timeline.
Given the change is to the repo's review policy itself, I'm deferring rather than auto-approving so a human can ratify the new thresholds.
What
Adds test-reliability and test-speed rules to the contributor docs, derived from an audit of recent CI builds (only 1 of 95 recent PR builds was fully green) and the anti-patterns behind the noise.
Measured in
test/today:setDefaultTimeout(1000 * 60 * 5); one hang costs 5min x 4 retry attemptsawait sleep()/Bun.sleep()with a literal of 1000ms or more, 148 at 100ms or moreregistry.npmjs.orgdirectlyChanges
CLAUDE.md (Writing Tests): four new bullets covering the ~1s/test and ~10s/file budget with
test.concurrentfor independent subprocess tests, never hitting the public internet (useVerdaccioRegistry/ localBun.serve),setDefaultTimeoutas a ceiling not a target, and leak tests branching their RSS threshold onisASAN/isDebug. The existing no-setTimeoutrule now also namesawait sleep(N)explicitly.REVIEW.md (Tests reviewers reject): extends four existing bullets rather than adding new ones, so the rules stay grouped with the concept they refine:
sleep/setTimeoutof 50ms+ outside a bounded poll loop needs a comment naming why no observable signal existsskipIfwhen unavailable; forked helper servers hard-deadline theirbeforeAllstartupisASAN/isDebugwith the bound well below the unfixed leak, and measure after a warmup window when signal is tighttest.concurrent, and staying serial needs a stated reasonWhy
These are structural rules that prevent flakes at authoring time instead of discovering them in CI. Each one maps to a concrete failure mode seen across recent builds, and each codifies what the passing tests in the tree already do (the leak-test rule matches the ~50 existing
isASAN ?-branched thresholds; theskipIflist matches the three availability probes inharness.ts). The additions match the existing voice and density of each file and do not restate rules already present.Docs-only change; no build needed.
no test proof · iteration 0 · docs-only change; test-proof not applicable