test(postgres): run binary NUMERIC sub-1e-8 test against the real server#31508
test(postgres): run binary NUMERIC sub-1e-8 test against the real server#31508robobun wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughMock-based Postgres binary numeric test is replaced with real Docker-backed integration test. Test coordinator is updated to prestart ChangesDocker-backed postgres binary numeric test
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 8:43 AM PT - Jun 10th, 2026
❌ @robobun, your commit 85db71d has 6 failures in
🧪 To try this PR locally: bunx bun-pr 31508That installs a local version of the PR into your bun-31508 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only refactor swapping a mock for the real postgres_plain container using established harness patterns.
Extended reasoning...
Overview
This PR touches two test-infrastructure files only. test/js/sql/postgres-binary-numeric.test.ts is rewritten to drop a hand-rolled net-based mock Postgres server in favor of the real postgres_plain docker-compose service via describeWithContainer / isDockerEnabled(). test/docker/warmup-ci.ts gains a one-line entry mapping js/sql/postgres-binary-numeric → postgres_plain so CI pre-starts the container.
Security risks
None. No production/runtime code is modified. The sql.unsafe(literal) call inlines hardcoded numeric string constants from the test file itself (necessary because .simple() cannot bind params), not external input. The connection URL targets a local test container.
Level of scrutiny
Low. This is test-only code following patterns already established in the repo: the describeWithContainer(..., { image: "postgres_plain" }, ...) + await container.ready + postgres://bun_sql_test@host:port/bun_sql_test shape matches test/regression/issue/21311.test.ts exactly, and the warmup-ci map entry is identical in form to the neighboring sql.test / sql-prepare-false entries. The suite is gated behind isDockerEnabled() so it skips where docker is unavailable, same as other postgres docker tests.
Other factors
The PR description documents that the new test was verified to fail before the #31211 fix and pass after, confirming it exercises the intended binary NUMERIC decoder path. The bug-hunting system found no issues. The change actually simplifies the test (removes ~80 lines of wire-protocol mocking) while increasing fidelity by testing against a real server.
There was a problem hiding this comment.
LGTM — test-only refactor following the established describeWithContainer / postgres_plain pattern.
Extended reasoning...
Overview
This PR touches two test-infrastructure files only: test/js/sql/postgres-binary-numeric.test.ts is rewritten to drop a hand-rolled mock Postgres server in favor of the real postgres_plain docker-compose service, and test/docker/warmup-ci.ts gets a one-line map entry so CI pre-warms that container for the shard. No production/runtime code is changed.
Security risks
None. The only "unsafe" construct is sql.unsafe(literal) inside .simple(), but the literals are a hardcoded array of numeric string constants in the test file itself — there is no user-controlled input. The connection URL and container setup are copied verbatim from existing tests.
Level of scrutiny
Low. This is a test refactor that follows established patterns line-for-line: isDockerEnabled() + describeWithContainer(..., { image: "postgres_plain" }, ...) with await container.ready in beforeAll and the postgres://bun_sql_test@${host}:${port}/bun_sql_test URL match test/js/sql/sql.test.ts and test/regression/issue/21311.test.ts exactly. The warmup-ci.ts addition is a mechanical one-liner identical in shape to its neighbors, and per that file's own header comment a missing entry would only make the test slower, not incorrect.
Other factors
The bug-hunting system found no issues. The describeWithContainer signature in test/harness.ts confirms the { image } option shape and the container.{host,port,ready} descriptor are used correctly. The harness already handles the no-docker case via describe.todo, so the outer isDockerEnabled() guard is redundant but harmless. The PR description documents that the new test was verified to fail pre-fix and pass post-fix, which is exactly what you want when replacing a mock with real infra.
Replace the minimal mock-Postgres server in postgres-binary-numeric.test.ts with the real postgres_plain docker-compose service, matching how sql.test.ts and sql-prepare-false.test.ts connect. A plain tagged-template query (select $1::numeric) goes through the extended protocol, so Bun requests the result column in binary and decodes it with the binary NUMERIC decoder, the path the sub-1e-8 fix touched. Each value is cross-checked against the simple-protocol (text) result, which was always correct. Add the file to the coordinator.ts prestart map so CI pre-starts postgres_plain for the shard.
robobun
force-pushed
the
farm/b82bdf6a/postgres-numeric-real-server
branch
from
0851158 to
85db71d
Compare
|
CI note on build 61752 (concluded): all hard failures are unrelated to this diff.
The postgres test added here passed on every lane; its only blemish was one retry-recovered container-startup race on alpine x64-baseline, the same class the existing postgres docker tests share. Diff is test-only and ready for review. |
1 participant
Follow-up to #31211 (requested on that PR): swap the mock-Postgres server in
postgres-binary-numeric.test.tsfor the realpostgres_plaindocker-compose service.What changed
test/js/sql/postgres-binary-numeric.test.tsdrops the hand-rollednet-based mock server (which fakedformat=1in the RowDescription to reach the binary decoder) and instead connects to the realpostgres_plainservice viadescribeWithContainer/isDockerEnabled(), the same infrasql.test.tsandsql-prepare-false.test.tsuse.test/docker/coordinator.tsgets ajs/sql/postgres-binary-numeric -> postgres_plainentry in the prestart map so CI pre-starts the container for that shard. (This was originally an entry inwarmup-ci.ts; rebased after ci: serialize docker service startup through a per-shard coordinator #32033 replaced that file with the coordinator, so the entry moved to the coordinator's equivalent map.)Why a real server hits the buggy path
#31211 fixed the binary NUMERIC decoder for sub-1e-8 values. Against a real server, a plain tagged-template query (
sqlselect ${v}::numeric``) uses the extended protocol, so Bun requests the result column in binary and decodes it with the binary NUMERIC decoder, no mock needed to forceformat=1. Each value is also cross-checked against the `.simple()` (text) result, which was always correct and pins the expected canonical rendering.Verification
Confirmed against a real Postgres server that the test fails before the fix and passes after. Running the same matrix through the test runner:
1e-9decodes as0.000010000), while the text-protocol assertion passes and the 7 boundary/always-correct cases pass, i.e. the test exercises exactly the decoder path the fix touched.The docker-gated suite runs on Linux x64 CI (where docker is available); it skips elsewhere, same as the other postgres docker tests.