feat(test): implement onTestFinished hook for bun:test#24038
Conversation
This adds the onTestFinished() function to bun:test, which runs after all afterEach hooks have completed. This allows cleanup code that needs to run at the very end of a test. Execution order: 1. Test body 2. afterAll hooks (if added inside test) 3. afterEach hooks 4. onTestFinished hooks onTestFinished can only be called inside a test, not in describe blocks or during preload. Multiple onTestFinished hooks can be registered and they execute in the order they were added. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 4:33 PM PT - Oct 24th, 2025
❌ @pfgithub, your commit c17b1d9 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 24038That installs a local version of the PR into your bun-24038 --bun |
Cleaner code structure using a switch statement to determine the append point based on the hook tag, rather than conditional checks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplified the loop logic to break directly to the test_entry when found, and fallback to active_entry when not found, matching the original behavior exactly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove redundant outer if-check by moving the error handling into the switch statement's else branch. Cleaner control flow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add TypeScript type definition for onTestFinished function in bun:test module with documentation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add onTestFinished to the test lifecycle hooks documentation table. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace manual for-loop assertions with cleaner expect().toEqual() calls in onTestFinished tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Increment the initial object size from 19 to 20 to account for the new onTestFinished export. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove unnecessary output.push("test 2") and check full output array
instead of using slice.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
pfgithub
left a comment
pfgithub
left a comment
There was a problem hiding this comment.
wording is a bit confusing for the docs. pending CI.
WalkthroughAdds a new onTestFinished lifecycle hook across docs, types, runtime, and tests; updates mock API usage in docs to use Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 (5)
docs/cli/test.md(1 hunks)packages/bun-types/test.d.ts(1 hunks)src/bun.js/test/bun_test.zig(2 hunks)src/bun.js/test/jest.zig(2 hunks)test/js/bun/test/test-on-test-finished.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.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
Files:
src/bun.js/test/jest.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/jest.zigsrc/bun.js/test/bun_test.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/test/jest.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/test-on-test-finished.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/test-on-test-finished.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/test-on-test-finished.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/test-on-test-finished.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/test/test-on-test-finished.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/test/test-on-test-finished.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/test/test-on-test-finished.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be created under test/ and end with .test.ts or .test.tsx
In tests that open network ports, always use port: 0; do not hardcode port numbers or roll your own random port
Use normalizeBunSnapshot to normalize snapshot output instead of comparing raw stdout
Do not write tests that merely assert absence of strings like "panic" or "uncaught exception"
Use tempDir from "harness" to create temporary directories; do not use fs.mkdtempSync or tmpdirSync
When spawning processes in tests, assert output before asserting the exit code
Avoid shell commands in tests (e.g., find, grep); prefer Bun's Glob and built-in tools
Prefer snapshot tests over direct string equality assertions on stdout
Files:
test/js/bun/test/test-on-test-finished.test.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
📚 Learning: 2025-08-30T00:11:00.890Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/javascriptcore-class.mdc:0-0
Timestamp: 2025-08-30T00:11:00.890Z
Learning: Applies to **/*.zig : Wrap the Bun__<Type>__toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Applied to files:
src/bun.js/test/jest.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/jest.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/js/bun/test/test-on-test-finished.test.tsdocs/cli/test.md
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Do not set explicit test timeouts; Bun already has timeouts
Applied to files:
src/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/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For `test/js/node/test/{parallel,sequential}/*.js` (no .test extension), run with `bun bd <file>` instead of `bun bd test <file>`
Applied to files:
src/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} : 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/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:
docs/cli/test.md
🧬 Code graph analysis (1)
test/js/bun/test/test-on-test-finished.test.ts (1)
src/js/node/test.ts (1)
afterEach(135-139)
🔇 Additional comments (3)
src/bun.js/test/bun_test.zig (1)
92-101: Tail-append logic for onTestFinished looks correct (preserves registration order)Finding the last entry from active_entry and inserting maintains FIFO for multiple registrations and ensures it runs after afterEach. Good.
If you want to double‑check interleaving with afterEach/afterAll added inside the test, consider a stress test with mixed sync/async hooks to assert final sequence.
src/bun.js/test/jest.zig (1)
166-166: Binding added and capacity bumped — LGTMExposes
onTestFinishedand adjusts object preallocation. Looks good.Also applies to: 186-186
packages/bun-types/test.d.ts (1)
361-382: No duplicate declarations found - single onTestFinished export confirmedVerification confirms there is exactly one
onTestFinishedfunction declaration in the type definitions atpackages/bun-types/test.d.ts:379. All other matches in the repository are function calls in test files, not conflicting declarations.
| | Hook | Description | | ||
| | ---------------- | -------------------------------------------------------- | | ||
| | `beforeAll` | Runs once before all tests. | | ||
| | `beforeEach` | Runs before each test. | | ||
| | `afterEach` | Runs after each test. | | ||
| | `afterAll` | Runs once after all tests. | | ||
| | `onTestFinished` | Runs after a test finishes, including after `afterEach`. | | ||
|
|
There was a problem hiding this comment.
Document exception: onTestFinished isn’t allowed in preload or concurrent tests
Right below, the docs say hooks can be defined in a preloaded file; that excludes onTestFinished. Add a note after the table.
| `afterAll` | Runs once after all tests. |
| `onTestFinished` | Runs after a test finishes, including after `afterEach`. |
+
+> Note
+> `onTestFinished` can only be registered while a test is executing. It is not allowed in `describe` blocks, cannot be used during `--preload`, and is not supported inside `test.concurrent` tests.📝 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.
| | Hook | Description | | |
| | ---------------- | -------------------------------------------------------- | | |
| | `beforeAll` | Runs once before all tests. | | |
| | `beforeEach` | Runs before each test. | | |
| | `afterEach` | Runs after each test. | | |
| | `afterAll` | Runs once after all tests. | | |
| | `onTestFinished` | Runs after a test finishes, including after `afterEach`. | | |
| | Hook | Description | | |
| | ---------------- | -------------------------------------------------------- | | |
| | `beforeAll` | Runs once before all tests. | | |
| | `beforeEach` | Runs before each test. | | |
| | `afterEach` | Runs after each test. | | |
| | `afterAll` | Runs once after all tests. | | |
| | `onTestFinished` | Runs after a test finishes, including after `afterEach`. | | |
| > Note | |
| > `onTestFinished` can only be registered while a test is executing. It is not allowed in `describe` blocks, cannot be used during `--preload`, and is not supported inside `test.concurrent` tests. |
🤖 Prompt for AI Agents
In docs/cli/test.md around lines 260 to 267, the hooks table lists
onTestFinished but the docs later say hooks can be defined in a preloaded file —
add a short note immediately after this table stating that onTestFinished is not
permitted in preload files and is also not supported for tests run with the
concurrent option; update wording to explicitly exclude onTestFinished from
preload and concurrent contexts.
| /** | ||
| * Runs a function after a test finishes, including after all afterEach hooks. | ||
| * | ||
| * This is useful for cleanup tasks that need to run at the very end of a test, | ||
| * after all other hooks have completed. | ||
| * | ||
| * Can only be called inside a test, not in describe blocks. | ||
| * | ||
| * @example | ||
| * test("my test", () => { | ||
| * onTestFinished(() => { | ||
| * // This runs after all afterEach hooks | ||
| * console.log("Test finished!"); | ||
| * }); | ||
| * }); | ||
| * | ||
| * @param fn the function to run | ||
| */ | ||
| export function onTestFinished( | ||
| fn: (() => void | Promise<unknown>) | ((done: (err?: unknown) => void) => void), | ||
| options?: HookOptions, | ||
| ): void; |
There was a problem hiding this comment.
Align JSDoc with runtime: add preload and concurrent-test constraints
Runtime throws in preload and when called inside a concurrent test; types should say so. Update the comment accordingly.
/**
- * Runs a function after a test finishes, including after all afterEach hooks.
+ * Runs a function after a test finishes, including after all afterEach hooks.
*
* This is useful for cleanup tasks that need to run at the very end of a test,
* after all other hooks have completed.
*
- * Can only be called inside a test, not in describe blocks.
+ * Can only be called while a test is executing. Not allowed in `describe` blocks,
+ * not allowed during `--preload`, and not supported inside `test.concurrent` tests.📝 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.
| /** | |
| * Runs a function after a test finishes, including after all afterEach hooks. | |
| * | |
| * This is useful for cleanup tasks that need to run at the very end of a test, | |
| * after all other hooks have completed. | |
| * | |
| * Can only be called inside a test, not in describe blocks. | |
| * | |
| * @example | |
| * test("my test", () => { | |
| * onTestFinished(() => { | |
| * // This runs after all afterEach hooks | |
| * console.log("Test finished!"); | |
| * }); | |
| * }); | |
| * | |
| * @param fn the function to run | |
| */ | |
| export function onTestFinished( | |
| fn: (() => void | Promise<unknown>) | ((done: (err?: unknown) => void) => void), | |
| options?: HookOptions, | |
| ): void; | |
| /** | |
| * Runs a function after a test finishes, including after all afterEach hooks. | |
| * | |
| * This is useful for cleanup tasks that need to run at the very end of a test, | |
| * after all other hooks have completed. | |
| * | |
| * Can only be called while a test is executing. Not allowed in `describe` blocks, | |
| * not allowed during `--preload`, and not supported inside `test.concurrent` tests. | |
| * | |
| * @example | |
| * test("my test", () => { | |
| * onTestFinished(() => { | |
| * // This runs after all afterEach hooks | |
| * console.log("Test finished!"); | |
| * }); | |
| * }); | |
| * | |
| * @param fn the function to run | |
| */ | |
| export function onTestFinished( | |
| fn: (() => void | Promise<unknown>) | ((done: (err?: unknown) => void) => void), | |
| options?: HookOptions, | |
| ): void; |
🤖 Prompt for AI Agents
In packages/bun-types/test.d.ts around lines 361 to 382, the JSDoc for
onTestFinished is out of sync with runtime behavior: the runtime throws if
called from a preload script or from inside a concurrent test. Update the
comment to explicitly state those constraints (cannot be used in preload and
cannot be used inside concurrent tests), and mention that runtime will throw in
those cases; keep existing examples and param description but add brief notes
about the preload and concurrent-test restrictions.
Change the error message for onTestFinished when called inside a concurrent test to suggest using test.serial or removing test.concurrent, which is more helpful than the generic message. Also add tests to verify the error message works correctly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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/bun_test.zig(2 hunks)test/js/bun/test/test-on-test-finished.test.ts(1 hunks)
🧰 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/test-on-test-finished.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/test-on-test-finished.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/test-on-test-finished.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/test-on-test-finished.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/bun/test/test-on-test-finished.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/bun/test/test-on-test-finished.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/bun/test/test-on-test-finished.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must be created under test/ and end with .test.ts or .test.tsx
In tests that open network ports, always use port: 0; do not hardcode port numbers or roll your own random port
Use normalizeBunSnapshot to normalize snapshot output instead of comparing raw stdout
Do not write tests that merely assert absence of strings like "panic" or "uncaught exception"
Use tempDir from "harness" to create temporary directories; do not use fs.mkdtempSync or tmpdirSync
When spawning processes in tests, assert output before asserting the exit code
Avoid shell commands in tests (e.g., find, grep); prefer Bun's Glob and built-in tools
Prefer snapshot tests over direct string equality assertions on stdout
Files:
test/js/bun/test/test-on-test-finished.test.ts
**/*.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
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
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/test/bun_test.zig
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/js/bun/test/test-on-test-finished.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Do not set explicit test timeouts; Bun already has timeouts
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending with `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
src/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/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
src/bun.js/test/bun_test.zig
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer concurrent tests (`test.concurrent`/`describe.concurrent`) over sequential when feasible
Applied to files:
src/bun.js/test/bun_test.zig
🧬 Code graph analysis (1)
test/js/bun/test/test-on-test-finished.test.ts (1)
src/js/node/test.ts (1)
afterEach(135-139)
🔇 Additional comments (9)
src/bun.js/test/bun_test.zig (4)
59-61: LGTM!The preload guard correctly prevents
onTestFinishedfrom being called during preload, with a clear error message directing users to call it inside a test.
70-72: LGTM!The collection phase guard correctly prevents
onTestFinishedfrom being called outside of a test (e.g., at the top level or inside describe blocks), with an appropriate error message.
78-85: Well done addressing the previous feedback!The error message now provides specific, actionable guidance for
onTestFinishedin concurrent tests, while maintaining appropriate messages for other hooks. This resolves the concern raised in the previous review.
87-112: LGTM!The append point logic is well-structured and correctly implements the execution order:
afterAll/afterEachhooks are inserted after the test entryonTestFinishedhooks are appended at the very end of the sequence by traversing to the last entry- The linked list insertion properly maintains the execution chain
This ensures the declared execution order: test body → afterAll → afterEach → onTestFinished.
test/js/bun/test/test-on-test-finished.test.ts (5)
4-25: LGTM!The basic ordering test correctly verifies that
onTestFinishedexecutes after all other hooks, including innerafterAllandafterEach. The use of a shared array to capture execution order is a clear and effective testing pattern.
28-48: LGTM!This test correctly verifies that multiple
onTestFinishedcallbacks execute in registration order and after all other hooks. Good coverage for a common use case.
51-69: LGTM!This test properly verifies that async
onTestFinishedcallbacks are awaited before proceeding to the next test. The minimal timeout keeps the test fast while still demonstrating async behavior.
72-92: LGTM!This test correctly verifies error handling for
onTestFinishedin concurrent tests, with the error message matching the implementation. This addresses the concurrent test scenario mentioned in the previous review feedback.
95-116: LGTM!This integration test provides comprehensive verification of the complete execution order when
onTestFinishedis used alongside all other lifecycle hooks. Good coverage for the most complex scenario.
| import { afterAll, afterEach, describe, expect, onTestFinished, test } from "bun:test"; | ||
|
|
||
| // Test the basic ordering of onTestFinished | ||
| describe("onTestFinished ordering", () => { | ||
| const output: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| output.push("afterEach"); | ||
| }); | ||
|
|
||
| test("test 1", () => { | ||
| afterAll(() => { | ||
| output.push("inner afterAll"); | ||
| }); | ||
| onTestFinished(() => { | ||
| output.push("onTestFinished"); | ||
| }); | ||
| output.push("test 1"); | ||
| }); | ||
|
|
||
| test("test 2", () => { | ||
| // After test 2 starts, verify the order from test 1 | ||
| expect(output).toEqual(["test 1", "inner afterAll", "afterEach", "onTestFinished"]); | ||
| }); | ||
| }); | ||
|
|
||
| // Test multiple onTestFinished calls | ||
| describe("multiple onTestFinished", () => { | ||
| const output: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| output.push("afterEach"); | ||
| }); | ||
|
|
||
| test("test with multiple onTestFinished", () => { | ||
| onTestFinished(() => { | ||
| output.push("onTestFinished 1"); | ||
| }); | ||
| onTestFinished(() => { | ||
| output.push("onTestFinished 2"); | ||
| }); | ||
| output.push("test"); | ||
| }); | ||
|
|
||
| test("verify order", () => { | ||
| expect(output).toEqual(["test", "afterEach", "onTestFinished 1", "onTestFinished 2"]); | ||
| }); | ||
| }); | ||
|
|
||
| // Test onTestFinished with async callbacks | ||
| describe("async onTestFinished", () => { | ||
| const output: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| output.push("afterEach"); | ||
| }); | ||
|
|
||
| test("async onTestFinished", async () => { | ||
| onTestFinished(async () => { | ||
| await new Promise(resolve => setTimeout(resolve, 1)); | ||
| output.push("onTestFinished async"); | ||
| }); | ||
| output.push("test"); | ||
| }); | ||
|
|
||
| test("verify async order", () => { | ||
| expect(output).toEqual(["test", "afterEach", "onTestFinished async"]); | ||
| }); | ||
| }); | ||
|
|
||
| // Test that onTestFinished throws proper error in concurrent tests | ||
| describe("onTestFinished errors", () => { | ||
| test.concurrent("cannot be called in concurrent test 1", () => { | ||
| expect(() => { | ||
| onTestFinished(() => { | ||
| console.log("should not run"); | ||
| }); | ||
| }).toThrow( | ||
| "Cannot call onTestFinished() here. It cannot be called inside a concurrent test. Use test.serial or remove test.concurrent.", | ||
| ); | ||
| }); | ||
|
|
||
| test.concurrent("cannot be called in concurrent test 2", () => { | ||
| expect(() => { | ||
| onTestFinished(() => { | ||
| console.log("should not run"); | ||
| }); | ||
| }).toThrow( | ||
| "Cannot call onTestFinished() here. It cannot be called inside a concurrent test. Use test.serial or remove test.concurrent.", | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| // Test onTestFinished with afterEach and afterAll together | ||
| describe("onTestFinished with all hooks", () => { | ||
| const output: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| output.push("afterEach"); | ||
| }); | ||
|
|
||
| test("test with all hooks", () => { | ||
| afterAll(() => { | ||
| output.push("inner afterAll"); | ||
| }); | ||
| onTestFinished(() => { | ||
| output.push("onTestFinished"); | ||
| }); | ||
| output.push("test"); | ||
| }); | ||
|
|
||
| test("verify complete order", () => { | ||
| // Expected order: test body, inner afterAll, afterEach, onTestFinished | ||
| expect(output).toEqual(["test", "inner afterAll", "afterEach", "onTestFinished"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding negative tests for invalid contexts.
The test suite provides excellent coverage for valid usage and concurrent test errors. However, per the previous review feedback, consider adding tests that verify errors when onTestFinished is called in invalid contexts (top-level or inside describe blocks).
These tests would likely need to spawn separate test processes and assert on the error output, similar to how other framework-level error cases are tested in the Bun test suite.
Do you want me to help draft these negative test cases that spawn child processes to verify the error behavior for top-level and describe-level invocations?
🤖 Prompt for AI Agents
In test/js/bun/test/test-on-test-finished.test.ts lines 1-116, add negative
tests that spawn separate test processes to ensure onTestFinished throws when
called in invalid contexts (top-level and inside describe). For each case create
a temporary test file string that calls onTestFinished at top-level or inside a
describe, spawn the test runner (e.g., using child_process.spawnSync or
Bun.spawnSync) to run that file, assert the process exits non-zero and stderr
contains the expected error message about "Cannot call onTestFinished() here"
(matching the existing concurrent test error text), and clean up the temp file;
place these new tests in the same suite near the "onTestFinished errors" block.
3 participants
Summary
Implements
onTestFinished()forbun:test, which runs after allafterEachhooks have completed.Implementation
onTestFinishedexport to the test module injest.ziggenericHookinbun_test.zigto handleonTestFinishedas a special case that:Execution Order
When called inside a test:
afterAllhooks (if added inside the test)afterEachhooksonTestFinishedhooks ✨Test Plan
bun bd testUSE_SYSTEM_BUN=1(feature not in released version)afterEach,afterAll, and multipleonTestFinishedcallsonTestFinishedcallbacks🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com