Synchronous concurrent test fix #22928
Conversation
|
Updated 3:26 AM PT - Sep 25th, 2025
❌ @Jarred-Sumner, your commit f117425 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 22928That installs a local version of the PR into your bun-22928 --bun |
WalkthroughPointer-based timespecs replace value-based timing across the test runner; Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/bun.js/test/Collection.zig(1 hunks)src/bun.js/test/Execution.zig(1 hunks)src/bun.js/test/bun_test.zig(2 hunks)test/js/bun/test/concurrent_immediate.fixture.ts(1 hunks)test/js/bun/test/concurrent_immediate.test.ts(1 hunks)test/js/bun/test/concurrent_immediate_error.fixture.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/test/Execution.zigsrc/bun.js/test/Collection.zigsrc/bun.js/test/bun_test.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/test/Execution.zigsrc/bun.js/test/Collection.zigsrc/bun.js/test/bun_test.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/test/Execution.zigsrc/bun.js/test/Collection.zigsrc/bun.js/test/bun_test.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/bun/test/concurrent_immediate.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/bun/test/concurrent_immediate.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
🧠 Learnings (25)
📓 Common learnings
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
📚 Learning: 2025-09-20T05:38:31.896Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: src/bun.js/test/Order.zig:70-78
Timestamp: 2025-09-20T05:38:31.896Z
Learning: In Bun's test Order.zig, the skip logic for beforeAll and afterAll hooks works by calling setFailureSkipTo at specific times: beforeAll failures skip to the first afterAll (setFailureSkipTo called before afterAll generation), and afterAll failures skip to the end (setFailureSkipTo called after afterAll generation). The timing of these calls relative to group generation determines the correct skip targets.
Applied to files:
src/bun.js/test/Execution.zig
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Applied to files:
src/bun.js/test/Execution.zigtest/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/test/Execution.zigsrc/bun.js/test/Collection.zigsrc/bun.js/test/bun_test.zig
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
src/bun.js/test/Execution.zigtest/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/Collection.zig
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
src/bun.js/test/Execution.zigtest/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/Collection.zigtest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: For platform-specific changes, run bun run zig:check-all to compile on all platforms
Applied to files:
src/bun.js/test/Execution.zigsrc/bun.js/test/Collection.zig
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
src/bun.js/test/Execution.zigtest/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/Collection.zigtest/js/bun/test/concurrent_immediate_error.fixture.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/Collection.zig
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/Collection.zigtest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*-fixture.ts : Name test fixture files that are spawned by tests with the suffix `-fixture.ts`
Applied to files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/bun_test.zig
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/bun/test/concurrent_immediate.test.tstest/js/bun/test/concurrent_immediate_error.fixture.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/test/Collection.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/test/Collection.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/test/Collection.zigsrc/bun.js/test/bun_test.zig
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/js/bun/test/concurrent_immediate_error.fixture.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/client-fixture.mjs : Client subprocess logic (page load, IPC, error/reload coordination) should live in client-fixture.mjs and be driven by Client
Applied to files:
test/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `describe` blocks for grouping, `describe.each` for parameterized tests, snapshots with `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`); track resources for cleanup in `afterEach`
Applied to files:
test/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Applied to files:
test/js/bun/test/concurrent_immediate_error.fixture.tstest/js/bun/test/concurrent_immediate.fixture.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/js/bun/test/concurrent_immediate.fixture.ts
🧬 Code graph analysis (3)
test/js/bun/test/concurrent_immediate.test.ts (1)
test/harness.ts (1)
normalizeBunSnapshot(1804-1835)
test/js/bun/test/concurrent_immediate_error.fixture.ts (1)
src/js/node/test.ts (2)
beforeEach(151-160)afterEach(162-171)
test/js/bun/test/concurrent_immediate.fixture.ts (1)
src/js/node/test.ts (2)
beforeEach(151-160)afterEach(162-171)
🔇 Additional comments (8)
src/bun.js/test/Execution.zig (1)
377-381: Proper handling of synchronous test results.The implementation correctly checks for a non-null return from
BunTest.runTestCallbackand advances the sequence immediately, enabling synchronous execution of concurrent tests as intended. This aligns with the PR objective to make concurrent tests with synchronous callbacks continue immediately rather than waiting for the next tick.test/js/bun/test/concurrent_immediate_error.fixture.ts (1)
1-15: Well-structured test fixture for error scenarios.The fixture appropriately defines a test scenario where one concurrent test fails while others succeed, providing necessary coverage for error handling in the synchronous concurrent execution path. The use of lifecycle hooks ensures proper testing of the hook sequencing changes.
src/bun.js/test/Collection.zig (1)
140-145: Consistent handling of optional return values.The implementation properly handles the optional return from
BunTest.runTestCallbackby queuing the result when available, maintaining consistency with the updated control flow in other parts of the test execution system.test/js/bun/test/concurrent_immediate.fixture.ts (1)
1-15: Clean test fixture supporting concurrency validation.The fixture provides a simple, focused test scenario for validating the synchronous execution behavior of concurrent tests with lifecycle hooks. The console.log statements enable verification of execution order in the test that spawns this fixture.
src/bun.js/test/bun_test.zig (3)
546-547: Clear documentation of function behavior change.The comment accurately describes the new return behavior: synchronous results are returned immediately, while asynchronous operations return null.
592-602: Appropriate handling of done callback scenarios.The implementation correctly handles the done callback case by returning null when a done callback reference exists but hasn't been called yet, ensuring the test waits for the asynchronous completion. The microtask draining after setting up the callback is also appropriate.
584-590: Proper handling of promise-based test callbacks — no action needed
Attaching then/catch and calling drain(globalThis) is correct; the VM/test harness already captures/suppresses unhandled rejections (see src/bun.js/test/expect.zig:558-569, src/bun.js/VirtualMachine.zig:672-677, src/cli/Arguments.zig:110-116).test/js/bun/test/concurrent_immediate.test.ts (1)
4-27: Verify snapshot stability across environments — test/js/bun/test/concurrent_immediate.test.tsInline snapshot asserts exact console ordering (beforeEach / start test N / afterEach) for concurrent tests; ordering can be nondeterministic. Reproduce by running the test multiple times (e.g., for i in {1..10}; do bun test test/js/bun/test/concurrent_immediate.test.ts --quiet; echo ---; done). If flaky, relax the snapshot (match per-test groups, use contains/sequential checks, or make the fixture deterministic).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/bun.js/test/bun_test.zig (4)
331-333: Use a more explicit null/undefined guard for the “extra handler” sentinel.Prefer checking
isEmptyOrUndefinedOrNull()to make the intent clear and robust, while keeping the pointer check as a fallback.- if (this_ptr.asPtrAddress() == 0) return; // extra handler + if (this_ptr.isEmptyOrUndefinedOrNull() or this_ptr.asPtrAddress() == 0) return; // extra handler
585-592: Fix typo and clarify intent around unhandled rejection suppression.The first
.then(...)is a deliberate “suppression” handler (with null ref) so draining microtasks won’t surface an unhandled rejection. Fix the typo and make the log wording consistent with the function name.- group.log("callTestCallback -> promise: data {}", .{cfg_data}); - // TODO: supress unhandled rejection error + group.log("runTestCallback -> promise: data {}", .{cfg_data}); + // TODO: suppress unhandled rejection error result_jsvalue.then(globalThis, null, bunTestThen, bunTestCatch);
557-571: Align debug messages with function name.Make log prefixes consistent (“runTestCallback”) for easier tracing.
- group.log("callTestCallback -> appending done callback param: data {}", .{cfg_data}); + group.log("runTestCallback -> appending done callback param: data {}", .{cfg_data}); ... - group.log("callTestCallback -> error", .{}); + group.log("runTestCallback -> error", .{}); ... - group.log("callTestCallback -> wait for done callback", .{}); + group.log("runTestCallback -> wait for done callback", .{}); ... - group.log("callTestCallback -> sync", .{}); + group.log("runTestCallback -> sync", .{});Also applies to: 589-589, 612-620
585-608: Double-attach then handlers is OK; consider a tiny helper for readability.Pattern is sound: attach a no-ref handler to suppress unhandled rejection, drain to detect immediate settlement, then attach the “real” handler if still pending. For readability, consider extracting “attachSuppressionHandler(promise)” to document intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/bun.js/test/bun_test.zig(3 hunks)test/js/bun/test/concurrent_immediate.test.ts(1 hunks)test/js/bun/test/concurrent_immediate_error_promise.fixture.ts(1 hunks)test/js/bun/test/concurrent_immediate_promise.fixture.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/js/bun/test/concurrent_immediate_promise.fixture.ts
🧰 Additional context used
📓 Path-based instructions (11)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
test/js/bun/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/bun/test/concurrent_immediate.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/bun/test/concurrent_immediate.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/test/bun_test.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/test/bun_test.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/test/bun_test.zig
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*-fixture.ts : Name test fixture files that are spawned by tests with the suffix `-fixture.ts`
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `describe` blocks for grouping, `describe.each` for parameterized tests, snapshots with `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`); track resources for cleanup in `afterEach`
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`
Applied to files:
test/js/bun/test/concurrent_immediate_error_promise.fixture.tstest/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-08T04:44:59.101Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-08T04:44:59.101Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, use normalizeBunSnapshot when asserting snapshots
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add a corresponding test case in test/v8/v8.test.ts that invokes checkSameOutput with the new function
Applied to files:
test/js/bun/test/concurrent_immediate.test.tssrc/bun.js/test/bun_test.zig
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/bake-harness.ts : Provide and maintain shared test utilities: devTest, prodTest, devAndProductionTest, Dev, Client, and helpers in the harness
Applied to files:
test/js/bun/test/concurrent_immediate.test.ts
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Applied to files:
src/bun.js/test/bun_test.zig
🧬 Code graph analysis (2)
test/js/bun/test/concurrent_immediate_error_promise.fixture.ts (1)
src/js/node/test.ts (2)
beforeEach(151-160)afterEach(162-171)
test/js/bun/test/concurrent_immediate.test.ts (1)
test/harness.ts (1)
normalizeBunSnapshot(1804-1835)
🔇 Additional comments (6)
test/js/bun/test/concurrent_immediate_error_promise.fixture.ts (1)
1-15: LGTM: succinct fixture exercising immediate rejection in concurrent mode.Covers beforeEach/afterEach interleaving and an immediate thrown error path.
test/js/bun/test/concurrent_immediate.test.ts (4)
4-26: Good coverage: validates interleaving and snapshot-stable stdout.Spawns fixtures with bunExe/bunEnv, asserts exit code and normalized snapshot order. Looks solid.
28-40: Nice cross-check with promise variant.Comparing normalized stdout and stderr (with filename normalization) keeps the test robust across variants.
42-47: Focused stderr filter helper is clear and minimal.Keeps snapshots stable by filtering for pass/fail/error lines.
49-77: Error-path validation looks correct.Asserts non-zero exit and compares filtered stderr across sync/promise error fixtures. Good.
src/bun.js/test/bun_test.zig (1)
547-549: Verify callers adapted to?RefDataValuereturn from runTestCallbackrg over src returned no matches; confirm all call sites (including outside src/tests) were updated to handle the optional return (sync immediate RefDataValue vs async null) and that callers do not double-enqueue results.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/test/bun_test.zig (1)
547-625: Fix ref-leak and reentrancy: only set timers when waiting; don’t drain globally on sync pathTwo intertwined issues likely tied to the CI crashes (segfaults, “threadlock locked by unexpected thread”):
- Ref leak: dcb_ref is allocated before promise-state decision. If the callback returns an already-fulfilled/rejected promise and never calls done(), dcb_ref remains attached to DoneCallback.ref and is never deref’d.
- Global reentrancy: drain(globalThis) is called on the synchronous/immediate path and in the done-callback-wait path. That runs handleRejectedPromises(), which can surface unrelated unhandled rejections mid-step and trigger nested scheduling, increasing chances of threadlock assertions and flakiness at scale.
Suggested fix:
- Only set/update the timer when we’re actually waiting (pending promise or waiting for done()).
- Only attach DoneCallback.ref when we’re waiting.
- Remove drain(globalThis) from both the “sync” path and the done-wait path; rely on the event loop and existing scheduling (bunTestThen/bunTestCatch and runNextTick) to advance. If you must flush microtasks to inspect a returned promise, call vm.drainMicrotasks() (as you already do) but avoid handleRejectedPromises() here.
Apply this diff within runTestCallback:
- this.updateMinTimeout(globalThis, timeout); const result: ?jsc.JSValue = cfg_callback.call(globalThis, .js_undefined, if (done_arg) |done| &.{done} else &.{}) catch blk: { globalThis.clearTerminationException(); this.onUncaughtException(globalThis, globalThis.tryTakeException(), false, cfg_data); group.log("callTestCallback -> error", .{}); break :blk null; }; - var dcb_ref: ?*RefData = null; - if (done_callback) |dcb| { - if (DoneCallback.fromJS(dcb)) |dcb_data| { - if (dcb_data.called or result == null) { - // done callback already called or the callback errored; add result immediately - } else { - dcb_ref = ref(this_strong, cfg_data); - dcb_data.ref = dcb_ref; - } - } else bun.debugAssert(false); // this should be unreachable, we create DoneCallback above - } + // Attach done ref only if we end up waiting (pending promise or waiting for done()). + var dcb_ref: ?*RefData = null; if (result) |result_jsvalue| if (result_jsvalue.asPromise()) |promise| { defer result_jsvalue.ensureStillAlive(); // because sometimes we use promise without result group.log("callTestCallback -> promise: data {}", .{cfg_data}); const vm = globalThis.bunVM(); vm.drainMicrotasks(); switch (promise.status(globalThis.vm())) { .pending => { // not immediately resolved; register 'then' to handle the result when it becomes available - const this_ref: *RefData = if (dcb_ref) |dcb_ref_value| dcb_ref_value.dupe() else ref(this_strong, cfg_data); + const this_ref: *RefData = ref(this_strong, cfg_data); + // If a done() is expected and wasn't already called, attach a ref for it too. + if (done_callback) |dcb| { + if (DoneCallback.fromJS(dcb)) |dcb_data| { + if (!dcb_data.called) { + dcb_ref = this_ref.dupe(); + dcb_data.ref = dcb_ref; + } + } else bun.debugAssert(false); + } result_jsvalue.then(globalThis, this_ref, bunTestThen, bunTestCatch); + // Only update timeout when we're actually waiting. + this.updateMinTimeout(globalThis, timeout); return null; }, .fulfilled => { // Do not register a then callback when it's already fulfilled. - return cfg_data; + return cfg_data; }, .rejected => { // Prevent the promise from entering the promise rejection queue. promise.setHandled(globalThis.vm()); const value = promise.result(globalThis.vm()); this.onUncaughtException(globalThis, value, true, cfg_data); return cfg_data; }, } }; - if (dcb_ref) |_| { - // completed asynchronously - group.log("callTestCallback -> wait for done callback", .{}); - drain(globalThis); - return null; - } + // No promise result; if done() is expected and wasn't called synchronously, wait for it. + if (done_callback) |dcb| { + if (DoneCallback.fromJS(dcb)) |dcb_data| { + if (!dcb_data.called) { + dcb_ref = ref(this_strong, cfg_data); + dcb_data.ref = dcb_ref; + this.updateMinTimeout(globalThis, timeout); + group.log("callTestCallback -> wait for done callback", .{}); + return null; + } + } else bun.debugAssert(false); + } group.log("callTestCallback -> sync", .{}); - drain(globalThis); return cfg_data;Why this matters:
- Eliminates a dangling RefData when promise is already settled (memory/scope leak and possible hangs).
- Reduces global microtask/rejection handling during sensitive sections, which can trigger out-of-order callbacks and the threadlock assertion you’re seeing in CI.
🧹 Nitpick comments (1)
src/bun.js/test/bun_test.zig (1)
331-333: Guard is fine; consider debug logging for unexpected pathsEarly-returning when this_ptr is empty/undefined/null prevents crashes. For observability, add a low-cost debug log (behind group.getLogEnabled()) to help track unexpected invocations; otherwise these drops can cause hangs that are hard to diagnose.
- if (this_ptr.isEmptyOrUndefinedOrNull()) return; + if (this_ptr.isEmptyOrUndefinedOrNull()) { + if (group.getLogEnabled()) group.log("bunTestThenOrCatch: missing this_ptr; ignoring then/catch", .{}); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/bun.js/test/Collection.zig(1 hunks)src/bun.js/test/Execution.zig(6 hunks)src/bun.js/test/bun_test.zig(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bun.js/test/Collection.zig
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/test/Execution.zigsrc/bun.js/test/bun_test.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/test/Execution.zigsrc/bun.js/test/bun_test.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/test/Execution.zigsrc/bun.js/test/bun_test.zig
🧠 Learnings (5)
📚 Learning: 2025-09-24T05:48:59.851Z
Learnt from: nektro
PR: oven-sh/bun#22806
File: scripts/runner.node.mjs:687-689
Timestamp: 2025-09-24T05:48:59.851Z
Learning: In the Bun codebase, the `startGroup` utility function in scripts/runner.node.mjs automatically closes any previously open group when called, so `startGroup("End")` correctly closes the final test group and keeps subsequent output ungrouped.
Applied to files:
src/bun.js/test/Execution.zig
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/**/*.zig : Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-08-30T00:11:57.076Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-08-30T00:11:57.076Z
Learning: Applies to src/{**/js_*.zig,bun.js/api/**/*.zig} : Use bun.JSError!JSValue for proper error propagation in JS-exposed Zig functions
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-08-30T00:13:36.815Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-08-30T00:13:36.815Z
Learning: Applies to src/bun.js/bindings/generated_classes_list.zig : Update src/bun.js/bindings/generated_classes_list.zig to include new classes
Applied to files:
src/bun.js/test/bun_test.zig
🔇 Additional comments (10)
src/bun.js/test/Execution.zig (7)
225-231: Pointer-plumbed "now" looks correctSwitching to a single, mutable now and passing its pointer down aligns with the new APIs and avoids repeated syscalls. No issues spotted here.
246-253: Correct: propagate now by pointer to stepSequence/stepGroupThe changes correctly forward now to downstream calls and re-use it after immediate callbacks.
259-279: *API alignment: stepGroup(now: bun.timespec)Signature and use sites are consistent with bun_test.zig updates. Good.
298-314: Min-timeout aggregation preserves earliest deadlineUsing minIgnoreEpoch across sequences is correct and keeps behavior stable.
323-327: Re-run loop structure is soundReturning null to continue within the same loop achieves immediate progression without yielding to the timer.
329-346: Timeout evaluation on active entry is correctUsing active_entry.evaluateTimeout(sequence, now) to decide whether to advance vs. wait matches the new evaluateTimeout contract.
374-381: Avoid unnecessary global microtask effects; keep behavior localAfter an immediate callback result, you refresh now and evaluate the timeout, then advance. That is good. To minimize reentrancy and global side-effects, avoid any additional microtask draining at this point (none here), and keep this path strictly synchronous as you’ve done. No change requested; just calling out this is the right place to stay synchronous.
If you see any lingering unexpected reentrancy, we should double‑check that runTestCallback doesn’t drain/handle global rejected promises on the “sync” path (see my comments in bun_test.zig).
src/bun.js/test/bun_test.zig (3)
473-488: Pointer-based timeout update is correctSignature change and logging dereference look good. Timer min-check remains monotonic, consistent with prior behavior.
866-883: evaluateTimeout behavior is solid; matches callsitesSets the correct failure variant based on entry type and done-ness, toggles maybe_skip for skipping to afterEach, and returns a simple bool. No changes requested.
547-625: Run a targeted verification to catch ref leaks and premature drains
- Ensure no remaining drain(globalThis) calls exist in runTestCallback after applying the above patch.
- Verify DoneCallback.ref is only set when awaiting (pending promise or done()) and is cleared/dereferenced exactly once.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/bun.js/event_loop.zig (1)
185-192: Enter/exit the event loop and avoid double-drain; add VM consistency assertWrap the forced-drain path with enter()/exitMaybeDrainMicrotasks(false) to preserve EventLoop invariants (entered_event_loop_count, Debug state) without draining twice. Also assert the provided global belongs to this loop’s VM to prevent cross-VM drain hazards.
-pub fn runCallbackWithResultAndForcefullyDrainMicrotasks(this: *EventLoop, callback: jsc.JSValue, globalObject: *jsc.JSGlobalObject, thisValue: jsc.JSValue, arguments: []const jsc.JSValue) !jsc.JSValue { - const result = try callback.call(globalObject, thisValue, arguments); - result.ensureStillAlive(); - try this.drainMicrotasksWithGlobal(globalObject, globalObject.bunVM().jsc_vm); - return result; -} +pub fn runCallbackWithResultAndForcefullyDrainMicrotasks(this: *EventLoop, callback: jsc.JSValue, globalObject: *jsc.JSGlobalObject, thisValue: jsc.JSValue, arguments: []const jsc.JSValue) !jsc.JSValue { + this.enter(); + defer this.exitMaybeDrainMicrotasks(false) catch {}; + + if (comptime Environment.allow_assert) { + if (globalObject.bunVM() != this.virtual_machine) { + @panic("EventLoop.runCallbackWithResultAndForcefullyDrainMicrotasks: globalObject VM != event loop VM"); + } + } + + const result = try callback.call(globalObject, thisValue, arguments); + result.ensureStillAlive(); + try this.drainMicrotasksWithGlobal(globalObject, globalObject.bunVM().jsc_vm); + return result; +}Follow-up:
- Confirm all call sites expect and handle the error union return (!jsc.JSValue).
- Confirm this helper is only used where globalObject and this.virtual_machine are the same VM.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/event_loop.zig(1 hunks)src/bun.js/test/bun_test.zig(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bun.js/test/bun_test.zig
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/event_loop.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/event_loop.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/event_loop.zig
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/bun.js/test/Execution.zig (2)
298-314: Earliest-timeout aggregation is fine; minor readability nitminIgnoreEpoch is the right behavior. Consider naming prev_timeout → earliest_timeout for clarity.
- const prev_timeout: bun.timespec = if (final_status == .execute) final_status.execute.timeout else .epoch; - const this_timeout = exec.timeout; - final_status = .{ .execute = .{ .timeout = prev_timeout.minIgnoreEpoch(this_timeout) } }; + const earliest_timeout: bun.timespec = if (final_status == .execute) final_status.execute.timeout else .epoch; + final_status = .{ .execute = .{ .timeout = earliest_timeout.minIgnoreEpoch(exec.timeout) } };
374-381: Immediate-callback fast path verifiedrunTestCallback drains microtasks via runCallbackWithResultAndForcefullyDrainMicrotasks and evaluateTimeout correctly classifies timeouts; fast‐path interleaving behaves as intended. Optional: add debug log to trace fast‐path behavior:
- // the result is available immediately; advance the sequence and run again. + // the result is available immediately; advance the sequence and run again. + groupLog.log("runSequence: callback returned immediately; advancing and rerunning", .{});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/test/Execution.zig(6 hunks)test/js/node/fs/fs.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})
Files:
src/bun.js/test/Execution.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/bun.js/test/Execution.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/test/Execution.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/fs/fs.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/fs/fs.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/fs/fs.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/fs/fs.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/node/fs/fs.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/node/fs/fs.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions
Files:
test/js/node/fs/fs.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/node/fs/fs.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`
Learnt from: pfgithub
PR: oven-sh/bun#22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
📚 Learning: 2025-09-24T05:48:59.851Z
Learnt from: nektro
PR: oven-sh/bun#22806
File: scripts/runner.node.mjs:687-689
Timestamp: 2025-09-24T05:48:59.851Z
Learning: In the Bun codebase, the `startGroup` utility function in scripts/runner.node.mjs automatically closes any previously open group when called, so `startGroup("End")` correctly closes the final test group and keeps subsequent output ungrouped.
Applied to files:
src/bun.js/test/Execution.zig
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/js/node/fs/fs.test.ts
🧬 Code graph analysis (1)
test/js/node/fs/fs.test.ts (1)
src/js/node/wasi.ts (1)
fstatSync(1795-1824)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (6)
test/js/node/fs/fs.test.ts (2)
16-16: Import update looks good.Bringing in
ftruncateSyncfromnode:fscleanly supports the new usage below.
2720-2726: Nice improvement usingftruncateSyncfor sparse growth.This keeps the test fast, avoids multi-gigabyte writes, and still verifies the intended 5 GB size accurately.
src/bun.js/test/Execution.zig (4)
225-253: Plumbing a single 'now' across a step looks correctCreating one now at step entry and threading it by pointer keeps timeout decisions consistent within the same tick. The call sites in this block are updated appropriately.
323-327: Tail-recursive loop via nullable return is cleanThe stepSequence trampoline pattern is clear and avoids extra state. No concerns.
329-346: Timeout evaluation switched to evaluateTimeout(..., now) — goodUsing the shared now pointer here removes per-call clock skew. The behavior on timeout (advance + continue) stays consistent.
259-296: *stepGroup(now: bun.timespec) — callers don’t retain the pointer
All found invocations pass&nowdirectly and immediately return; none store the pointer beyond the call chain. Approving.
Uh oh!
There was an error while loading. Please reload this page.