Skip to content

refactor(test): use container-based postgres_tls for TLS SQL tests - #26518

Merged
Jarred-Sumner merged 4 commits into
mainfrom
claude/refactor-tls-sql-test-to-use-container
Jan 28, 2026
Merged

Jarred-Sumner merged 4 commits into
mainfrom
claude/refactor-tls-sql-test-to-use-container

Conversation

@robobun

@robobun robobun commented Jan 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Refactors tls-sql.test.ts to use describeWithContainer with a local Docker container instead of external Neon secrets
  • Updates postgres_tls service to build from Dockerfile (fixes SSL key permission issues)
  • Fixes pg_hba.conf to allow local socket connections for init scripts

Test plan

  • Verified tests pass locally with bun bd test test/js/sql/tls-sql.test.ts (30 tests pass)
  • CI passes on x64 Linux (arm64 Docker tests are currently disabled)

🤖 Generated with Claude Code

Refactors tls-sql.test.ts to use describeWithContainer with a local
postgres_tls Docker container instead of relying on external Neon secrets.

Changes:
- Update docker-compose.yml to build postgres_tls from Dockerfile instead of
  using volume mounts (fixes SSL key permission issues)
- Fix Dockerfile pg_hba.conf to allow local socket connections for init scripts
  and use broader CIDR ranges for remote TLS connections
- Update health check to use start_interval for faster startup detection
- Add postgres_tls to the list of services that need building in index.ts
- Simplify test file to use describeWithContainer pattern with prepare on/off

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@robobun

robobun commented Jan 28, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:02 PM PT - Jan 27th, 2026

❌ Your commit cf2b8677 has 3 failures in Build #36003 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 26518

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

bun-26518 --bun

@coderabbitai

coderabbitai Bot commented Jan 28, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

Switches the test PostgreSQL TLS service to a locally-built image with added startup flags and extended healthcheck timing, broadens pg_hba.conf to allow remote TLS, adds postgres_tls to pre-up builds, and refactors TLS tests to run against the containerized Postgres via a Docker-gated harness.

Changes

Cohort / File(s) Summary
Docker compose & helper
test/docker/docker-compose.yml, test/docker/index.ts
postgres_tls changed from image: postgres:15 with host-mounted volumes to a local build (build: context: ../js/sql/docker-tls, dockerfile: Dockerfile) and image bun-postgres-tls:local; removed env/volumes; added command: postgres -c hba_file=... and SSL flags; healthcheck start_period increased to 60s and start_interval added at 1s; postgres_tls added to pre-up build list.
Postgres TLS Dockerfile / pg_hba.conf
test/js/sql/docker-tls/Dockerfile, test/js/sql/docker-tls/...pg_hba.conf
Rewrote pg_hba.conf rules to allow remote TLS connections (0.0.0.0/0 and ::/0) for various users and replication while retaining a final reject rule; Dockerfile adjusted to support the new TLS-enabled, remotely-accessible configuration.
TLS test suite
test/js/sql/tls-sql.test.ts
Replaced secret/multi-URL matrix with a Docker-based harness guarded by isDockerEnabled and describeWithContainer; tests now run against the postgres_tls container using container-derived Bun.SQL.Options and include transactional, savepoint, concurrency, and error-propagation scenarios adapted to the container.

Possibly related PRs

Suggested reviewers

  • Jarred-Sumner
  • markovejnovic
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: refactoring TLS SQL tests to use a container-based postgres_tls service instead of external secrets.
Description check ✅ Passed The description covers the main changes and includes a test plan with verification steps, but lacks detail on 'How did you verify your code works?' section as specified in the template.

✏️ 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.

@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

Caution

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

⚠️ Outside diff range comments (1)
test/docker/index.ts (1)

388-390: Include postgres_tls in buildServices() to keep CI prep complete.
Line 389 still omits postgres_tls, so prepareImages() won’t prebuild the TLS image even though it now requires building. Please add it to servicesToBuild.

