Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
32 changes: 19 additions & 13 deletions src/runtime/test_runner/ScopeFunctions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,7 @@
ParseArgumentsCfg { callback: callback_mode, kind: FunctionKind::TestOrDescribe },
)?;

let callback_length: usize = if let Some(callback) = args.callback {
callback.get_length(global)? as usize
} else {
0
};
let callback_length: usize = args.callback_length as usize;

if !this.each.is_empty() {
if this.each.is_undefined_or_null() || !this.each.is_array() {
Expand Down Expand Up @@ -521,6 +517,10 @@
pub struct ParseArgumentsResult {
pub description: Option<Vec<u8>>,
pub callback: Option<JSValue>,
/// `.length` of the original callback, read before any `AsyncContextFrame`
/// wrapping. `callback` may be the wrapper (which has no `.length`), so
/// callers must read arity from here, not from `callback.get_length()`.
pub callback_length: u64,
pub options: ParseArgumentsOptions,
}

Expand Down Expand Up @@ -665,18 +665,24 @@
};
let (description, callback, options) = (items.description, items.callback, items.options);

let result_callback: Option<JSValue> = if cfg.callback != CallbackMode::Require && callback.is_undefined_or_null() {
None
} else if callback.is_function() {
Some(callback.with_async_context_if_needed(global))
} else {
let ordinal = if cfg.kind == FunctionKind::Hook { "first" } else { "second" };
return Err(global.throw(format_args!("{} expects a function as the {} argument", signature, ordinal)));
};
let (result_callback, callback_length): (Option<JSValue>, u64) =
if cfg.callback != CallbackMode::Require && callback.is_undefined_or_null() {
(None, 0)
} else if callback.is_function() {
// Read `.length` from the user's function before wrapping it in an
// `AsyncContextFrame`: the wrapper has no `.length` and callers
// would otherwise misread the arity (done-callback detection).
let length = callback.get_length(global)?;
(Some(callback.with_async_context_if_needed(global)), length)

Check notice on line 676 in src/runtime/test_runner/ScopeFunctions.rs

View check run for this annotation

Claude / Claude Code Review

test.each()/describe.each() inside ALS still broken: bind() throws on non-callable AsyncContextFrame

Pre-existing, same bug class: `test.each()` / `describe.each()` registered inside `als.run()` still fails at collection time — `args.callback` is the non-callable `AsyncContextFrame` wrapper, and the `.each` path passes it to `JSValueTestExt::bind()`, which throws `TypeError: bind() called on non-callable`. Not introduced by this PR, but it's the sibling site of the arity fix (and the new `callback_length.saturating_sub(...)` on line 245 is unreachable because `bind()` throws first); consider al
Comment thread
robobun marked this conversation as resolved.
Outdated
} else {
let ordinal = if cfg.kind == FunctionKind::Hook { "first" } else { "second" };
return Err(global.throw(format_args!("{} expects a function as the {} argument", signature, ordinal)));
};

let mut result = ParseArgumentsResult {
description: None,
callback: result_callback,
callback_length,
options: ParseArgumentsOptions::default(),
};
// `result` cleanup handled by Drop on early return.
Expand Down
6 changes: 1 addition & 5 deletions src/runtime/test_runner/bun_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,7 @@ pub mod js_fns {
ScopeFunctions::ParseArgumentsCfg { callback: ScopeFunctions::CallbackMode::Require, kind: ScopeFunctions::FunctionKind::Hook },
)?;

let has_done_parameter = if let Some(callback) = args.callback {
callback.get_length(global_this)? > 0
} else {
false
};
let has_done_parameter = args.callback.is_some() && args.callback_length > 0;

let bun_test_root = get_active_test_root(
global_this,
Expand Down
71 changes: 71 additions & 0 deletions test/js/bun/test/als-hook-arity.fixture.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from "bun:test";
import { AsyncLocalStorage } from "node:async_hooks";

// When a test or hook is registered while an AsyncLocalStorage context is
// active, bun:test wraps the callback in an AsyncContextFrame so the context
// is restored at call time. Done-param detection must read the arity of the
// *user* function, not the wrapper (which has no `.length`), otherwise every
// zero-arg callback waits for a done() that never comes and times out.

const als = new AsyncLocalStorage<{ tag: string }>();
const order: string[] = [];

describe("registered inside an active ALS context", () => {
als.run({ tag: "collection" }, () => {
beforeAll(function zeroArg() {
order.push("beforeAll");
});
beforeEach(function zeroArg() {
order.push("beforeEach");
});
afterEach(function zeroArg() {
order.push("afterEach");
});
afterAll(function zeroArg() {
order.push("afterAll");
});
afterAll(function withDone(done) {
order.push("afterAll-done");
setImmediate(done);
});

test("zero-arg test", function zeroArg() {
order.push("zero-arg test");
expect(als.getStore()?.tag).toBe("collection");
});

test("one-arg test still receives done", function withDone(done) {
order.push("done test");
expect(typeof done).toBe("function");
expect(als.getStore()?.tag).toBe("collection");
setImmediate(done);
});

describe("nested describe", function zeroArg() {
afterAll(function zeroArg() {
order.push("nested.afterAll");
});
test("passes", function zeroArg() {
order.push("nested.test");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
});
});

test("hooks and tests registered inside an ALS context use the callback's real arity", () => {
expect(order).toEqual([
"beforeAll",
"beforeEach",
"zero-arg test",
"afterEach",
"beforeEach",
"done test",
"afterEach",
"beforeEach",
"nested.test",
"afterEach",
"nested.afterAll",
"afterAll",
"afterAll-done",
]);
});
34 changes: 34 additions & 0 deletions test/js/bun/test/als-hook-arity.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe, normalizeBunSnapshot } from "harness";
import path from "node:path";

// Registering a zero-arg test() or hook inside an active AsyncLocalStorage
// context used to be treated as taking a done callback (the AsyncContextFrame
// wrapper has no `.length`), so the callback would wait for done() and time
// out. Run the fixture with a short per-test timeout so the unfixed build
// fails fast rather than sitting on the 5s default for each entry.
test("tests and hooks registered inside an AsyncLocalStorage context detect done-callback arity correctly", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "--timeout=500", path.join(import.meta.dir, "als-hook-arity.fixture.ts")],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(normalizeBunSnapshot(stderr)).toMatchInlineSnapshot(`
"test/js/bun/test/als-hook-arity.fixture.ts:
(pass) registered inside an active ALS context > zero-arg test
(pass) registered inside an active ALS context > one-arg test still receives done
(pass) registered inside an active ALS context > nested describe > passes
(pass) hooks and tests registered inside an ALS context use the callback's real arity

4 pass
0 fail
4 expect() calls
Ran 4 tests across 1 file."
`);
expect(stdout).not.toContain("timed out");

Check warning on line 32 in test/js/bun/test/als-hook-arity.test.ts

View check run for this annotation

Claude / Claude Code Review

Vacuous assertion: 'timed out' checked on stdout instead of stderr

This assertion is vacuously true: `bun test` writes timeout messages to **stderr**, not stdout, and the fixture writes nothing to stdout — so `expect(stdout).not.toContain("timed out")` passes on both the fixed and unfixed builds. Either change it to `expect(stderr).not.toContain("timed out")` or just drop the line, since the stderr snapshot above already proves no timeout occurred.
Comment thread
robobun marked this conversation as resolved.
Outdated
expect(exitCode).toBe(0);
});
Loading