test: allow onTestFinished() and expect.assertions() in concurrent tests #29238
+273
−29
Claude / Claude Code Review
completed
Apr 12, 2026 in 10m 2s
Code review found 1 important issue
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | packages/bun-types/test.d.ts:381-383 |
Missing synchronous-registration caveat in docs and JSDoc |
| 🟡 Nit | test/regression/issue/29236.test.ts:60-65 |
Weak pass/fail count assertions susceptible to false positives |
| 🟡 Nit | test/js/bun/test/test-on-test-finished.test.ts:82-91 |
setTimeout used in new tests, violating CLAUDE.md policy |
Annotations
Check failure on line 383 in packages/bun-types/test.d.ts
claude / Claude Code Review
Missing synchronous-registration caveat in docs and JSDoc
Both docs/test/lifecycle.mdx (line 109) and packages/bun-types/test.d.ts (lines 381-383) now unconditionally state that onTestFinished() "Works inside concurrent tests" with no caveat, but the PR own Known Limitation section acknowledges that calling it after an await in a concurrent test still throws the old (now-wrong) error. A user reading the docs or hovering over the type declaration would reasonably expect the post-await pattern to work, but it does not — and the error message they receive
Check warning on line 65 in test/regression/issue/29236.test.ts
claude / Claude Code Review
Weak pass/fail count assertions susceptible to false positives
The pass/fail count assertions in the new regression test use substring matching (toContain) which can produce false positives: toContain("0 fail") would pass on output containing "10 fail" or "20 fail", and toContain("4 pass") would match "14 pass". The exitCode check acts as a safety net, but anchoring with toMatch(/\b4 pass\b/m) would make intent explicit.
Check warning on line 91 in test/js/bun/test/test-on-test-finished.test.ts
claude / Claude Code Review
setTimeout used in new tests, violating CLAUDE.md policy
New test bodies use setTimeout to simulate async work, violating CLAUDE.md's explicit prohibition on setTimeout in tests. In test-on-test-finished.test.ts lines 82 and 90, the delays have a technical justification (macrotasks keep concurrent sequences genuinely in-flight), but the assertions do not require true concurrent execution and would pass with Bun.sleep(0) or a similar alternative; in 29236.test.ts lines 24, 30, 36, 69, 75, the delays are inside fixture strings simulating user repro code
Loading