Repository navigation
fix(Bun.SQL) fix timers test and disable describeWithContainer on macos - #22382
Conversation
Previously, when PostgreSQL returned TIME or TIMETZ columns in binary format, Bun.sql would not properly convert them from their binary representation (microseconds since midnight) to readable time strings. This resulted in garbled output with null bytes instead of proper time values. This fix: - Adds proper binary format handling for TIME (8 bytes) and TIMETZ (12 bytes) types - Uses C++ helper functions following WebKit's pattern with snprintf for formatting - Converts microseconds to HH:MM:SS.ffffff format with proper formatting - Handles timezone offsets correctly for TIMETZ type (PostgreSQL uses negative for positive UTC) - Trims trailing zeros from fractional seconds for cleaner output - Adds comprehensive tests for both types and their array variants The C++ formatting functions follow WebKit's established pattern of using snprintf for date/time formatting as seen in their toISOString implementation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 4:53 PM PT - Sep 3rd, 2025
❌ @cirospaciari, your commit a6c628c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22382That installs a local version of the PR into your bun-22382 --bun |
WalkthroughAdds macOS to container-skip logic in test harness. Removes Postgres TIME/TIMETZ tests from test/js/bun/sql/postgres-time.test.ts and introduces an equivalent Time/TimeZ suite under test/js/sql/sql.test.ts that runs only with Docker-based Postgres. Changes
Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/harness.ts (2)
909-912: Use isMacOS and make the skip reason accuratePrefer the existing isMacOS helper and adjust the message to reflect an intentional disable (not “unsupported”).
- if ((archs && !archs?.includes(arch)) || platform === "win32" || platform === "darwin") { - test.skip(`docker image is not supported on ${platform}/${arch}, skipped: ${image}`, () => {}); + if ((archs && !archs?.includes(arch)) || platform === "win32" || isMacOS) { + test.skip(`container tests disabled on ${platform}/${arch}, skipped: ${image}`, () => {}); return false; }
909-912: Optional: allow local macOS opt-in while keeping CI disabledGate the darwin block behind an env override so contributors with Docker Desktop/Podman can run these locally.
- if ((archs && !archs?.includes(arch)) || platform === "win32" || isMacOS) { + const allowDarwinContainers = process.env.BUN_TEST_CONTAINER_ON_DARWIN === "1"; + if ((archs && !archs?.includes(arch)) || platform === "win32" || (isMacOS && !allowDarwinContainers)) { test.skip(`container tests disabled on ${platform}/${arch}, skipped: ${image}`, () => {}); return false; }test/js/sql/sql.test.ts (2)
318-323: Make table cleanup resilient (run in finally and use IF EXISTS).If assertions throw before cleanup, tables can be left behind. Move DROP TABLE into finally with IF EXISTS.
Apply this diff in each test’s cleanup:
- // Clean up - await db`DROP TABLE bun_time_test`; - } finally { - await db.end(); - } + } finally { + try { await db`DROP TABLE IF EXISTS bun_time_test`; } catch {} + await db.end(); + }Repeat similarly for bun_time_array_test and bun_time_json_test.
Also applies to: 386-391, 429-434
256-256: Optional: useawait usingfor connection lifecycle.
await using db = postgres(options)auto-closes on scope exit and reduces boilerplate try/finally.Example:
- const db = postgres(options); + await using db = postgres(options);Also applies to: 326-326, 394-394
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
test/harness.ts(1 hunks)test/js/bun/sql/postgres-time.test.ts(0 hunks)test/js/sql/sql.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- test/js/bun/sql/postgres-time.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/harness.tstest/js/sql/sql.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/harness.tstest/js/sql/sql.test.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Run Prettier to format JS/TS files (bun run prettier)
Files:
test/harness.tstest/js/sql/sql.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/sql/sql.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Tests must be named with the suffix .test.ts or .test.tsx and live under the test/ directory
In tests, always use port: 0 and never hardcode ports or use custom random-port utilities
Prefer snapshot tests using normalizeBunSnapshot(...) over direct string equality on stdout/stderr
Do not write tests that assert absence of 'panic', 'uncaught exception', or similar strings in output
Avoid shelling out to tools like find or grep in tests; use Bun.Glob and built-in utilities
Files:
test/js/sql/sql.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/sql/sql.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sql.test.ts
🧠 Learnings (1)
📚 Learning: 2025-09-03T17:10:13.460Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.460Z
Learning: Applies to test/**/*.test.ts : Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Applied to files:
test/js/sql/sql.test.ts
🧬 Code graph analysis (1)
test/harness.ts (3)
scripts/runner.node.mjs (2)
arch(331-331)platform(1979-1979)scripts/utils.mjs (2)
arch(1618-1618)arch(1677-1677)test/js/node/process/process.test.js (2)
arch(142-142)platform(141-141)
⏰ 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
3 participants
What does this PR do?
Actually run the Timer/TimerZ tests in CI and disable describeWithContainer in macos
How did you verify your code works?
CI