♻️ Suggested fix
-    const servicesToBuild = ["mysql_tls", "redis_unified"];
+    const servicesToBuild = ["mysql_tls", "redis_unified", "postgres_tls"];
🤖 Fix all issues with AI agents
In `@test/js/sql/tls-sql.test.ts`:
- Around line 8-15: Two parallel test suites use describeWithContainer with
concurrent: true causing both beforeAll hooks to call
dockerHelper.ensure("postgres_tls") and race on docker-compose up; fix by making
the container setup single-threaded: either set concurrent: false on the
describeWithContainer that uses image "postgres_tls" or merge the two suites
into one describeWithContainer and move the for (const prepare of [true, false])
loop inside it so only one beforeAll/dockerHelper.ensure() runs; update
references to describeWithContainer, beforeAll, and dockerHelper.ensure
accordingly.

Comment thread test/js/sql/tls-sql.test.ts Outdated
Move the for loop inside the describeWithContainer callback to ensure
only one beforeAll/dockerHelper.ensure() runs, avoiding the race
condition where two parallel test suites would both try to start
the postgres_tls container.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

@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: 2

🤖 Fix all issues with AI agents
In `@test/js/sql/tls-sql.test.ts`:
- Around line 26-33: The test double-disposes the SQL connection: the `await
using sql = new SQL(getOptions())` line will automatically dispose `sql` at
scope exit, so remove the explicit `await sql.close()` call to avoid a
double-close; locate the test function (tls (explicit)) and delete the final
`await sql.close();` so that disposal is handled solely by the `using`/disposer
for the `SQL` instance.
- Around line 97-113: The test currently masks the real error by converting any
rejection to the string "fail"; update the assertion to capture and assert the
actual thrown error message instead: when using sql.begin and sql.savepoint in
the "Transaction throws on uncaught named savepoint" test, replace the .catch(()
=> "fail") handler with one that returns the caught error's message (e.g., e =>
e.message) and assert that the message equals "fail" so the test verifies the
real error thrown by the savepoint.

Comment thread test/js/sql/tls-sql.test.ts
Comment thread test/js/sql/tls-sql.test.ts
- Remove explicit sql.close() call since await using already handles disposal
- Fix assertion to capture actual error message instead of masking it

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

@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

🤖 Fix all issues with AI agents
In `@test/js/sql/tls-sql.test.ts`:
- Around line 114-136: The test "Transaction succeeds on caught savepoint"
creates a persistent table; change the CREATE statement in that test to create a
temporary table instead by replacing the SQL`CREATE TABLE IF NOT EXISTS
${sql(random_name)} (a int)` call inside the test with a TEMPORARY variant
(e.g., SQL`CREATE TEMPORARY TABLE IF NOT EXISTS ${sql(random_name)} (a int)`) so
it matches other tests and prevents persistence if cleanup fails; locate this in
the test function around the random_name and SQL usage (sql`...`) and update the
SQL string accordingly.

Comment thread test/js/sql/tls-sql.test.ts
Change CREATE TABLE to CREATE TEMPORARY TABLE to match other tests
and prevent persistence if cleanup fails.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@Jarred-Sumner
Jarred-Sumner merged commit a394063 into main Jan 28, 2026
50 of 55 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/refactor-tls-sql-test-to-use-container branch January 28, 2026 07:32
xhjkl pushed a commit to xhjkl/bun that referenced this pull request May 14, 2026
…ven-sh#26518)

## Summary
- Refactors `tls-sql.test.ts` to use `describeWithContainer` with a
local Docker container instead of external Neon secrets
- Updates `postgres_tls` service to build from Dockerfile (fixes SSL key
permission issues)
- Fixes pg_hba.conf to allow local socket connections for init scripts

## Test plan
- [x] Verified tests pass locally with `bun bd test
test/js/sql/tls-sql.test.ts` (30 tests pass)
- [ ] CI passes on x64 Linux (arm64 Docker tests are currently disabled)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Bot <claude-bot@bun.sh>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants