docs(agents): note Bun process-level assertion gotchas in TS test rule (EXSC-687) - #2134
docs(agents): note Bun process-level assertion gotchas in TS test rule (EXSC-687)#21340xDEnYO wants to merge 2 commits into
Conversation
…e (EXSC-687) Adds [CONV:TEST-PROCESS-SURFACE] to 402-typescript-tests: spyOn on process globals silently misses writes made by the code under test, so the output/exit surface must be injected instead; and expect().rejects trips @typescript-eslint/await-thenable, so a local expectRejects helper is used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds TypeScript test guidance for asserting process-level stdout and exit behavior through injected surfaces, and for matching rejected promises with a local helper instead of ChangesTypeScript test assertion guidance
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Light-triage gate note: bullet 1's factual premise fails to reproduce on Bun 1.3.8 (the exact version the observation cites). Falsification attempt before this ships as a rule:
Suggestion: keep the inject-the-surface advice (good design regardless), but reword bullet 1's justification — as written it bans a working technique on a premise that doesn't repro. If the original failure is reproducible, the rule should name the actual trigger (console vs stream write) instead of the blanket claim. Repro snippet: import { describe, it, expect, spyOn } from 'bun:test'
import { writeViaStdout, writeViaConsole } from './mod' // process.stdout.write(...) / console.log(...)
it('stdout.write from another module IS captured', () => {
const spy = spyOn(process.stdout, 'write').mockImplementation(() => true)
writeViaStdout()
expect(spy.mock.calls.length).toBeGreaterThan(0) // passes on 1.3.8
spy.mockRestore()
}) |
…rejection rule (EXSC-687)
Which Linear task belongs to this PR?
EXSC-687 — this note is a byproduct of writing
script/deploy/github/verify-approvals.test.tsin #2128. No separate ticket was cut; the findingbelongs to the same work.
Why did I implement it this way?
Two Bun-specific traps cost real debugging time while writing tests for the approval-check exit-code
gate, and both are invisible until you hit them:
spyOn(process.stdout, 'write')does not capture writes made by the code under test — theassertion reports "was not called" even though the write demonstrably happens. The working
pattern is to inject the output/exit surface as a parameter defaulting to
process(
reportApprovalResult(failures, target: IReportTarget = process)in fix(deploy): gate branch-deploy approval check on exit code (EXSC-687) #2128) and pass a recorderin the test.
expect(...).rejectstrips@typescript-eslint/await-thenable, because Bun's matcher isn't areal Promise.
script/deploy/safe/parked-tasks.test.tsalready documents this inline and worksaround it with a local
expectRejectshelper.Both are "asserting on process-level behaviour" problems, so they're captured as one
[CONV:TEST-PROCESS-SURFACE]section rather than two scattered bullets. Landed in.agents/rules/402-typescript-tests.md(globs**/*.test.ts) because that's the narrowest owningrule — a test author hits both traps, and the rule auto-loads exactly when they do. The inline
comment in
parked-tasks.test.tsstays where it is; the rule generalises it so the next authordoesn't have to have read that file.
Edited in
.agents/per the repo's symlink convention;.cursor/rules/402-typescript-tests.mdcand
.claude/rules/402-typescript-tests.mdalready point at it, so no symlink work was needed..agents/README.mdis unchanged —name,descriptionandglobsare all untouched, which is theonly trigger for a README update.
One thing for the reviewer: the
verify-approvals.tsreference points at code that only existson #2128's branch — it resolves once that PR merges. Everything else cited (
expectRejectsinparked-tasks.test.ts) is onmaintoday. The Bun version is called out as observed on 1.3.8(the local toolchain); the repo pins
bun@1.3.13inpackageManagerand I did not re-verify thespy behaviour there, so the guidance is stated unconditionally rather than as version-gated.
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)