Skip to content

test(sql): centralize fault-injection wire frames; convert decode mocks to real servers#32467

Open
alii wants to merge 9 commits into
mainfrom
ali/sql-wire-frames-and-mock-sweep
Open

test(sql): centralize fault-injection wire frames; convert decode mocks to real servers#32467
alii wants to merge 9 commits into
mainfrom
ali/sql-wire-frames-and-mock-sweep

Conversation

@alii

@alii alii commented Jun 17, 2026

Copy link
Copy Markdown
Member

Three pieces, all in test/js/sql/. Net −1,064 lines.

1. wire-frames.ts — single source of truth for fault-injection protocol bytes

New shared module with one builder per Postgres/MySQL frame type the fault-injection tests emit (pgAuthenticationOk, pgReadyForQuery, pgErrorResponse, pgRowDescription, pgDataRow, pgCommandComplete, pgRaw, mysqlHandshakeV10, mysqlOkPacket, mysqlAuthSwitchRequest, mysqlColumnDefinition, mysqlStmtPrepareOk, mysqlLenencInt/Str, mysqlRawPacket, mysqlReadPackets), plus the listeningServer/closedPort/neverAnsweringServer/pgMinimalReadyServer helpers every fault-injection file was reimplementing. Each builder carries a one-line spec citation (PostgreSQL FE/BE protocol §55.x / MySQL page_protocol_*).

wire-frames.test.ts is the spec-compliance self-test: it spins a net.createServer emitting pgAuthenticationOk() + pgReadyForQuery() (and the MySQL equivalent), does await new Bun.SQL({url}).connect(), and asserts it resolves — so if a builder ever drifts from what Bun's own parser accepts, this file goes red before the fault-injection tests start silently misbehaving.

2. Mark + refactor the fault-injection tests

These genuinely need a misbehaving server (refused/dropped/malformed frames) and stay as net.createServer mocks, but now (a) carry a "DO NOT COPY THIS PATTERN — anything a real server can produce belongs in describeWithContainer" header, and (b) import every frame from ./wire-frames instead of inlining Buffer.alloc/writeInt32BE sequences:

sql-connect-error-reporting, sql-close-pending-connection, postgres-tls-ctx-leak, postgres-binary-array-bounds, sql-mysql-auth-short-nonce, sql-mysql-tls-plaintext-injection, sql-mysql-columns-realloc-oom, sql-mysql-clean-reentry, tls-sql.test.ts (the two SSLRequest-spoofing blocks), sql-onconnect-onclose-throw (closedPort/neverAnsweringServer), and the never-responds blocks in sql.test.ts / sql-mysql.test.ts.

