Skip to content

test(sql): cover both adapters in the synchronous-tls-rejection test#32415

Merged
alii merged 1 commit into
mainfrom
ali/sql-invalid-tls-test
Jun 16, 2026
Merged

test(sql): cover both adapters in the synchronous-tls-rejection test#32415
alii merged 1 commit into
mainfrom
ali/sql-invalid-tls-test

Conversation

@alii

@alii alii commented Jun 16, 2026

Copy link
Copy Markdown
Member

Extends the existing postgres-only rejects tls that is neither a boolean nor an object test (added in #32352) to mysql via test.each.

The native constructor throws synchronously on a non-boolean/non-object tls, and createPooledConnectionHandle defers onClose via process.nextTick (since #32145) so the pending query rejects cleanly instead of hanging. This locks that in for both adapters.

No code change; 2/2 pass on bun bd test.

Extends the existing postgres-only test to mysql via test.each. The
constructor throws synchronously and createPooledConnectionHandle
defers onClose via process.nextTick (since #32145) so the pending
query rejects cleanly; this locks that in for both adapters.
@robobun

robobun commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator
Updated 3:26 PM PT - Jun 16th, 2026

@alii, your commit d852045 has 2 failures in Build #62866 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32415

That installs a local version of the PR into your bun-32415 executable, so you can run:

bun-32415 --bun

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4292955d-6b50-43d0-8fd2-3344ccbf6020

📥 Commits

Reviewing files that changed from the base of the PR and between 62cc078 and d852045.

📒 Files selected for processing (1)
  • test/js/sql/sql.test.ts
👮 Files not reviewed due to content moderation or server errors (1)
  • test/js/sql/sql.test.ts

Walkthrough

The TLS validation test in the "shared createInstance validation (no server)" suite is converted from a single-adapter case to a parameterized test.concurrent.each that runs the same invalid-tls assertion (tls: 1 as any, expecting "tls must be a boolean or an object") for both postgres and mysql adapters.

Changes

TLS Validation Test Parameterization

Layer / File(s) Summary
Parameterize TLS rejection test for postgres and mysql
test/js/sql/sql.test.ts
Replaces a single test.concurrent(...) call with test.concurrent.each(["postgres", "mysql"]), so the invalid-tls rejection assertion (tls: 1 as any"tls must be a boolean or an object") now runs for both adapters.

Possibly related PRs

  • oven-sh/bun#32352: Modifies the same createInstance "no server" validation suite in test/js/sql/sql.test.ts, touching TLS-related and null-byte validation assertions in closely adjacent test code.

Suggested reviewers

  • cirospaciari
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description lacks both required template sections (What does this PR do? and How did you verify your code works?) and provides only informal explanation instead. Add the required template sections: 'What does this PR do?' and 'How did you verify your code works?' with appropriate content.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: extending a test to cover both PostgreSQL and MySQL adapters in the synchronous TLS rejection test.
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.

@alii alii enabled auto-merge (squash) June 16, 2026 20:32
@alii

alii commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@robobun can you stamp/approve this pr?

@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.

LGTM — test-only change parameterizing an existing assertion across both SQL adapters.

Extended reasoning...

Overview

This PR touches a single test file (test/js/sql/sql.test.ts), converting one existing test.concurrent case into a test.concurrent.each(["postgres", "mysql"]) so the synchronous tls must be a boolean or an object rejection is asserted for both adapters instead of just postgres. The test body is unchanged aside from spreading adapter over base (which already defaults to postgres) and an expanded comment referencing #32145.

Security risks

None. No production code is modified; the change only adds a second parameterized run of an existing negative-path test against a non-listening port (127.0.0.1:1), so no network or auth surface is involved.

Level of scrutiny

Low. This is a mechanical test extension following the exact test.concurrent.each pattern already used a few lines above in the same describe block (e.g. the null-byte credential test). The diff is small, self-contained, and the PR description confirms 2/2 pass locally.

Other factors

The bug-hunting system found no issues. There are no outstanding reviewer comments, and the only timeline entry is the robobun build link. The override of base.adapter via spread is correct (later key wins), so the mysql case actually exercises the mysql path.

@alii

alii commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@robobun please! it's really simple. I actually meant can you review it and then stamp...

@alii

alii commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@robobun please can you review it 😢 you don't have to stamp for now, just a review

@alii

alii commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

lol

@alii alii disabled auto-merge June 16, 2026 22:09
@alii alii merged commit 66cd2fd into main Jun 16, 2026
77 of 79 checks passed
@alii alii deleted the ali/sql-invalid-tls-test branch June 16, 2026 22:09
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