Fix fetch-preconnect test failure #23016
Conversation
|
Updated 4:29 PM PT - Sep 26th, 2025
❌ @Jarred-Sumner, your commit 2b3d893 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 23016That installs a local version of the PR into your bun-23016 --bun |
WalkthroughMoved the pending_sockets insertion in HTTPContext.zig to occur after socket validity checks. Updated fetch-preconnect tests to add bunEnv/bunExe imports, use typed Promise.withResolvers, adjust Windows skip to concurrent, and invoke Bun.spawn for running the command, asserting successful exit. Changes
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/fetch/fetch-preconnect.test.ts (1)
89-112: Await the listener promise before checking exitAfter spawning, the test never awaits
promisefromPromise.withResolvers, so it may pass before the preconnect handshake occurs. Add:await using proc = Bun.spawn({ cmd: [bunExe(), `--fetch-preconnect=http://localhost:${listener.port}`, "--eval", "Bun.sleep(64)"], stdio: ["inherit", "inherit", "inherit"], env: bunEnv, }); + await promise; expect(await proc.exited).toBe(0);
🧹 Nitpick comments (1)
src/http/HTTPContext.zig (1)
426-429: Good fix; additionally neutralize the ext pointer before freeing the pool entryMoving pool reinsertion until after closed/error checks prevents re-enqueuing bad sockets. To further tighten safety, set the socket’s ext to a neutral tag before returning the pooled entry to the allocator, so the ext doesn’t briefly point at memory that’s being returned to the pool.
- assert(context().pending_sockets.put(socket)); + if (http_socket.ext(**anyopaque)) |ctx| { + ctx.* = bun.cast(**anyopaque, ActiveSocket.init(dead_socket).ptr()); + } + assert(context().pending_sockets.put(socket)); log("+ Keep-Alive reuse {s}:{d}", .{ hostname, port }); return http_socket;
📜 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/http/HTTPContext.zig(1 hunks)test/js/web/fetch/fetch-preconnect.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
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/http/HTTPContext.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/http/HTTPContext.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.test.ts
test/js/web/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Web API tests under test/js/web/, separated by category
Files:
test/js/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.test.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript files with Prettier (bun run prettier)
Files:
test/js/web/fetch/fetch-preconnect.test.ts
🧠 Learnings (13)
📚 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/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.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/js/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.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/js/web/fetch/fetch-preconnect.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:
test/js/web/fetch/fetch-preconnect.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} : Import testing utilities (devTest, prodTest, devAndProductionTest, Dev, Client) from test/bake/bake-harness.ts
Applied to files:
test/js/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.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: For Node.js compatibility bugs, validate expected behavior in Node.js first, then confirm failure on Bun canary
Applied to files:
test/js/web/fetch/fetch-preconnect.test.ts
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
test/js/web/fetch/fetch-preconnect.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/web/fetch/fetch-preconnect.test.ts
🧬 Code graph analysis (1)
test/js/web/fetch/fetch-preconnect.test.ts (1)
packages/bun-types/bun.d.ts (1)
Socket(6719-6723)
🔇 Additional comments (4)
test/js/web/fetch/fetch-preconnect.test.ts (4)
3-3: Harness imports look correctUsing bunExe/bunEnv from harness aligns with our testing conventions.
9-16: Typed Promise.withResolvers improves type safetyAnnotating with Bun.Socket makes the test intent clearer and avoids any type widening.
32-52: Nice: concurrency + typed resolvers in the robustness suiteThe generic typing and concurrent describe usage are consistent and improve clarity.
7-7: Confirm describe.concurrent.todoIf is supported across CI
test/js/web/fetch/fetch-preconnect.test.ts:7 uses describe.concurrent.todoIf – verify your test runner on CI supports this API combination or switch to a supported pattern.
What does this PR do?
How did you verify your code works?