test: let the docker test helper target a remote Docker host#31731
Merged
Conversation
Collaborator
|
Updated 2:19 PM PT - Jun 2nd, 2026
❌ @alii, your commit b818483 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31731That installs a local version of the PR into your bun-31731 --bun |
The darwin CI runners are moving to an ephemeral-VM model (Tart on Apple Silicon): each test job runs in a fresh macOS VM, and Docker runs in a separate Linux sidecar VM rather than on the same host as the test process. In that setup the container ports are published on the sidecar's address, not localhost. DockerComposeHelper hardcoded 127.0.0.1 for both the readiness probe and the returned service host. Route both through a BUN_DOCKER_TEST_HOST env var that defaults to 127.0.0.1, so the local / bare-metal path is unchanged and the sidecar path just sets the env (alongside DOCKER_HOST, which points the docker CLI at the same daemon). Verified against a real Linux Docker sidecar: with BUN_DOCKER_TEST_HOST set, ensure() returns the sidecar host and a connection through it succeeds.
alii
force-pushed
the
claude/darwin-docker-sidecar-host
branch
from
b818483 to
ac42b46
Compare
Contributor
|
Caution Review failedPull request was closed or merged during review 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 (1)
WalkthroughDockerComposeHelper adds a ChangesDocker Test Host Configuration
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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)
171-191: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInclude the target host in the timeout error message.
When debugging remote Docker connectivity (e.g., sidecar VM setup), the error message should identify which host was being probed.
Proposed improvement
- throw new Error(`Port ${port} did not become ready within ${timeout}ms`); + throw new Error(`Port ${port} on ${this.testHost} did not become ready within ${timeout}ms`);🤖 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/docker/index.ts` around lines 171 - 191, The timeout error thrown from waitForPort should include the target host being probed; update the thrown Error in the waitForPort method (use this.testHost) so the message names the host, port and timeout (e.g. “Port ${port} on ${this.testHost} did not become ready within ${timeout}ms”) to make debugging remote Docker connectivity easier.
🤖 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/docker/index.ts`:
- Around line 171-191: The timeout error thrown from waitForPort should include
the target host being probed; update the thrown Error in the waitForPort method
(use this.testHost) so the message names the host, port and timeout (e.g. “Port
${port} on ${this.testHost} did not become ready within ${timeout}ms”) to make
debugging remote Docker connectivity easier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ef674a9-ac50-44dc-b30e-91105544f984
📒 Files selected for processing (1)
test/docker/index.ts
alii
added a commit
that referenced
this pull request
Jun 2, 2026
#31732) ## What this does Converts the four `test/docker` services that bind-mounted local files — `postgres_plain`, `postgres_auth`, `autobahn`, `squid` — to **build their own images** (a small Dockerfile that `COPY`s the init-scripts/config in) instead of `image:` + `volumes:` bind mounts. ## Why Bind mounts resolve on the **Docker daemon's** filesystem, not the client's. The darwin CI runners are moving to a **sidecar-Docker model** (the daemon runs in a separate Linux VM, reached over `DOCKER_HOST` — Apple Silicon can't run accelerated Docker inside a macOS guest). In that model the bind-mounted `./init-scripts` / `./config` files don't exist on the daemon side, so they **silently don't apply** — e.g. the Postgres init scripts never run and the `bun_sql_test` role is missing, which then looks like a confusing test failure rather than a setup problem. `docker build`'s context, by contrast, **ships to the daemon**, so baking the files into the image works against both a local daemon and a remote/sidecar one. This follows the pattern the repo already uses for its TLS services (`postgres_tls`, `mysql_tls`, `redis_unified` are all `build:`-based). ## Changes - New `Dockerfile.postgres-plain`, `Dockerfile.postgres-auth`, `Dockerfile.autobahn`, `Dockerfile.squid` (each `FROM` the stock image + `COPY` the scripts/config). - The four services now use `build:` + a `bun-*:local` image tag; their `volumes:` bind mounts are removed. ## Compatibility - **Local dev unchanged in behavior** — `docker compose up` just builds the image on first run (cached after), then runs identically. Init scripts/config are baked in rather than mounted. - Minor DX note: editing a baked config now needs a `docker compose build` to pick up (same as the existing TLS services). ## Verification Ran the real compose against an actual Linux Docker sidecar VM (Tart) over `DOCKER_HOST`: - `docker compose config` valid; all four images **build** (context shipped to the remote daemon). - `postgres_plain` + `postgres_auth` come up **healthy**, and the baked init scripts run → **`bun_sql_test` role present on both** (the exact thing that was silently missing with bind mounts). ## Context Second piece of the darwin Tart-runner migration; pairs with #31731 (the `BUN_DOCKER_TEST_HOST` host indirection).
alii
added a commit
that referenced
this pull request
Jun 5, 2026
…ect using uri' (#31901) ## What One-line fix in `test/js/sql/sql.test.ts`: the "Connect using uri" test built its connection string with a hardcoded `@localhost:` instead of `container.host`. ## Why Every other test in the file uses `container.host` (which respects `BUN_DOCKER_TEST_HOST`, added in #31731), so they pass against any Docker setup — local or remote daemon. This one test only worked when the daemon is on the same machine. Against a remote daemon it fails instantly with `PostgresError: Connection closed`, and that single subtest marks the whole 845-test file as failed even though the other 844 pass. ## How verified With this change, the full `sql.test.ts` passes (843 pass / 2 todo / 0 fail) against a remote Docker daemon on macOS arm64, and behavior on a local daemon is unchanged (`container.host` resolves to `127.0.0.1` there).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
test/docker/index.ts(DockerComposeHelper) hardcoded127.0.0.1for both the port-readiness probe and thehostit returns fromensure(). This adds aBUN_DOCKER_TEST_HOSTenv var (default127.0.0.1) and routes both through it.Why
The darwin CI runners are moving to an ephemeral-VM model — Tart on Apple Silicon, where each test job runs in a fresh macOS VM and Docker runs in a separate Linux sidecar VM (Apple Silicon can't run accelerated Docker inside a macOS guest, so the daemon lives next door). In that setup the container ports are published on the sidecar's address, not
localhost, so the test process needs to connect there.This is the first, self-contained piece of that migration. On its own it's a no-op for everyone today.
Backward compatible
BUN_DOCKER_TEST_HOSTunset →127.0.0.1, exactly as before. Local dev and the current bare-metal agents are unaffected.BUN_DOCKER_TEST_HOST=<sidecar-ip>alongsideDOCKER_HOST=tcp://<sidecar-ip>:2375(which points thedockerCLI at the same daemon).Verification
Ran the real
ensure("postgres_plain")against an actual Linux Docker sidecar VM (Tart) from a macOS host, withBUN_DOCKER_TEST_HOST+DOCKER_HOSTset:What's next (not in this PR)
The follow-up changes that complete the sidecar model: switching bind-mounted init-scripts/configs to
build:/COPY(bind mounts don't cross to a remote daemon;docker buildcontext does — already validated), plus the CI pipeline wiring. This PR is just the host indirection that everything else builds on.