Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
3 changes: 0 additions & 3 deletions scripts/runner.node.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -786,9 +786,6 @@ async function runTests() {
FORCE_COLOR: "0",
NO_COLOR: "1",
BUN_DEBUG_QUIET_LOGS: "1",
// Node parity: a node test process exits only when its event loop
// drains, and common.mustCall() verifies counts in 'exit' handlers.
BUN_TEST_DRAIN_EVENT_LOOP: "1",
};
if (title.includes("test-util-styletext")) {
// These assert styleText's own color decisions against a TTY, so they need a
Expand Down
4 changes: 0 additions & 4 deletions src/bun_core/env_var.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,6 @@ platform_specific_new!(pub C_INCLUDE_PATH: string, posix = "C_INCLUDE_PATH", win
// Standard C compiler environment variable for library paths (colon-separated).
// Used by bun:ffi's TinyCC integration for systems like NixOS.
platform_specific_new!(pub LIBRARY_PATH: string, posix = "LIBRARY_PATH", windows = None, {});
// Drain the event loop after a file's tests finish so node-style
// `process.on('exit')` checks (e.g. common.mustCall) see completed async work.
// Opt-in for the vendored node:test suite and run() children.
new!(pub BUN_TEST_DRAIN_EVENT_LOOP: boolean, "BUN_TEST_DRAIN_EVENT_LOOP", { default: false });
new!(pub BUN_TMPDIR: string, "BUN_TMPDIR", {});
new!(pub BUN_TRACY_PATH: string, "BUN_TRACY_PATH", {});
new!(pub BUN_WATCHER_TRACE: string, "BUN_WATCHER_TRACE", {});
Expand Down
2 changes: 1 addition & 1 deletion src/js/node/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ async function runOneFile(
const proc = Bun.spawn({
cmd: args,
cwd: opts.cwd as string,
env: { ...(opts.env ?? process.env), BUN_TEST_DRAIN_EVENT_LOOP: "1", [kRunChildEnv]: kRunChildEnvValue },
env: { ...(opts.env ?? process.env), [kRunChildEnv]: kRunChildEnvValue },
Comment thread
robobun marked this conversation as resolved.
stdout: "pipe",
stderr: "pipe",
signal: opts.signal,
Expand Down
52 changes: 29 additions & 23 deletions src/runtime/cli/test_command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -900,14 +900,6 @@
}
}

/// Drain the event loop after a file's tests finish, like a node process
/// would before exiting; the vendored-node-test runner opts in via
/// `BUN_TEST_DRAIN_EVENT_LOOP=1` so mustCall()-style exit checks see
/// completed async work. Off by default: bun suites keep exit-after-tests.
pub(crate) fn should_drain_event_loop() -> bool {
env_var::BUN_TEST_DRAIN_EVENT_LOOP.get().unwrap_or(false)
}

pub struct CommandLineReporter {
// `TestRunner<'a>` borrows `TestOptions`/regex from the CLI ctx; the
// reporter is held in a `Box` local to `TestCommand::exec` which never
Expand Down Expand Up @@ -2250,6 +2242,7 @@
test_options: unsafe { bun_ptr::detach_lifetime_ref(&ctx.test_options) },
unhandled_errors_between_tests: 0,
summary: Summary::default(),
node_test_module_used: false,
},
last_dot: 0,
repeat_count: 1,
Expand Down Expand Up @@ -3053,18 +3046,39 @@
{
vm.exit_handler.exit_code = 1;
}
// Run `process.on('exit')` handlers like `bun run` does. Node's test
// harness verifies mustCall() counts from one, so skipping them made
// those assertions silently pass. Must precede the GC-root release
// below: handlers are user JS and may touch still-live state.
{
if reporter.jest.node_test_module_used {
// Node parity: a node test process exits only when its loop drains,
// and node's test harness verifies mustCall() counts from a
// `process.on('exit')` handler. `on_before_exit()` spins while
// `is_event_loop_alive()`, which cannot tell one file's handles
// from another's; gate the drain on single-file runs (the vendored
// node suite and run() children both spawn one file per process) so
// a leaked handle from an earlier file cannot wedge a mixed
// multi-file run. Must precede the GC-root release below; handlers
// are user JS and may touch still-live state.
let drain = reporter.summary().files <= 1;
let prev_unhandled = vm.unhandled_error_counter;
let vm_ptr: *mut VirtualMachine = vm;
// SAFETY: `vm_ptr` reborrows the live `&mut VirtualMachine`;
// `run_with_api_lock` takes `&self` only, so the closure holds the
// unique mutable access on this single-threaded path.
vm.run_with_api_lock(|| unsafe { (*vm_ptr).on_exit() });
vm.run_with_api_lock(|| unsafe {
if drain {
(*vm_ptr).on_before_exit();
}
(*vm_ptr).global().handle_rejected_promises();
(*vm_ptr).on_exit();
});
Comment thread
robobun marked this conversation as resolved.
// The drain and handlers above run after `active_file` is cleared,
// so an uncaught throw there prints but does not reach the
// exit-code decision above; propagate it here.
if vm.unhandled_error_counter > prev_unhandled {
vm.exit_handler.exit_code = 1;
}

Check warning on line 3077 in src/runtime/cli/test_command.rs

View check run for this annotation

Claude / Claude Code Review

Exit handlers receive stale code=0 for drain-surfaced errors

The `unhandled_error_counter > prev_unhandled` check at line 3075 runs *after* `on_exit()` has already dispatched `process.on('exit')` handlers, so a drain-surfaced throw makes handlers see `code === 0` even though the process will exit 1. Node's `common.mustCall()` checker starts with `if (exitCode !== 0) return;`, so this prints spurious 'Mismatched' noise on top of the real error. Move the check inside the closure, between `handle_rejected_promises()` and `(*vm_ptr).on_exit()` (the post-closu
Comment thread
robobun marked this conversation as resolved.
Outdated
// on_exit() already set is_shutting_down; global_exit() asserts it.
} else {
vm.is_shutting_down = true;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
// on_exit() already set is_shutting_down; global_exit() asserts it.
// Release `bun:test` GC roots before `global_exit()` so
// `destructOnExit()`'s `collectNow()` can reach the closures they pin
// (preload hooks, per-file describe/test callbacks). Clear `RUNNER`
Expand Down Expand Up @@ -3377,14 +3391,6 @@
let el = vm.event_loop();
// SAFETY: el is the VM-owned event loop; vm is passed back as *mut.
unsafe { (*el).tick_immediate_tasks(vm) };

// Node parity: a node test file exits only when its loop drains.
// on_before_exit() drains and dispatches 'beforeExit' like `bun run`;
// it early-returns when unhandled_error_counter > 0, which is fine
// here since such a file already failed. Opt-in; one file per process.
if should_drain_event_loop() {
vm.on_before_exit();
}
drop(buntest_strong);
}

Expand Down
14 changes: 12 additions & 2 deletions src/runtime/test_runner/jest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ pub struct TestRunner<'a> {
pub unhandled_errors_between_tests: u32,
pub summary: Summary,

/// Set the first time `node:test` registers something in this process.
/// Gates node-parity shutdown (event-loop drain, `process.on('exit')`
/// handlers) so bun:test-only runs keep exit-after-tests.
pub node_test_module_used: bool,

pub bun_test_root: bun_test::BunTestRoot,
}

Expand Down Expand Up @@ -523,8 +528,13 @@ pub(crate) fn js_file_generation(
// registration, and an exclusive `&mut TestRunner` would invalidate the
// `bun_test_root` pointer `test_command.rs` keeps live across the file run.
// SAFETY: same invariant as `runner()` — RUNNER is only read on the JS thread.
let generation =
Jest::runner_ptr().map_or(0, |p| unsafe { (*p.as_ptr()).bun_test_root.file_generation });
let generation = Jest::runner_ptr().map_or(0, |p| unsafe {
let runner = p.as_ptr();
// `node_test_module_used` is disjoint from `bun_test_root`; the field
// projection does not overlap the caller's live `&BunTestRoot`.
(*runner).node_test_module_used = true;
(*runner).bun_test_root.file_generation
});
Ok(JSValue::from(generation))
}

Expand Down
81 changes: 76 additions & 5 deletions test/cli/test/bun-test.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1457,8 +1457,8 @@
expect(output).toContain("app message");
});

test("runs process.on('exit') handlers", async () => {
using dir = tempDir("bun-test-exit-handler", {
test("bun:test does not run process.on('exit') handlers", async () => {
using dir = tempDir("bun-test-no-exit-handler", {
"exit.test.ts": `
import { test } from "bun:test";
process.on("exit", () => console.log("exit handler ran"));
Expand All @@ -1473,16 +1473,87 @@
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).not.toContain("exit handler ran");
expect(stderr).toContain("1 pass");
expect(exitCode).toBe(0);
});

test("node:test runs process.on('exit') handlers", async () => {
using dir = tempDir("bun-test-node-exit-handler", {
"exit.test.ts": `
import { test } from "node:test";
process.on("exit", () => console.log("exit handler ran"));
test("a test", () => {});
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "test", "exit.test.ts"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toContain("exit handler ran");
expect(stderr).toContain("1 pass");
expect(exitCode).toBe(0);
});

test("an exit handler can fail the run, like node's common.mustCall()", async () => {
using dir = tempDir("bun-test-exit-handler-code", {
"exit-code.test.ts": `
test("node:test in a multi-file run does not hang on a handle an earlier bun:test file leaked", async () => {
using dir = tempDir("bun-test-node-multi-file-drain", {
"a.test.ts": `
import { test } from "bun:test";
setInterval(() => {}, 100);
test("a", () => {});
`,
"b.test.ts": `
import { test } from "node:test";
process.on("exit", () => console.log("exit handler ran"));
test("b", () => {});
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "test", "a.test.ts", "b.test.ts"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toContain("exit handler ran");
expect(stderr).toContain("2 pass");
expect(exitCode).toBe(0);
});

test("node:test: an uncaught throw surfaced by the event-loop drain fails the run", async () => {
using dir = tempDir("bun-test-node-drain-throw", {
"drain-throw.test.ts": `
import { test } from "node:test";
test("a", () => {});
setTimeout(() => { throw new Error("boom") }, 500);

Check warning on line 1536 in test/cli/test/bun-test.test.ts

View check run for this annotation

Claude / Claude Code Review

drain-throw fixture's setTimeout(500) adds unnecessary 500ms to every run

The `setTimeout(..., 500)` in the drain-throw fixture gives this test a hard 500ms wall-clock floor — the spawned `bun test` must spin `on_before_exit()` until the timer fires before it can exit. The 500ms was carried over verbatim from the ad-hoc repro; since the fixture's only test is a synchronous no-op and timers don't fire during collection/`tick_immediate_tasks`, a much smaller delay (~20ms — the reviewer's own analysis used 10ms) exercises the identical end-of-run drain path. REVIEW.md: '
Comment thread
robobun marked this conversation as resolved.
Outdated
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "test", "drain-throw.test.ts"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain("boom");
expect(stderr).toContain("1 pass");
expect(exitCode).toBe(1);
});

test("node:test: an exit handler can fail the run, like node's common.mustCall()", async () => {
using dir = tempDir("bun-test-node-exit-handler-code", {
"exit-code.test.ts": `
import { test } from "node:test";
process.on("exit", () => process.exit(1));
test("a passing test", () => {});
`,
Expand Down
Loading