Conversation
…ackend instead of a docker container
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
No bugs found. This is a well-reasoned test refactor that follows the established postgres-infinity-date.test.ts wire-mock pattern, and the fail-before property is demonstrably preserved. Deferring only because swapping a real-Postgres container test for a scripted backend is a coverage trade-off (the mock encodes the GUC-precedence assumption rather than exercising it) — that judgment call is worth a maintainer's quick sign-off rather than a bot approval.
What was reviewed
- Verified all
wire-frames.tsimports exist and the P/B response sequence matches the sibling test's pattern. - Checked the mock buffers partial TCP chunks correctly before parsing the StartupMessage and hands the remainder to
pgReadFrontendMessages. - Confirmed
parseStartupParamsis a byte-identical hoist of the previously-inlined logic. - Confirmed the mock's DateStyle branch keeps the test failing on a build without the StartupMessage fix (fails for the right reason).
Extended reasoning...
Overview
Test-only change to test/js/sql/postgres-datestyle.test.ts. The second test previously ran inside describeWithContainer("postgres", { image: "postgres_plain" }, ...), which cost ~34s of container cold-start on macOS CI and was .todo'd entirely when docker/env-var wasn't available. It is replaced with a scripted v3 backend built from the existing test/js/sql/wire-frames.ts helpers — the same pattern postgres-infinity-date.test.ts already uses for its extended-protocol mock. The first test is unchanged apart from hoisting the StartupMessage key/value parser into a shared parseStartupParams helper. No production code is touched.
Security risks
None. This is a test file talking to an in-process net.createServer on 127.0.0.1:0. No external network, no auth, no crypto, no file writes outside the test's own socket.
Level of scrutiny
Moderate. It's test-only, but REVIEW.md is explicit that weakening or deleting an existing test needs a stated reason and equivalent coverage. The PR body argues equivalence carefully: the container test's "ALTER DATABASE default is overridden by StartupMessage param" step is Postgres's documented GUC precedence (PGC_S_CLIENT > PGC_S_DATABASE), not Bun behaviour, so the mock encodes that precedence and instead exercises what Bun controls — that the StartupMessage carries DateStyle=ISO and that Bun's decoder handles the resulting ISO text on both simple (Q) and extended (P/B) paths. Crucially, the mock branches on the actual StartupMessage bytes, so a build without the StartupMessage.rs fix falls through to SQL, DMY output and the test fails with the month/day-swap and Invalid-Date diff shown in the PR body. That preserves the "fails for the right reason" property REVIEW.md requires.
Why defer rather than approve
The refactor is correct and idiomatic, but it does trade real-server end-to-end coverage for a mock that assumes Postgres honours the startup parameter. That assumption is well-documented and unlikely to break, and the first (unchanged) test already asserts the load-bearing wire bytes directly — so I think the trade is sound. Still, "replace container integration test with a wire mock" is exactly the kind of coverage decision a maintainer should ratify rather than a bot. There's nothing to fix; this just wants a human 👍.
Other factors
- All imported
wire-frames.tshelpers exist and are used with the same message sequencing aspostgres-infinity-date.test.ts(ParseComplete + ParameterDescription + RowDescription + RFQ onP; BindComplete + DataRow + CommandComplete + RFQ onB). - The socket
datahandler correctly accumulates partial chunks, length-prefix-frames the StartupMessage, and only then feedspgReadFrontendMessages— no split-read hazard. - Server is closed in
finally;connectionTimeout: 5bounds any hang. - The new
seenDateStyle: "ISO, MDY"assertion is stricter than the first test's/^ISO\b/regex, which is fine (it pins the exact value Bun sends).
|
Updated 10:14 AM PT - Jul 25th, 2026
❌ @robobun, your commit 3ed2e0f has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35738That installs a local version of the PR into your bun-35738 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
CI is green apart from the Self-review raised no concerns. Measured wall time on
The file also runs third in its shard now instead of being reordered to the end, because it no longer matches any |
What
test/js/sql/postgres-datestyle.test.tswas taking ~34s ondarwin 14 aarch64in CI (build #80083). All of that was thepostgres_plaincontainer cold-start insidedescribeWithContainer; the two tests themselves run in about a second.This replaces the container-backed end-to-end test with a scripted v3 backend (the same
wire-frames.tshelperspostgres-infinity-date.test.tsalready uses), so the file no longer needs docker at all.Why this is equivalent coverage
The container test did three things:
ALTER DATABASE bun_sql_test SET datestyle = 'SQL, DMY', then opened a new connection and checkedcurrent_setting('datestyle')was ISO. That proves a StartupMessageDateStyleparameter outranks anALTER DATABASEdefault, which is Postgres's documented GUC precedence (PGC_S_CLIENT>PGC_S_DATABASE); it is not Bun behaviour.'2026-04-03'::dateand'2026-07-22'::datevia.simple()and checked the decodedDateobjects.'2026-04-03'::datevia the extended protocol and checked the decodedDate.The scripted backend covers the same ground from Bun's side:
DateStylethe client sent. IfDateStyle=ISOis present it emits ISO date text; if not it emits theSQL, DMYtext (03/04/2026,22/07/2026) that anALTER DATABASE ... SET datestyle = 'SQL, DMY'default would produce.Q) with three columns (current_setting, two dates) and the extended query (P/B) with one date column..toISOString()values, plus the sessionDateStylethe client observed.Because the mock reacts to what Bun actually sends, the test still fails-before the
StartupMessage.rsfix: on a build without theDateStylestartup parameter the mock falls through toSQL, DMY, and the decoder produces2026-03-04(month/day swapped) andInvalid Date, which the singletoEqualshows as a clean diff.The first test (StartupMessage bytes contain
DateStyle=ISO) is unchanged apart from hoisting the key/value-pair parser into a sharedparseStartupParamshelper.Side benefit
Without docker and without a
BUN_TEST_SERVICE_postgres_plainenv override, the old end-to-end test wasdescribe.todo'd (0 coverage). The wire-mock version always runs.Timing
darwin 14 aarch64(build #80083): 34s wall time, entirely container cold-start. After: no container, so that cost is gone; the test body is <1s.bun bd test(debug+ASAN, warm Postgres via env var): before ~4–8s, after ~4–8s. Unchanged here because the env-var path never paid the cold-start cost; the whole file is dominated by debug-runner startup either way.bun bd testwith no Postgres / docker: before ran 1 test (container block todo'd), after runs 2 tests in the same ~4s.Verification
fail-before on a build without the StartupMessage fix
[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
28 concerns were raised and did not survive verification.