Re-enable afterAll inside a test#23110
Conversation
|
Updated 6:32 PM PT - Sep 30th, 2025
❌ @pfgithub, your commit 3a41f13 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 23110That installs a local version of the PR into your bun-23110 --bun |
WalkthroughRefactors test execution from index/range-based to pointer-linked entries. Updates ExecutionSequence API and related traversal, order generation, and state reporting. Extends ExecutionEntry with phase, next, and skip pointers. Adjusts DescribeScope methods to pass phase. Simplifies node:test hook handling. Adds new fixtures, regression test, and updates snapshots. Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (6)test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/bun/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/**📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-09-03T17:10:13.486ZApplied to files:
🧬 Code graph analysis (1)test/js/bun/test/bun_test.fixture.ts (1)
⏰ 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)
🔇 Additional comments (2)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/test.ts (1)
59-59: Undefined constant kDefaultFilePath still referenced on line 59. Remove the fallback or restore thekDefaultFilePathdefinition to prevent a runtime crash.
🧹 Nitpick comments (5)
test/regression/issue/23077/23077.test.ts (2)
1-2: Consider importingbunEnvfor test isolation.When spawning Bun in tests, it's best practice to pass
bunEnvto ensure a clean, isolated environment.-import { bunExe } from "harness"; +import { bunEnv, bunExe } from "harness";Then update the spawn call to include
env: bunEnv:const result = Bun.spawn({ cmd: [bunExe(), "test", import.meta.dir + "/a.fixture.ts", import.meta.dir + "/b.fixture.ts"], stdio: ["pipe", "pipe", "pipe"], env: bunEnv, });
10-10: Remove unusedstdoutvariable.The
stdoutvariable is read but never used in assertions or logging.- const stdout = await result.stdout.text(); const stderr = await result.stderr.text();test/js/third_party/grpc-js/test-metadata.test.ts (1)
45-45: Remove debug console.log from third-party test.This console.log appears to be debug code left from development. Third-party test files should remain clean and free of debugging artifacts.
Apply this diff to remove the debug log:
beforeEach(() => { - console.log("create testmetadata"); metadata = new TestMetadata(); });repro.ts (1)
3-11: Consider replacing null pattern with proper resource cleanup.The pattern of using
Set<string> | nullwith manual null assignment in afterEach is unconventional. Since the set is always initialized in beforeEach before each test runs, the null type and assignment add no safety value.Consider simplifying to:
-let set: Set<string> | null; +let set: Set<string>; beforeEach(() => { console.log("outer beforeEach"); set = new Set(); }); afterEach(() => { console.log("outer afterEach"); - set = null; });Then remove the
!operators on lines 16, 17, and 22.src/bun.js/test/bun_test.zig (1)
54-103: Deferred cleanup hooks inside tests are covered by existing tests
The new support for callingafterEach()inside a test (lines 67–69) andafterAll()inside nested blocks (lines 84–87), including sync, promise, and callback variants, is exercised injest-hooks.test.ts, confirming the restored behavior. The insertion logic (lines 75–95) correctly appends execution-phase entries. Consider clarifying the error message on line 76—currently “Cannot call {s}() here. It cannot be called inside a concurrent test…”—to more accurately describe invalid hook registration contexts.
📜 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 (14)
TODO.md(1 hunks)repro.ts(1 hunks)src/bun.js/test/Execution.zig(14 hunks)src/bun.js/test/Order.zig(3 hunks)src/bun.js/test/ScopeFunctions.zig(1 hunks)src/bun.js/test/bun_test.zig(11 hunks)src/bun.js/test/debug.zig(1 hunks)src/js/node/test.ts(6 hunks)test/js/bun/test/bun_test.fixture.ts(1 hunks)test/js/bun/test/bun_test.test.ts(3 hunks)test/js/third_party/grpc-js/test-metadata.test.ts(1 hunks)test/regression/issue/23077/23077.test.ts(1 hunks)test/regression/issue/23077/a.fixture.ts(1 hunks)test/regression/issue/23077/b.fixture.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/bun/test/bun_test.fixture.tstest/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.test.tstest/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.fixture.tstest/js/third_party/grpc-js/test-metadata.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/bun_test.fixture.tstest/js/bun/test/bun_test.test.tstest/js/third_party/grpc-js/test-metadata.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/bun_test.fixture.tstest/js/bun/test/bun_test.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/bun_test.fixture.tstest/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.test.tstest/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.fixture.tstest/js/third_party/grpc-js/test-metadata.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/bun_test.fixture.tstest/js/bun/test/bun_test.test.tstest/js/third_party/grpc-js/test-metadata.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/bun/test/bun_test.fixture.tstest/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.test.tstest/regression/issue/23077/a.fixture.tsrepro.tstest/regression/issue/23077/b.fixture.tstest/js/third_party/grpc-js/test-metadata.test.tssrc/js/node/test.ts
test/regression/issue/+([0-9])/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place regression tests under test/regression/issue/[number] (a numeric directory per issue)
Files:
test/regression/issue/23077/23077.test.tstest/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.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/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.test.tstest/js/third_party/grpc-js/test-metadata.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue number
Files:
test/regression/issue/23077/23077.test.tstest/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.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/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.test.tstest/js/third_party/grpc-js/test-metadata.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/ScopeFunctions.zigsrc/bun.js/test/debug.zigsrc/bun.js/test/Execution.zigsrc/bun.js/test/Order.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/ScopeFunctions.zigsrc/bun.js/test/debug.zigsrc/bun.js/test/Execution.zigsrc/bun.js/test/Order.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/ScopeFunctions.zigsrc/bun.js/test/debug.zigsrc/bun.js/test/Execution.zigsrc/bun.js/test/Order.zigsrc/bun.js/test/bun_test.zig
test/js/third_party/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place third-party npm package tests under test/js/third_party/
Files:
test/js/third_party/grpc-js/test-metadata.test.ts
src/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g.,
node:fs,node:path) undernode/
Files:
src/js/node/test.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts}: Use.$calland.$applyinstead of.callor.applyto avoid user tampering
Use string-literalrequire("...")only (no dynamic or non-literal specifiers)
Author modules as CommonJS-style withrequire(...)and export viaexport default {}(no ESMimport/named exports)
Prefer JSC intrinsics and$-prefixed private APIs for performance and safety (e.g.,$Array,$newArrayWithSize,$putByIdDirectPrivate,$assert,$debug)
Validate function arguments with$isCallableand throw$ERR_INVALID_ARG_TYPEfor invalid callbacks
Use$isObjectand throw appropriate TypeErrors for constructor/initializer inputs that must be objects
Files:
src/js/node/test.ts
src/js/**/*.{js,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
src/js/**/*.{js,ts}: In built-in modules (src/js), require() must use string literals resolved at compile time
In built-in modules (src/js), use export default (converted to a return statement by the preprocessor)
Files:
src/js/node/test.ts
🧠 Learnings (24)
📚 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/bun_test.fixture.tstest/js/bun/test/bun_test.test.tssrc/js/node/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/bun_test.fixture.tstest/regression/issue/23077/23077.test.tstest/regression/issue/23077/a.fixture.tsrepro.tstest/regression/issue/23077/b.fixture.tssrc/js/node/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/bun_test.fixture.tstest/regression/issue/23077/23077.test.tsrepro.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/bun_test.fixture.tstest/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.test.tsrepro.tssrc/js/node/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/regression/issue/23077/23077.test.tssrc/js/node/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 : Name test files `*.test.ts` and use `bun:test`
Applied to files:
test/regression/issue/23077/23077.test.tsrepro.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/regression/issue/23077/23077.test.tstest/js/bun/test/bun_test.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 `using`/`await using` with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Applied to files:
test/regression/issue/23077/23077.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/regression/issue/23077/23077.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/regression/issue/23077/23077.test.tstest/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.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 : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/regression/issue/23077/23077.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} : Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Applied to files:
test/regression/issue/23077/23077.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/regression/issue/23077/23077.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/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/regression/issue/23077/23077.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/bun_test.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} : Prefer snapshot tests over exact stdout equality assertions
Applied to files:
test/js/bun/test/bun_test.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/regression/issue/23077/a.fixture.tsrepro.tstest/regression/issue/23077/b.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 : Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Applied to files:
test/regression/issue/23077/a.fixture.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/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.fixture.tssrc/js/node/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/node/test/** : Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
Applied to files:
test/regression/issue/23077/a.fixture.tstest/regression/issue/23077/b.fixture.ts
📚 Learning: 2025-08-30T00:07:54.573Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/building-bun.mdc:0-0
Timestamp: 2025-08-30T00:07:54.573Z
Learning: Applies to src/**/*.zig : Implement debug logs in Zig using `const log = bun.Output.scoped(.${SCOPE}, false);` and invoking `log("...", .{})`
Applied to files:
src/bun.js/test/debug.zig
📚 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/Order.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:
src/js/node/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} : Use shared utilities from test/harness.ts where applicable
Applied to files:
src/js/node/test.ts
🧬 Code graph analysis (4)
test/js/bun/test/bun_test.fixture.ts (1)
src/js/node/test.ts (2)
afterEach(135-139)beforeEach(129-133)
test/regression/issue/23077/23077.test.ts (1)
test/harness.ts (1)
bunExe(103-106)
repro.ts (1)
src/js/node/test.ts (2)
beforeEach(129-133)afterEach(135-139)
src/js/node/test.ts (1)
test/harness.ts (1)
bunTest(320-335)
🔇 Additional comments (25)
test/regression/issue/23077/a.fixture.ts (1)
1-2: LGTM! Minimal regression fixture is appropriate.This fixture correctly uses
node:testAPI to reproduce issue 23077, where Bun misdetected top-leveltest()calls as nested when running multiple files together. The minimal test with a small async sleep is appropriate for validating the test runner's context detection.test/regression/issue/23077/b.fixture.ts (1)
1-2: LGTM! Intentional duplication for multi-file regression test.This fixture is intentionally identical to
a.fixture.tsto reproduce the conditions of issue 23077, which only manifested when running multiplenode:testfiles together withbun test .TODO.md (1)
1-1: Clarify the TODO timeline and scope.This TODO suggests reverting node:test implementation changes. Given that this PR aims to fix regressions (#23064, #23077), please clarify:
- What specific changes should be reverted?
- What is the timeline for this revert?
- Are the current changes a temporary workaround or the intended long-term fix?
Understanding this will help ensure the fixes are maintainable and don't introduce new technical debt.
test/js/bun/test/bun_test.fixture.ts (1)
272-298: LGTM! Test coverage for afterAll/afterEach inside tests.This test suite correctly validates the PR's core functionality: allowing afterAll and afterEach hooks to be registered from within test blocks. The test structure properly demonstrates:
- Outer hooks at describe level (lines 273-278)
- Inner hooks registered within individual tests (lines 281-286, 290-295)
- Console logs to verify execution order
This addresses the regression mentioned in issue #23064.
test/js/bun/test/bun_test.test.ts (1)
104-106: LGTM! Snapshot updates reflect new test coverage.The snapshot updates correctly capture:
- Three new passing tests for the "after inside test" suite
- Updated test counts (29→32 pass, 43→46 total)
- Proper execution order in stdout showing afterAll/afterEach hooks running in the expected sequence
These changes align with the new test fixtures added in bun_test.fixture.ts.
Based on learnings: Using snapshots with
toMatchSnapshotandnormalizeBunSnapshotas per coding guidelines.Also applies to: 134-134, 139-140, 205-214
src/js/node/test.ts (3)
117-139: LGTM! Hook methods now directly delegate to bunTest().The simplified hook methods (before, after, beforeEach, afterEach) in TestContext now directly call
bunTest()hooks instead of staging them internally. This aligns with the PR objective to re-enable afterAll/afterEach inside tests by removing the overly restrictive internal orchestration.
307-332: Verify synchronous endTest behavior with promises.The refactored
runTestfunction now handles both synchronous and asynchronous test functions, but the promise handling at line 326 callsendTest()in.then()and.catch(). SinceendTestis now a synchronous function that callsdone(error), ensure that:
- The
donecallback properly handles being called from promise continuations- Context restoration (
ctx = originalContext) happens correctly in async scenariosThe logic appears sound, but please verify with a test that covers:
- Synchronous test with error
- Async test (Promise) with success
- Async test (Promise) with rejection
to ensure context is properly restored in all cases.
377-396: LGTM! createHook simplified to match new architecture.The
createHookfunction now returns a simpler{ options, fn: runHook }structure, removing thefnInsideTestvariant. This aligns with the broader refactoring to eliminate internal hook staging and matches the PR objective of allowing hooks to be called from within tests.src/bun.js/test/ScopeFunctions.zig (2)
249-253: LGTM! Phase parameter added to appendTest.The addition of
.collectionas the phase parameter aligns with the broader refactoring to track when tests are registered (collection phase vs execution phase). This supports proper detection of whether a test/hook is being called inside another test, addressing the regressions described in issues #23064 and #23077.
177-181: Verify regression fixture for issue #23064 exists
Found only the fixture for issue #23077 attest/regression/issue/23077/23077.test.ts; the directory or test for #23064 is missing. Please add or confirm a corresponding test covering afterAll/afterEach calls inside the execution phase.src/bun.js/test/debug.zig (1)
37-40: LGTM! Linked-list traversal replaces index-based iteration.The refactored debug loop now uses pointer-based traversal (
sequence.first_entry→entry.next) instead of index-based iteration. This aligns with the broader ExecutionSequence refactoring and provides better support for dynamically managed test entries, which is essential for the PR's goal of allowing hooks to be registered during test execution.src/bun.js/test/Order.zig (4)
5-14: LGTM: Arena allocator integration looks correct.The arena field and updated
initsignature enable efficient allocation of temporary ExecutionEntry nodes during order generation. The arena is correctly stored and used forbun.createcalls ingenerateOrderTest.
85-105: LGTM: EntryList implementation is correct.The
prependandappendmethods correctly maintain a singly-linked list with properfirstandlastpointer tracking. Assertions ensure entries are not already linked before insertion.
107-151: Verify skip_to logic for beforeEach/test/afterEach sequence.Lines 138-144 set
skip_tofor all entries in the sequence. The logic initializesskip_to = current.next(line 140), wherecurrentis the test entry. This means all entries (including beforeEach hooks) will skip to whatever comes after the test entry (the first afterEach or null).Based on the retrieved learning, beforeAll failures should skip to the first afterAll. By analogy, beforeEach failures should likely skip to the first afterEach of the same scope, not to after the test. The comment on line 143 acknowledges this limitation.
Verify the intended skip behavior for beforeEach failures. The current implementation skips all entries to
current.next, which may not align with the scoped skip logic mentioned in the comment.Based on learnings.
42-56: No changes needed: preload entries bypass the assertion and pointers are reset unconditionally.src/bun.js/test/Execution.zig (5)
74-99: LGTM: ExecutionSequence refactor to pointer-based traversal is consistent.The replacement of
active_indexwithactive_entryand addition offirst_entryaligns with the broader refactor to pointer-linked entries. Initialization correctly sets both pointers to the first entry.
411-447: LGTM: Public visibility enables execution state validation in hook registration.Making
getCurrentAndValidExecutionSequencepublic allowsbun_test.zigto validate the current execution context when registering hooks during the execution phase (e.g., afterAll inside a test). The validation logic correctly checks for stale or mismatched state.
186-406: LGTM: active_entry usage is consistent throughout step logic.The refactored step functions correctly use
sequence.active_entryfor null checks, timeout evaluation, and detecting sequence start. The logic properly handles sequence completion whenactive_entryis null.
538-540: LGTM: onSequenceCompleted correctly uses first_entry as fallback for reporting.The updated condition
if (sequence.first_entry) |first_entry|with fallback tosequence.test_entry orelse first_entryensures that hook-only sequences (without a test entry) are still reported correctly.
567-590: No memory leak: execution-phase entries are tracked and destroyedDynamically added
ExecutionEntryobjects are appended tobunTest.extra_execution_entrieswhen created, and inBunTest.deinit(bun_test.zig lines 228–231) each entry is destroyed and the list is deinitialized.src/bun.js/test/bun_test.zig (5)
882-905: LGTM: ExecutionEntry fields support pointer-based sequencing and phase tracking.The addition of
added_in_phase,next, andskip_tofields enables proper linked-list traversal and lifecycle management. TheAddedInPhaseenum distinguishes preload, collection, and execution-phase entries for correct cleanup logic.
183-231: LGTM: extra_execution_entries properly tracks and cleans up dynamic entries.Lines 228-231 correctly iterate and destroy all execution-phase entries allocated during test runs. This ensures no memory leaks for afterAll/afterEach hooks registered inside tests (as referenced in line 92 and Execution.zig line 576).
240-282: LGTM: RefDataValue correctly tracks entry by pointer instead of index.The change from
entry_index: usizetoentry: *const anyopaque(line 249) aligns with the pointer-based execution model. The updatedentry()method (lines 265-270) correctly retrieves the current active entry usinggetCurrentAndValidExecutionSequenceandactive_entry, ensuring the RefDataValue remains valid across sequence state changes.
312-349: LGTM: getCurrentStateData correctly uses active_entry for state tracking.Lines 331-342 correctly compute the current execution state using
sequence.active_entryinstead of index-based logic, storing the entry pointer for validation in later callbacks. The null check on line 331 properly handles completed sequences.
856-875: LGTM: appendTest and appendHook signatures correctly propagate phase tracking.The addition of the
phase: ExecutionEntry.AddedInPhaseparameter to bothappendTest(line 856) andappendHook(line 871) ensures all execution entries are properly tagged with their creation phase. This enables correct lifecycle management and cleanup logic throughout the execution pipeline.
| const result = Bun.spawn({ | ||
| cmd: [bunExe(), "test", import.meta.dir + "/a.fixture.ts", import.meta.dir + "/b.fixture.ts"], | ||
| stdio: ["pipe", "pipe", "pipe"], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use await using for automatic subprocess cleanup.
Per coding guidelines, Bun resources like Bun.spawn should use await using to ensure proper cleanup.
Based on learnings.
- const result = Bun.spawn({
+ await using result = Bun.spawn({
cmd: [bunExe(), "test", import.meta.dir + "/a.fixture.ts", import.meta.dir + "/b.fixture.ts"],
stdio: ["pipe", "pipe", "pipe"],
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = Bun.spawn({ | |
| cmd: [bunExe(), "test", import.meta.dir + "/a.fixture.ts", import.meta.dir + "/b.fixture.ts"], | |
| stdio: ["pipe", "pipe", "pipe"], | |
| }); | |
| await using result = Bun.spawn({ | |
| cmd: [bunExe(), "test", import.meta.dir + "/a.fixture.ts", import.meta.dir + "/b.fixture.ts"], | |
| stdio: ["pipe", "pipe", "pipe"], | |
| }); |
🤖 Prompt for AI Agents
In test/regression/issue/23077/23077.test.ts around lines 5 to 8, the Bun.spawn
call is not using automatic cleanup; change the declaration to use await using
so the subprocess is disposed automatically (for example: await using const
result = Bun.spawn({ cmd: [...], stdio: [...] });), update any subsequent uses
to reference the new variable, and remove any manual cleanup logic that becomes
redundant.
3 participants
Fixes #23064, Fixes #23077