node:test: pass a done() callback to (t, done) => {} style tests - #32682
node:test: pass a done() callback to (t, done) => {} style tests#32682Sanjays2402 wants to merge 2 commits into
Conversation
When a user passes a test function with two or more parameters to `test()` from node:test, Node.js binds a `done` callback to the second argument and waits for it to be invoked before completing the test. Previously Bun's node:test wrapper always called the user function with just `(context)`, so `done` resolved to `undefined` and the user got `TypeError: done is not a function` (issue oven-sh#32527). This commit: - Detects callback-style tests via `fn.length >= 2` in `createTest`. - Forwards the wrapper's `endTest` (which calls the bun:test `done` parameter once it is wired up by the runtime) as the user's second argument. - Makes `endTest` idempotent so calling `done()` and also returning a non-Promise from the same function does not complete the test twice. - Skips the auto-end fall-through for callback-style synchronous returns so the runner waits on `done` (or its timeout) instead of marking the test passed immediately. - Extends the `TestFn` type to reflect the optional `done` parameter. A new fixture (06-callback-tests.js) exercises the sync, async, and plain (t)/async (t) signatures, and a matching case has been added to node-test.test.ts. Fixes oven-sh#32527
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Walkthrough
Changesnode:test callback completion
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/test.ts`:
- Around line 698-700: The promise-handling in the test runner is incorrectly
completing callback-style tests when useCallback is true, so result resolution
should not call endTest in that mode. Update the logic around the result
instanceof Promise check in the test execution flow so only non-callback tests
are finished by promise resolution, and keep completion for callback-style tests
tied solely to done()/endTest. Refer to the test runner’s promise branch and the
useCallback condition to locate the fix.
In `@test/js/node/test_runner/fixtures/06-callback-tests.js`:
- Around line 13-19: The callback-style async fixture in the test is still
timer-based, so it does not reliably prove that runTest waits for done() before
completing. Update the test in 06-callback-tests.js to use an observable
completion sentinel or teardown assertion tied to the callback path in callback
test resolves with done() asynchronously, so the test fails if completion
happens before done() runs. Remove the setTimeout-based delay and make the
assertion depend directly on the callback ordering instead.
In `@test/js/node/test_runner/node-test.test.ts`:
- Around line 56-61: The current node test runner coverage only exercises the
successful done() path, so add end-to-end tests for the new callback error and
idempotency behavior. Extend the existing node-test suite around runTests and
the 06-callback-tests.js fixture with a case where a test calls done(new
Error("boom")) and asserts the failure output, plus a mixed-completion case that
calls done more than once or combines done with another completion path to
verify the ended guard suppresses duplicate completion handling. Use the
existing node-test.test.ts runTests helper and the callback fixture names to
keep the new cases alongside the current done() callback coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7eccdad4-332e-4b17-bbaa-573b58899141
📒 Files selected for processing (3)
src/js/node/test.tstest/js/node/test_runner/fixtures/06-callback-tests.jstest/js/node/test_runner/node-test.test.ts
| // Async-but-callback-style test: schedules done() in a setTimeout. | ||
| test("callback test resolves with done() asynchronously", (t, done) => { | ||
| setTimeout(() => { | ||
| callCount++; | ||
| done(); | ||
| }, 5); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the async callback case observable instead of timer-based.
This fixture still passes if runTest incorrectly auto-completes the test before done() is called, because the timer fires later and callCount still reaches 4. Please make the async path assert completion ordering directly (for example with a teardown/sentinel that only flips inside the callback) so it actually exercises the new callback-waiting branch.
As per coding guidelines, "Do not use setTimeout in tests; instead, await the actual condition to be met rather than testing time passing."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/node/test_runner/fixtures/06-callback-tests.js` around lines 13 - 19,
The callback-style async fixture in the test is still timer-based, so it does
not reliably prove that runTest waits for done() before completing. Update the
test in 06-callback-tests.js to use an observable completion sentinel or
teardown assertion tied to the callback path in callback test resolves with
done() asynchronously, so the test fails if completion happens before done()
runs. Remove the setTimeout-based delay and make the assertion depend directly
on the callback ordering instead.
Source: Coding guidelines
| test("should pass a done() callback to tests whose function takes two parameters", async () => { | ||
| const { exitCode, stderr } = await runTests(["06-callback-tests.js"]); | ||
| expect({ exitCode, stderr }).toMatchObject({ | ||
| exitCode: 0, | ||
| stderr: expect.stringContaining("0 fail"), | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add end-to-end coverage for the new callback error/idempotency paths.
This only checks the all-pass case. The runtime change also introduced new callback-specific behavior for done(error) and duplicate completion suppression, but those branches are still untested here. A tiny companion fixture for done(new Error("boom")) and one mixed-completion case would lock down the new ended guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/node/test_runner/node-test.test.ts` around lines 56 - 61, The current
node test runner coverage only exercises the successful done() path, so add
end-to-end tests for the new callback error and idempotency behavior. Extend the
existing node-test suite around runTests and the 06-callback-tests.js fixture
with a case where a test calls done(new Error("boom")) and asserts the failure
output, plus a mixed-completion case that calls done more than once or combines
done with another completion path to verify the ended guard suppresses duplicate
completion handling. Use the existing node-test.test.ts runTests helper and the
callback fixture names to keep the new cases alongside the current done()
callback coverage.
When a (t, done) => {} test also returns a promise (e.g. an async
callback-style body), completing the test on promise resolution could
finish it before done() was ever called, contradicting the callback
contract. Only auto-complete on resolution when the test is NOT
callback-style; a rejected promise is still surfaced as a failure.
Make the async-callback fixture prove ordering instead of relying on a
timer: a follow-up test asserts a sentinel that is only set inside the
deferred done(), so a premature completion would fail. Add a done()
idempotency case and a separate fixture exercising done(error) so the
failure path and the double-completion guard are covered end to end.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/test_runner/fixtures/06-callback-tests.js (1)
1-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for synchronous throws in callback-style tests.
The PR objectives state that
runTest"forwards synchronous throws" for callback-style tests, but no test here exercises a callback-style test body that throws synchronously (rather than callingdone(error)or rejecting a promise). Consider adding a case liketest("...", (t, done) => { throw new Error("sync"); })to lock down that path alongside the existingdone(error)coverage in07-callback-done-error.js.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/node/test_runner/fixtures/06-callback-tests.js` around lines 1 - 61, Add a callback-style test case in the fixture that throws synchronously from its `(t, done)` body without calling `done()`, and assert the runner reports the thrown error through the expected test-failure behavior. Keep the existing invocation count and exit assertion consistent, and place it alongside the current callback-style completion tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/test_runner/fixtures/06-callback-tests.js`:
- Around line 19-28: Replace the setTimeout call in the callback test with
setImmediate, preserving the delayed callback body that increments callCount,
sets sawDone, and invokes done(). Keep the promise resolution and macrotask
boundary behavior unchanged without relying on elapsed time.
---
Outside diff comments:
In `@test/js/node/test_runner/fixtures/06-callback-tests.js`:
- Around line 1-61: Add a callback-style test case in the fixture that throws
synchronously from its `(t, done)` body without calling `done()`, and assert the
runner reports the thrown error through the expected test-failure behavior. Keep
the existing invocation count and exit assertion consistent, and place it
alongside the current callback-style completion tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7766eccc-3280-4b77-8b4d-7e19e04bd87b
📒 Files selected for processing (4)
src/js/node/test.tstest/js/node/test_runner/fixtures/06-callback-tests.jstest/js/node/test_runner/fixtures/07-callback-done-error.jstest/js/node/test_runner/node-test.test.ts
| test("callback test waits for done() even when the body returns a promise", (t, done) => { | ||
| // Returning a resolved promise must NOT complete the test; only done() may. | ||
| return Promise.resolve().then(() => { | ||
| setTimeout(() => { | ||
| callCount++; | ||
| sawDone = true; | ||
| done(); | ||
| }, 5); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace setTimeout with a timer-free macrotask deferral.
Line 22 uses setTimeout(..., 5) to force an async gap between promise resolution and done(). As per path instructions, test/**: "CRITICAL: Do not use setTimeout in tests... you are not testing the TIME PASSING, you are testing the CONDITION." setImmediate achieves the same macrotask boundary without an arbitrary timer.
♻️ Proposed fix
return Promise.resolve().then(() => {
- setTimeout(() => {
+ setImmediate(() => {
callCount++;
sawDone = true;
done();
- }, 5);
+ });
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/node/test_runner/fixtures/06-callback-tests.js` around lines 19 - 28,
Replace the setTimeout call in the callback test with setImmediate, preserving
the delayed callback body that increments callCount, sets sawDone, and invokes
done(). Keep the promise resolution and macrotask boundary behavior unchanged
without relying on elapsed time.
Source: Path instructions
|
Thank you for this contribution! This was fixed in #32631: Closing as already fixed. Thanks again for the investigation and patch! |
2 participants
What does this PR do?
Closes #32527.
Bun's
node:testdid not pass the seconddonecallback argument when a test function declares two or more parameters (test('name', (t, done) => { ... })), unlike Node. As a resultdoneresolved toundefinedand callback-style tests could never signal completion.This:
fn.length >= 2increateTest.endTestas the user's seconddoneargument, and waits for it to be called instead of auto-completing synchronously (the existing test timeout still applies ifdoneis never called).endTestidempotent so callingdone()and also returning a non-Promise (or throwing) from the same function does not complete the test twice.TestFntype to include the optionaldoneparameter.How did you verify your code works?
Added
test/js/node/test_runner/fixtures/06-callback-tests.jsand a runner case intest/js/node/test_runner/node-test.test.tscovering:done(),done()from asetTimeout,(t) => {}sync signature, andasync (t) => {}signature,asserting all four invoke and the suite reports
0 fail. Run withbun test test/js/node/test_runner/node-test.test.ts.