After this, grep writeInt32BE test/js/sql/*.test.ts finds exactly one hit — postgres-binary-array-bounds.test.ts:27, building the deliberately-hostile column payload body (the frames around it come from ./wire-frames).

3. Convert decode/roundtrip mocks to describeWithContainer

These were testing decode behavior a real server produces — they shouldn't have been mocks. Each now wraps in describeWithContainer("postgres"|"mysql", { image: ..._plain }) and issues real SQL that produces the same data the mock was hand-encoding, with the original assertions preserved:

postgres-binary-numeric (SELECT '...'::numeric), postgres-multi-statement-fields (.simple() multi-statement), sql-mysql-mediumint, sql-mysql-raw-length-prefix, sql-mysql-cached-error, sql-mysql-query-string-leak, sql-mysql-datetime-roundtrip (mock half + 197-line sql-mysql-datetime-text-mock-fixture.ts deleted — the real-server sibling already covered it), the DECIMAL block in sql-mysql.test.ts, and three inline mocks in sql.test.ts (text-array / multi-result-set / short-DataRow regressions, the latter two extracted to *.fixture.ts since they assert process exit).

Verification

  • 108 pass / 0 fail across the 18 touched test files (postgres_plain + mysql_plain containers); sql.test.ts and sql-mysql.test.ts load clean.
  • wire-frames.test.ts self-test green for both adapters.
  • No src/ changes.

@robobun

robobun commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator
Updated 4:16 PM PT - Jun 17th, 2026

@robobun, your commit 4d4aeba is building: #63233

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Introduces test/js/sql/wire-frames.ts as a shared byte-accurate SQL wire-protocol frame builder and TCP server helper library. Migrates all fault-injection tests to consume the shared helpers instead of defining inline implementations, and rewrites several mock-server-only tests to run against real Docker containers for integration coverage.

Changes

Wire-frames library and test migration

Layer / File(s) Summary
wire-frames.ts: TCP helpers, Postgres and MySQL frame builders
test/js/sql/wire-frames.ts
Adds listeningServer, closedPort, neverAnsweringServer; all Postgres FE/BE frame builders (pgSSLRequest/Response, pgAuthenticationOk, pgReadyForQuery, pgErrorResponse, pgRaw, pgCommandComplete, pgRowDescription, pgDataRow, pgMinimalReadyServer); MySQL capability constants plus mysqlRawPacket, mysqlHandshakeV10, mysqlOkPacket, mysqlAuthSwitchRequest, mysqlLenencInt/Str, mysqlColumnDefinition, mysqlStmtPrepareOk, mysqlReadPackets.
wire-frames.test.ts: encoder unit tests and connection acceptance checks
test/js/sql/wire-frames.test.ts
Adds byte-level unit tests for mysqlLenencInt and pgErrorResponse, plus integration connection-acceptance tests verifying Bun's Postgres and MySQL parsers accept frames produced by the new builders.
PostgreSQL binary payload tests: array-bounds and numeric using wire-frames
test/js/sql/postgres-binary-array-bounds.test.ts, test/js/sql/postgres-binary-numeric.test.ts
postgres-binary-array-bounds replaces inline TCP server with listeningServer and imported frame constructors. postgres-binary-numeric migrates from a mock server to a real postgres_plain container and runs NUMERIC literal round-trip tests via describeWithContainer.
PostgreSQL multi-statement and TLS fault-injection tests migrated to wire-frames
test/js/sql/postgres-multi-statement-fields.test.ts, test/js/sql/postgres-tls-ctx-leak.test.ts, test/js/sql/tls-sql.test.ts
postgres-multi-statement-fields uses listeningServer and wire-frames builders for async message injection between result sets. postgres-tls-ctx-leak uses pgSSLResponse/pgAuthenticationOk/pgReadyForQuery via listeningServer. TLS tests replace hardcoded SSLRequest byte arrays with generated pgSSLRequest() and pgSSLResponse() frames.
PostgreSQL startup and error handling tests using wire-frames
test/js/sql/sql-connect-error-reporting.test.ts, test/js/sql/sql.test.ts
sql-connect-error-reporting replaces inline buffer construction with pgAuthenticationOk(), pgReadyForQuery(), and pgErrorResponse() helpers. sql.test.ts connection-timeout tests adopt neverAnsweringServer and assert ERR_POSTGRES_CONNECTION_TIMEOUT.
PostgreSQL fault-injection fixtures extracted to subprocess scripts
test/js/sql/sql-postgres-json-array-bool-literal.fixture.ts, test/js/sql/sql-postgres-short-data-row.fixture.ts, test/js/sql/sql.test.ts
Extracts two inline fault-injection scenarios into standalone fixture scripts; sql-postgres-short-data-row sends a 62-column RowDescription with zero or 62 DataRow values. Adds a regression test for zero-column RowDescription followed by a wider result set.
MySQL authentication and handshake tests using wire-frames
test/js/sql/sql-mysql-auth-short-nonce.test.ts, test/js/sql/sql-mysql-tls-plaintext-injection.test.ts
sql-mysql-auth-short-nonce uses listeningServer, mysqlHandshakeV10, and mysqlAuthSwitchRequest instead of custom packet helpers. sql-mysql-tls-plaintext-injection uses wire-frame builders for handshake and OK packet construction.
MySQL fixture migration to wire-frames and reachable server setup
test/js/sql/sql-mysql-clean-reentry.test.ts, test/js/sql/sql-close-pending-connection.test.ts, test/js/sql/sql-onconnect-onclose-throw.test.ts
sql-mysql-clean-reentry migrates fixture from CommonJS .js to ESM .ts with wire-frames import. sql-close-pending-connection and sql-onconnect-onclose-throw replace inline net-based servers with neverAnsweringServer and closedPort helpers; consolidates duplicated adapter tests into a single test.concurrent loop.
MySQL binary protocol and OOM regression tests using wire-frames
test/js/sql/sql-mysql-columns-realloc-oom.test.ts
Refactors mock MySQL server to use listeningServer and wire-frame builders; introduces hugeResultSetHeader via mysqlRawPacket for deliberately malformed field-count encoding; runs regression in a Bun.spawn subprocess with JSON output validation.
MySQL integration tests rewritten against real containers
test/js/sql/sql-mysql-cached-error.test.ts, test/js/sql/sql-mysql-mediumint.test.ts, test/js/sql/sql-mysql-query-string-leak.test.ts, test/js/sql/sql-mysql-raw-length-prefix.test.ts, test/js/sql/sql-mysql-datetime-roundtrip.test.ts
Rewrites cache-failure, MEDIUMINT decoding, query-string leak, length-prefix stripping, and datetime tests to run against real mysql_plain containers via describeWithContainer, validating prepared-statement caching, binary column alignment, query GC behavior, .raw() prefix stripping, and NULL-marker collision edge cases.
MySQL timeout and NEWDECIMAL mock suite cleanup
test/js/sql/sql-mysql.test.ts
Refactors timeout tests to use listeningServer instead of raw net server; removes the offline NEWDECIMAL mock-server test suite.
Supporting MySQL timezone fixture expansion
test/js/sql/sql-mysql-datetime-tz-fixture.ts
Expands validation to test both zero-dates and invalid non-zero dates using ALLOW_INVALID_DATES SQL mode; asserts both rows decode as Invalid Date across binary and text protocols.

Possibly related PRs

  • oven-sh/bun#31211: The postgres-binary-numeric.test.ts refactor to use a real Postgres container directly validates the NUMERIC decoding fix for weight <= -3 leading-zero handling introduced in that PR.
  • oven-sh/bun#31606: The postgres-multi-statement-fields.test.ts updates add coverage for interleaving async NOTIFY/NotificationResponse messages between result sets, validating the backend message parsing behavior from that PR.
  • oven-sh/bun#32041: The sql-onconnect-onclose-throw.test.ts refactor to use neverAnsweringServer and closedPort from wire-frames directly validates the pool hook-throwing fix introduced in that PR.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the PR's main contribution: centralizing fault-injection wire frames and converting decode mocks to real servers.
Description check ✅ Passed The description comprehensively covers the three main pieces (wire-frames.ts module, refactored fault-injection tests, converted mock tests), verification results, and includes a full PR objectives summary with commit history and CI feedback.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. test(postgres): run binary NUMERIC sub-1e-8 test against the real server #31508 - Also converts postgres-binary-numeric.test.ts from a hand-rolled mock server to a real database container via describeWithContainer, which is a subset of this PR's broader mock-to-real-server conversion

🤖 Generated with Claude Code

@alii

alii commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

@robobun adopt

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
test/js/sql/sql-onconnect-onclose-throw.test.ts (1)

226-240: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider using test.concurrent.each for consistency.

Similar to the refused-connection tests above, this loop could be refactored to use test.concurrent.each for cleaner parameterized tests. This would make the test structure more consistent throughout the file.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/sql/sql-onconnect-onclose-throw.test.ts` around lines 226 - 240,
Replace the for loop that iterates over adapter and closedCode tuples with a
test.concurrent.each() call to create parameterized tests. Pass the array of
adapter/code pairs (including "postgres"/"ERR_POSTGRES_CONNECTION_CLOSED" and
"mysql"/"ERR_MYSQL_CONNECTION_CLOSED") as the parameter to
test.concurrent.each(), and update the test callback to receive the adapter and
closedCode parameters directly. This will make the test structure consistent
with similar parameterized tests in the file and eliminate the manual loop
iteration, while keeping the same test logic inside the callback that uses
forcedCloseFixture(adapter) and verifies the expected stdout and exitCode.
test/js/sql/sql-mysql.test.ts (1)

1165-1189: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Move the mock timeout matrix out of the Docker suite.

This block uses only listeningServer and does not depend on container, but it sits under isDockerEnabled() and runs once per image. Move it to a standalone non-Docker MySQL fault-injection test, or otherwise outside the container loop, so it runs once and is not skipped on non-Docker runs.

Based on learnings, “keep Docker-gated MySQL container tests and non-Docker MySQL tests in separate files” and standalone test/js/sql/sql-mysql-*.test.ts files are intentional for mock/non-Docker tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/sql/sql-mysql.test.ts` around lines 1165 - 1189, The "timeouts"
describe block with its test.each for connection timeout keys should be moved
out of the Docker-gated container loop since it only depends on listeningServer
and not on the container configuration. Extract this timeout test block and
place it in a separate standalone test file following the pattern of
test/js/sql/sql-mysql-*.test.ts for non-Docker MySQL tests, or move it outside
the isDockerEnabled() conditional block, so that it runs once in a non-Docker
context instead of being repeated for each Docker image.

Source: Learnings

test/js/sql/postgres-multi-statement-fields.test.ts (1)

81-119: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the fault-injection server cleanup deterministic.

This mock writes and ends sockets on an error-path test, but the socket has no expected-error handler and server.close() is not awaited. Add the handler and await close completion so the test does not leak a listener or fail on an unhandled reset. As per coding guidelines, “Tests must be hermetic and leave nothing behind.”

Proposed fix
     const { port, server } = await listeningServer(socket => {
+      socket.on("error", () => {});
       let startup = true;
       socket.on("data", data => {
         if (startup) {
@@
     try {
       const result = await sql`select 1 as x; select 2 as y`.simple();
       expect(result).toEqual([[{ x: "1" }], [{ y: "2" }]]);
     } finally {
-      await sql.close();
-      server.close();
+      await sql.close().catch(() => {});
+      await new Promise<void>(resolve => server.close(() => resolve()));
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/sql/postgres-multi-statement-fields.test.ts` around lines 81 - 119,
The mock server's socket has no error handler and server.close() is not awaited,
which can cause test flakiness and resource leaks when the socket ends
unexpectedly. Within the socket.on("data") handler or at the socket setup, add a
socket.on("error") handler to gracefully handle expected errors like connection
resets so they don't cause unhandled rejections, and change server.close() to
await server.close() in the finally block to ensure deterministic cleanup and
prevent listener leaks.

Source: Coding guidelines

test/js/sql/sql.test.ts (1)

2981-2983: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the 1s test timeout.

Line 2982 adds a second timing bound even though the query is already bounded by [key]: 0.2; this can fail under slow debug/ASAN CI for scheduler latency rather than behavior. Let the default test timeout handle true hangs. As per coding guidelines, Bun already has test timeouts and tests should avoid custom timing bounds unless they are load-bearing.

Proposed fix
       },
-      {
-        timeout: 1000,
-      },
     );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/sql/sql.test.ts` around lines 2981 - 2983, Remove the timeout
configuration object containing `timeout: 1000` from the test. The query is
already bounded by a `[key]: 0.2` timing constraint, so the additional 1-second
test timeout creates an unnecessary secondary timing bound that can cause
failures in slow CI environments with debug or ASAN builds. Delete the entire
configuration object with the timeout property and let the default Bun test
timeout handle any actual test hangs instead.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/sql/sql-mysql-columns-realloc-oom.test.ts`:
- Around line 42-45: The test does not verify that the malformed execute path
was actually triggered on the server side. Add a tracking variable (flag) that
is set to true when the mock server receives the COM_STMT_EXECUTE command, then
assert that this flag is true after checking the subprocess exit code in the
test. This ensures that the test passes only when the huge result-set header is
actually sent by the mock, preventing false positives where any clean JSON error
would cause the test to pass. Apply the same pattern to the other test cases
mentioned at lines 65-67 and 107-113.

In `@test/js/sql/sql-onconnect-onclose-throw.test.ts`:
- Around line 126-139: Replace the for loop containing test.concurrent with
test.concurrent.each for a more idiomatic approach. Move the array of test
parameters directly into the test.concurrent.each call, use %s placeholder in
the test name string instead of template literal for the adapter name, and
update the async callback function to receive the array elements (adapter,
refusedCode, url) as direct parameters instead of destructuring from the for
loop. The test body logic remains the same but now uses these parameters from
the function arguments.

In `@test/js/sql/sql.test.ts`:
- Around line 12523-12531: The CREATE TABLE statement at the beginning of this
test fixture creates a permanent table that may not be cleaned up if the process
crashes before reaching the finally block. Replace the CREATE TABLE statement
with CREATE TEMPORARY TABLE to ensure Postgres automatically drops the table
when the fixture connection closes, making the test hermetic and preventing
orphaned tables in the shared database.

---

Outside diff comments:
In `@test/js/sql/postgres-multi-statement-fields.test.ts`:
- Around line 81-119: The mock server's socket has no error handler and
server.close() is not awaited, which can cause test flakiness and resource leaks
when the socket ends unexpectedly. Within the socket.on("data") handler or at
the socket setup, add a socket.on("error") handler to gracefully handle expected
errors like connection resets so they don't cause unhandled rejections, and
change server.close() to await server.close() in the finally block to ensure
deterministic cleanup and prevent listener leaks.

In `@test/js/sql/sql-mysql.test.ts`:
- Around line 1165-1189: The "timeouts" describe block with its test.each for
connection timeout keys should be moved out of the Docker-gated container loop
since it only depends on listeningServer and not on the container configuration.
Extract this timeout test block and place it in a separate standalone test file
following the pattern of test/js/sql/sql-mysql-*.test.ts for non-Docker MySQL
tests, or move it outside the isDockerEnabled() conditional block, so that it
runs once in a non-Docker context instead of being repeated for each Docker
image.

In `@test/js/sql/sql-onconnect-onclose-throw.test.ts`:
- Around line 226-240: Replace the for loop that iterates over adapter and
closedCode tuples with a test.concurrent.each() call to create parameterized
tests. Pass the array of adapter/code pairs (including
"postgres"/"ERR_POSTGRES_CONNECTION_CLOSED" and
"mysql"/"ERR_MYSQL_CONNECTION_CLOSED") as the parameter to
test.concurrent.each(), and update the test callback to receive the adapter and
closedCode parameters directly. This will make the test structure consistent
with similar parameterized tests in the file and eliminate the manual loop
iteration, while keeping the same test logic inside the callback that uses
forcedCloseFixture(adapter) and verifies the expected stdout and exitCode.

In `@test/js/sql/sql.test.ts`:
- Around line 2981-2983: Remove the timeout configuration object containing
`timeout: 1000` from the test. The query is already bounded by a `[key]: 0.2`
timing constraint, so the additional 1-second test timeout creates an
unnecessary secondary timing bound that can cause failures in slow CI
environments with debug or ASAN builds. Delete the entire configuration object
with the timeout property and let the default Bun test timeout handle any actual
test hangs instead.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: cf86b4a3-4f08-491c-8ae2-48825a96401e

📥 Commits

Reviewing files that changed from the base of the PR and between dcc34a8 and feee4c2.

📒 Files selected for processing (24)
  • test/js/sql/postgres-binary-array-bounds.test.ts
  • test/js/sql/postgres-binary-numeric.test.ts
  • test/js/sql/postgres-multi-statement-fields.test.ts
  • test/js/sql/postgres-tls-ctx-leak.test.ts
  • test/js/sql/sql-close-pending-connection.test.ts
  • test/js/sql/sql-connect-error-reporting.test.ts
  • test/js/sql/sql-mysql-auth-short-nonce.test.ts
  • test/js/sql/sql-mysql-cached-error.test.ts
  • test/js/sql/sql-mysql-clean-reentry.test.ts
  • test/js/sql/sql-mysql-columns-realloc-oom.test.ts
  • test/js/sql/sql-mysql-datetime-roundtrip.test.ts
  • test/js/sql/sql-mysql-datetime-text-mock-fixture.ts
  • test/js/sql/sql-mysql-mediumint.test.ts
  • test/js/sql/sql-mysql-query-string-leak.test.ts
  • test/js/sql/sql-mysql-raw-length-prefix.test.ts
  • test/js/sql/sql-mysql-tls-plaintext-injection.test.ts
  • test/js/sql/sql-mysql.test.ts
  • test/js/sql/sql-onconnect-onclose-throw.test.ts
  • test/js/sql/sql-postgres-json-array-bool-literal.fixture.ts
  • test/js/sql/sql-postgres-short-data-row.fixture.ts
  • test/js/sql/sql.test.ts
  • test/js/sql/tls-sql.test.ts
  • test/js/sql/wire-frames.test.ts
  • test/js/sql/wire-frames.ts
💤 Files with no reviewable changes (2)
  • test/js/sql/sql-mysql-datetime-roundtrip.test.ts
  • test/js/sql/sql-mysql-datetime-text-mock-fixture.ts

Comment thread test/js/sql/sql-mysql-columns-realloc-oom.test.ts
Comment thread test/js/sql/sql-onconnect-onclose-throw.test.ts Outdated
Comment thread test/js/sql/sql.test.ts Outdated
Comment thread test/js/sql/sql-mysql-datetime-roundtrip.test.ts
Comment thread test/js/sql/sql.test.ts
@robobun

robobun commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

✅ Adopted. All review feedback addressed (10 threads resolved); touched test/js/sql/ files pass on every CI lane that ran tests.

CI status: build 63216 and the re-roll build 63233 are red only on unrelated infrastructure:

  • All Windows test lanes canceled on agent provisioning ("Failed to create agent")
  • One macOS lane failed on test/js/valkey/ due to Docker Hub 429 Too Many Requests pulling redis:8-alpine

260/261 non-Windows jobs passed in build 63216, including every test/js/sql/ file this PR touches. Ready for maintainer review/merge.

Review-feedback changes (82abf88..4458e9a)
  • sql-mysql-columns-realloc-oom: assert mock reached COM_STMT_EXECUTE
  • sql-mysql-cached-error: assert Com_stmt_prepare did not increment across the cache-hit query (+ non-zero precondition)
  • sql-mysql-raw-length-prefix: assert CONNECTION_ID() unchanged across oversized COM_QUERY rollback
  • zero-column fixture + sql-mysql-mediumint + sql-mysql-raw-length-prefix: CREATE TEMPORARY TABLE
  • sql-mysql-datetime-tz-fixture: restore 2024-02-31 / ALLOW_INVALID_DATES coverage
  • postgres-multi-statement-fields: socket error handler + awaited server.close()
  • sql-onconnect-onclose-throw: allocate closedPort() inside the fixture subprocess
  • converted mysql_plain tests gated behind isDockerEnabled() (matches sql-mysql-bind-oob pattern)

- sql-mysql-columns-realloc-oom: assert the mock actually reached
  COM_STMT_EXECUTE so a JSON error before execute cannot satisfy the
  stdout check
- sql.test.ts zero-column fixture: use CREATE TEMPORARY TABLE so a
  crashed/killed child does not leave a permanent table in the shared
  container
- sql-mysql-datetime-tz-fixture: restore coverage for a non-zero day
  past its month length (2024-02-31 under ALLOW_INVALID_DATES), which
  the deleted text-protocol mock previously exercised
- postgres-multi-statement-fields: add socket error handler and await
  server.close() in the fault-injection block for deterministic
  teardown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
test/js/sql/sql.test.ts (1)

12549-12560: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Don’t make SQL fixture subprocesses fail on benign stderr noise.

These subprocess fixtures already assert stable stdout markers and exit code. The filteredStderr checks can still flake on ASAN/debug lanes; only surface stderr when the child actually fails.

Based on learnings, SQL fixture subprocess tests in test/js/sql/** should assert stable stdout and exit code while treating stderr-only ASAN/debug noise as acceptable.

Suggested cleanup pattern
-      const filteredStderr = stderr
-        .split(/\r?\n/)
-        .filter(l => l && !l.startsWith("WARNING: ASAN interferes"))
-        .join("\n");
-
       // The second result set must expose all three named columns with their
       // values; reusing the zero-property structure from the first result set
       // would yield a row object with no own properties.
       expect(stdout).toContain('SECOND_RESULT_SET [{"a":"1","b":"2","c":"3"}]');
       expect(stdout).toContain("SECOND_RESULT_SET_KEYS a,b,c");
       expect(stdout).toContain("FIXTURE_DONE");
-      expect(filteredStderr).toBe("");
+      if (exitCode !== 0) {
+        expect(stderr).toBe("");
+      }
       expect(exitCode).toBe(0);

Apply the same pattern to the other two fixture subprocess blocks.

Also applies to: 12591-12600, 12744-12756

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/js/sql/sql.test.ts` around lines 12549 - 12560, The test is asserting
that filteredStderr must be empty which causes flakiness on ASAN/debug lanes due
to benign noise. Remove the expect(filteredStderr).toBe("") assertion since the
test already validates stable stdout markers and exit code for the fixture
subprocess. Only surface stderr when the child process actually fails by
checking the exit code. Apply this same pattern to remove stderr-only assertions
in the other two fixture subprocess test blocks at lines 12591-12600 and
12744-12756.

Source: Learnings

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@test/js/sql/sql.test.ts`:
- Around line 12549-12560: The test is asserting that filteredStderr must be
empty which causes flakiness on ASAN/debug lanes due to benign noise. Remove the
expect(filteredStderr).toBe("") assertion since the test already validates
stable stdout markers and exit code for the fixture subprocess. Only surface
stderr when the child process actually fails by checking the exit code. Apply
this same pattern to remove stderr-only assertions in the other two fixture
subprocess test blocks at lines 12591-12600 and 12744-12756.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6002f792-c4a4-4b60-a11e-a8f23f38c71e

📥 Commits

Reviewing files that changed from the base of the PR and between feee4c2 and 82abf88.

📒 Files selected for processing (4)
  • test/js/sql/postgres-multi-statement-fields.test.ts
  • test/js/sql/sql-mysql-columns-realloc-oom.test.ts
  • test/js/sql/sql-mysql-datetime-tz-fixture.ts
  • test/js/sql/sql.test.ts

Comment thread test/js/sql/sql-mysql-mediumint.test.ts Outdated
Comment thread test/js/sql/sql-mysql-cached-error.test.ts Outdated
…nverted MySQL tests

- sql-mysql-cached-error: bracket the cache-hit query with
  SHOW SESSION STATUS LIKE 'Com_stmt_prepare' and assert the counter
  did not increment, restoring the prepareCount===2 precondition the
  original mock asserted. Without it, a fresh prepare would return an
  identical error for identical SQL and silently satisfy every
  assertion without exercising the cached-slice path.
- sql-mysql-mediumint, sql-mysql-raw-length-prefix: CREATE TEMPORARY
  TABLE so a hung/SIGKILLed test does not leak tables into the shared
  mysql_plain container (same class as the Postgres fixture in
  82abf88; these four MySQL sites were missed).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/sql/sql-mysql-cached-error.test.ts`:
- Around line 48-50: The test currently only verifies that preparesAfter does
not exceed preparesBefore, which can pass vacuously if prepared statements were
never used at all. Add an assertion immediately after capturing preparesBefore
to verify that it is greater than zero, ensuring that prepared statements were
actually being used before the cache-hit check. This ensures the test fails for
the right reason and proves the strongest invariant that prepared statement
execution actually occurred.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9809338c-e4c0-43fd-8394-07bca4e21d59

📥 Commits

Reviewing files that changed from the base of the PR and between 82abf88 and 4b69e5d.

📒 Files selected for processing (3)
  • test/js/sql/sql-mysql-cached-error.test.ts
  • test/js/sql/sql-mysql-mediumint.test.ts
  • test/js/sql/sql-mysql-raw-length-prefix.test.ts

Comment thread test/js/sql/sql-mysql-cached-error.test.ts Outdated
…heck

Guards the no-increment assertion against being vacuously satisfied if
the prepared-statement path were never taken.
Comment thread test/js/sql/sql-onconnect-onclose-throw.test.ts Outdated
robobun added 2 commits June 17, 2026 22:06
Matches the forcedCloseFixture pattern in the same file: import
closedPort from ./wire-frames via absolute path inside the spawned
subprocess so the bind/close/connect window is not widened by the
tempDir write + bunExe spawn + module load, during which the
concurrent forcedCloseFixture tests are issuing bind(0). Restores the
pre-PR in-process window and drops the now-unused top-level
closedPort import.
Matches sql-mysql-bind-oob.test.ts / sql-mysql-bind-blob-borrow.test.ts /
sql-mysql-datetime-roundtrip.test.ts. describeWithContainer proceeds when
BUN_TEST_SERVICE_mysql_plain is set, but in non-docker environments that
points at a MariaDB whose root@localhost uses unix_socket auth, so
mysql://root@host:port fails. isDockerEnabled() is the established gate
for the mysql://root@ path; CI (docker-compose mysql:8 with
MYSQL_ALLOW_EMPTY_PASSWORD) is unaffected.
Comment thread test/js/sql/sql-mysql-raw-length-prefix.test.ts Outdated
…Y rollback

Restores the same-session precondition the original mock enforced via
queries=['select 1'] / desynced=false: capture CONNECTION_ID() before
the oversized attempt and after the follow-up query, and assert they
match. Without this, a regression that flushed partial bytes (or closed
on overflow) could let the pool transparently reconnect, and
'select 1 as ok' would succeed on a fresh session without the
write-buffer rollback having worked. Same pattern as the
Com_stmt_prepare check in sql-mysql-cached-error.test.ts.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All prior feedback has been addressed and no new issues found, but this is a 25-file test refactor that converts ~10 mock-server regression tests to Docker-gated container tests and introduces a new shared wire-protocol module — worth a human pass to confirm the coverage equivalence and the new isDockerEnabled() gating is acceptable.

Extended reasoning...

Overview

This PR is a test-only refactor in test/js/sql/ (net −1,064 lines, no src/ changes) with three parts: (1) a new wire-frames.ts module centralizing all hand-rolled Postgres/MySQL protocol-frame builders plus shared listeningServer/closedPort/neverAnsweringServer helpers, with a wire-frames.test.ts self-test that round-trips the builders through Bun's own parsers; (2) ~12 fault-injection tests migrated to import frames from the shared module instead of inlining Buffer.alloc/writeInt32BE sequences, each now carrying a "DO NOT COPY" header; (3) ~8 decode/roundtrip tests converted from mock net.createServer servers to real describeWithContainer("postgres_plain"|"mysql_plain") containers, plus two inline fixtures extracted to *.fixture.ts files and one (the zero-column result-set test) rewritten against a real Postgres table.

Security risks

None. All changes are to test code; no production code paths, auth, crypto, or permissions are touched. The wire-frame builders only encode protocol bytes for local mock servers used in tests.

Level of scrutiny

Medium-high. While test-only, this PR materially changes how several regression tests assert their invariants:

  • Mock→container conversions now gate previously-always-on tests behind isDockerEnabled(), so coverage on non-Docker CI legs drops.
  • Several converted tests originally instrumented the mock server's wire traffic directly (e.g. prepareCount, queries: ["select 1"], desynced: false); the real-server equivalents had to reconstruct those preconditions via SHOW SESSION STATUS / CONNECTION_ID(), which the author added in response to review feedback.
  • The deleted 197-line sql-mysql-datetime-text-mock-fixture.ts and the 188-line NEWDECIMAL mock in sql-mysql.test.ts were both judged redundant with existing container-backed siblings — that judgment is reasonable but worth a maintainer confirming.

Other factors

All six rounds of inline feedback (mine and CodeRabbit's) have been addressed in commits 82abf884458e9a, and every inline thread is resolved. The author confirmed 108 pass / 0 fail across the touched files with both containers up. The remaining CI failures are unrelated freebsd/android build-rust infra issues. A human (alii) has engaged via @robobun adopt but not yet reviewed. Given the breadth (25 files), the test-semantics changes, and the new shared module that future fault-injection tests will depend on, I'm deferring rather than approving so a maintainer can sign off on the coverage equivalence and the Docker-gating trade-off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants