test/docker: BUN_TEST_SERVICE_<name> env override for ensure() - #32139
Conversation
|
Updated 6:21 PM PT - Jun 11th, 2026
❌ @alii, your commit 00a63f5 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32139That installs a local version of the PR into your bun-32139 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
WalkthroughDocker test services gain env-driven overrides and a metadata table for ports/TLS/users. DockerComposeHelper prefers env overrides, uses metadata-driven port resolution, tracks compose startup, and skips down if unused. describeWithContainer defers unknown-image errors and skips tests only when no override/coordinator and Docker is unavailable. ChangesDocker service configuration and harness gating refactor
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/valkey/test-utils.ts (1)
240-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate error message to reflect env vars are also an option.
The error message "Docker Compose is required" is misleading after adding the env-var override path. By the time this error is thrown, both the env-var path and docker-compose path have failed.
Proposed fix
} catch (error) { console.error("Failed to start Redis via docker-compose:", error); - throw new Error(`Docker Compose is required. Redis container failed to start via docker-compose: ${error}`); + throw new Error( + `Redis unavailable: set BUN_TEST_REDIS_HOST/PORT env vars or ensure docker-compose is available. ` + + `Docker-compose error: ${error}` + ); }🤖 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/valkey/test-utils.ts` around lines 240 - 243, Update the catch block that currently logs "Failed to start Redis via docker-compose:" and throws "Docker Compose is required…" in test/js/valkey/test-utils.ts to reflect that the env-var override was attempted too; change both the console.error and thrown Error to indicate that both docker-compose and the environment variable fallback (e.g., REDIS_URL/Redis env-var) failed, include the actual error details in the messages, and ensure the thrown Error text clearly states that either Docker Compose or the env-var override must succeed.test/integration/mysql2/mysql2.test.ts (1)
29-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the resolved host into
createConnection.On Line 38, the connection options include
portbut omithost. WithdescribeWithMysql, the resolved endpoint can come from env/docker, so mysql2 will otherwise default to localhost and may connect to the wrong server.Suggested fix
test("can connect to database", async () => { sql = await createConnection({ ...client, + host: container.host, port: container.port, }); });🤖 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/integration/mysql2/mysql2.test.ts` around lines 29 - 41, The test calls createConnection inside the describeWithMysql block but only passes port (so mysql2 may default to localhost); update the createConnection call in the test (inside describeWithMysql / test "can connect to database") to include the resolved host from the test container (e.g., add host: container.host or the container property that exposes the resolved hostname) alongside port and existing client options so the connection uses the correct endpoint.
🤖 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/harness.ts`:
- Around line 1221-1227: The current env override block ignores partial
overrides; update the logic around envHost/envPort so that if envHost
(spec.envHost) is set you treat the override as authoritative: always assign
_host = process.env[spec.envHost], then if process.env[spec.envPort] exists
parse it to a number and fail fast (throw or process.exit) if
Number(process.env[spec.envPort]) is NaN, otherwise set _port to that number; if
process.env[spec.envPort] is missing set _port = spec.internalPort as the
default; keep setting _tlsPort from process.env[spec.envTlsPort] only when that
env var exists (and validate numeric similarly), and ensure _source = "env". Use
the existing symbols envHost, envPort, _host, _port, spec.envTlsPort, _tlsPort,
and spec.internalPort to locate and implement this change.
In `@test/js/bun/s3/s3.test.ts`:
- Around line 78-103: The Docker fallback currently shells out with a literal
piped command and ignores mc mb failures; update the branch guarded by
isDockerEnabled() to use the dockerCLI variable for all subprocesses instead of
execSync with a shell pipeline: call child_process.spawnSync(dockerCLI, [`ps`,
`--filter`, `ancestor=minio/minio:latest`, `--filter`, `status=running`,
`--format`, `{{.Names}}`]) to get containerName from stdout (no use of head),
check spawnSync.status and stdout to fail fast if the ps command fails or
returns empty, then call child_process.spawnSync(dockerCLI, [`exec`,
containerName, `mc`, `mb`, `data/buntest`]) and verify its exit code and
stderr/stdout; if mc mb fails, throw or call processLogger.error/process.exit(1)
so tests don't point at a non-existent bucket; keep
dockerCompose.ensure("minio") and minioInfo usage but remove any reliance on
shell-only helpers.
In `@test/js/sql/local-sql.test.ts`:
- Around line 142-145: The catch block in test/js/sql/local-sql.test.ts
currently calls process.exit(1), which aborts the whole test runner; instead,
remove the process.exit(1) and rethrow the caught error (or throw a new Error
with context) so the test framework fails and runs normal teardown; update the
catch handling where the container startup is awaited (the try/catch around the
local SQL container startup in this test helper) to console.error the error if
desired then throw error to propagate failure to the test runner.
- Around line 276-325: The Promise executor for spawning the server is async
which can hide failures; remove the async executor and instead use a synchronous
executor that launches an internal async helper (e.g., an async IIFE) to await
reader.read() and call resolve/reject; specifically, in the spawnServer block
replace "new Promise(async (resolve, reject) => { ... const url =
decoder.decode((await reader.read()).value); resolve(...)" with "new
Promise((resolve, reject) => { (async () => { const res = await reader.read();
if (res.done || !res.value) { server.kill(); return reject(new Error('no stdout
from server')); } const url = decoder.decode(res.value); resolve({ url, kill: ()
=> server.kill() }); })().catch(err => { server.kill(); reject(err); });" and
keep outputData(reader) / errorReader handling; ensure you check reader.read()
result for done/undefined before decoding and call reject/cleanup on error
(reference server, reader, errorReader, decoder, outputData).
In `@test/js/valkey/test-utils.ts`:
- Around line 199-214: The env-override branch trusts envTlsPort fallback to
REDIS_TLS_PORT (which is random) and doesn’t validate reachability; update the
redisSource === "env" block to require envTlsPort be set (do not fall back to
REDIS_TLS_PORT) or explicitly mark TLS tests as skipped when it's absent, and
add a connectivity check before returning: after computing host/port/tlsPort use
the same probe logic used in the docker path (e.g. attempt a TCP/TLS connection
or reuse UnixDomainSocketProxy.create/connection attempt) and throw/log and skip
tests if the endpoint is unreachable; key symbols to change: redisSource,
envHost/envPort/envTlsPort, REDIS_TLS_PORT, UnixDomainSocketProxy.create,
applyRedisEndpoint, containerConfig, dockerStarted.
In `@test/regression/issue/28632.test.ts`:
- Around line 15-21: The test suite creates a shared SQL client in beforeAll
(SQL instance `sql`) but never closes it; add an afterAll teardown that closes
the client (call sql.end() to terminate the DB connection) to ensure no open
connections remain after the test suite finishes; implement this by adding an
afterAll(async () => { if (sql) await sql.end(); }) paired with the existing
beforeAll so the shared SQL client is properly cleaned up.
---
Outside diff comments:
In `@test/integration/mysql2/mysql2.test.ts`:
- Around line 29-41: The test calls createConnection inside the
describeWithMysql block but only passes port (so mysql2 may default to
localhost); update the createConnection call in the test (inside
describeWithMysql / test "can connect to database") to include the resolved host
from the test container (e.g., add host: container.host or the container
property that exposes the resolved hostname) alongside port and existing client
options so the connection uses the correct endpoint.
In `@test/js/valkey/test-utils.ts`:
- Around line 240-243: Update the catch block that currently logs "Failed to
start Redis via docker-compose:" and throws "Docker Compose is required…" in
test/js/valkey/test-utils.ts to reflect that the env-var override was attempted
too; change both the console.error and thrown Error to indicate that both
docker-compose and the environment variable fallback (e.g., REDIS_URL/Redis
env-var) failed, include the actual error details in the messages, and ensure
the thrown Error text clearly states that either Docker Compose or the env-var
override must succeed.
🪄 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: 134de2aa-1a19-43bc-9be0-67c6eb2ac57a
📒 Files selected for processing (23)
test/harness.tstest/integration/mysql2/mysql2.test.tstest/js/bun/s3/s3.test.tstest/js/sql/local-sql.test.tstest/js/sql/sql-mysql-bind-blob-borrow.test.tstest/js/sql/sql-mysql-bind-oob.test.tstest/js/sql/sql-mysql-column-name-digits.test.tstest/js/sql/sql-mysql-datetime-roundtrip.test.tstest/js/sql/sql-mysql.auth.test.tstest/js/sql/sql-mysql.helpers.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql-mysql.transactions.test.tstest/js/sql/sql-onconnect-onclose-throw.test.tstest/js/sql/sql-postgres-datetime-roundtrip.test.tstest/js/sql/sql-prepare-false.test.tstest/js/sql/sql.test.tstest/js/sql/tls-sql.test.tstest/js/valkey/test-utils.tstest/regression/issue/21311.test.tstest/regression/issue/24850.test.tstest/regression/issue/26030.test.tstest/regression/issue/26063.test.tstest/regression/issue/28632.test.ts
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 (2)
test/js/sql/local-sql.test.ts (1)
80-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the port reservation race in
findRandomPort.This reserves and releases a host port before Docker binds it, so another process can take it and make
docker run -p ${port}:5432flaky.As per coding guidelines, "
**/*.test.{ts,tsx}: Useport: 0in server tests - do not hardcode ports or implement custom random port functions".🤖 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/local-sql.test.ts` around lines 80 - 89, The test's findRandomPort function reserves a host port then releases it, causing a race when running docker run -p ${port}:5432; remove findRandomPort and stop reserving host ports. Instead start the container without hardcoding the host port (use Docker's automatic publishing, e.g., docker run -P or the test framework's equivalent) and after the container starts query the container port mapping (docker port or inspect) to get the assigned host port; update any code that uses docker run -p ${port}:5432 to use automatic publish and then read the mapped host port at runtime.Source: Coding guidelines
test/integration/mysql2/mysql2.test.ts (1)
30-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the resolved service host when creating MySQL connections.
Line 40 only passes
port, so this suite ignorescontainer.hostfromdescribeWithMysql. That breaks env-resolved/non-localhost service targets introduced by the new harness resolution flow.Suggested fix
sql = await createConnection({ ...client, + host: container.host, port: container.port, });🤖 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/integration/mysql2/mysql2.test.ts` around lines 30 - 41, The test is only passing container.port to createConnection and ignores the resolved service host, so update the createConnection call inside the describeWithMysql block (the test "can connect to database") to include container.host as the host option in addition to container.port (i.e., spread client and add host: container.host, port: container.port) so the suite uses the resolved service host provided by describeWithMysql when creating the MySQL Connection.
♻️ Duplicate comments (2)
test/js/sql/local-sql.test.ts (2)
142-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not terminate the entire test process from helper startup failures.
Line 144 (
process.exit(1)) aborts the test runner and bypasses normal suite failure/teardown reporting. Throw the error so this suite fails cleanly.Suggested fix
} catch (error) { console.error("Error:", error); - process.exit(1); + throw error; }🤖 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/local-sql.test.ts` around lines 142 - 145, The catch block in test/js/sql/local-sql.test.ts currently calls process.exit(1) after logging the error, which aborts the test runner; replace the call to process.exit(1) with throwing the caught error (throw error) so the test framework records the failure and runs teardown. Locate the catch that does console.error("Error:", error) and change it to rethrow the error instead of exiting the process.
276-326:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace the async Promise executor in
spawnServer.Using
new Promise(async ...)can leave startup failures afterawaituncoupled fromresolve/reject, which risks hangs in this stress test flow.Suggested fix
- function spawnServer(controller) { - return new Promise(async (resolve, reject) => { + function spawnServer(controller) { + return new Promise((resolve, reject) => { + (async () => { const server = Bun.spawn([bunExe(), "index.ts"], { stdin: "ignore", stdout: "pipe", stderr: "pipe", cwd: dir, env: { ...bunEnv, BUN_DEBUG_QUIET_LOGS: "1", DATABASE_URL: connectionString, DATABASE_CA: path.join(import.meta.dir, "docker-tls", "server.crt"), }, onExit(proc, exitCode, signalCode, error) { if (exitCode !== 0) { failed = true; controller.abort(); } }, }); const reader = server.stdout.getReader(); const errorReader = server.stderr.getReader(); const decoder = new TextDecoder(); async function outputData(reader, type = "log") { while (true) { const { done, value } = await reader.read(); if (done) break; if (value) { if (type === "error") { console.error(decoder.decode(value)); } else { console.log(decoder.decode(value)); } } } } - const url = decoder.decode((await reader.read()).value); + const first = await reader.read(); + if (first.done || !first.value) throw new Error("Server did not print URL"); + const url = decoder.decode(first.value); resolve({ url, kill: () => server.kill() }); outputData(reader); errorReader.read().then(({ value }) => { if (value) { console.error(decoder.decode(value)); failed = true; } outputData(errorReader, "error"); }); + })().catch(reject); }); }#!/bin/bash # Verify no async Promise executors remain in this test file. rg -nP 'new Promise\s*\(\s*async\s*\(' test/js/sql/local-sql.test.ts🤖 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/local-sql.test.ts` around lines 276 - 326, The Promise executor in spawnServer uses an async function (new Promise(async (resolve, reject) => ...)) which can detach awaited errors from reject; change the executor to a synchronous function and move any awaits into an async IIFE or promise chains so all startup failures are routed to reject. Concretely, in the block that creates the Bun.spawn server (and the subsequent reader.read() that yields the url), remove the async keyword from the Promise executor, wrap the startup awaits (e.g., the initial reader.read() that reads url) in an immediately-invoked async function (async () => { try { const url = decoder.decode((await reader.read()).value); resolve({ url, kill: () => server.kill() }); } catch (err) { reject(err); } })(); and also ensure the onExit handler and errorReader handling call reject or set failed consistently so no startup failure remains uncaught.
🤖 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/integration/mysql2/mysql2.test.ts`:
- Around line 30-41: The test is only passing container.port to createConnection
and ignores the resolved service host, so update the createConnection call
inside the describeWithMysql block (the test "can connect to database") to
include container.host as the host option in addition to container.port (i.e.,
spread client and add host: container.host, port: container.port) so the suite
uses the resolved service host provided by describeWithMysql when creating the
MySQL Connection.
In `@test/js/sql/local-sql.test.ts`:
- Around line 80-89: The test's findRandomPort function reserves a host port
then releases it, causing a race when running docker run -p ${port}:5432; remove
findRandomPort and stop reserving host ports. Instead start the container
without hardcoding the host port (use Docker's automatic publishing, e.g.,
docker run -P or the test framework's equivalent) and after the container starts
query the container port mapping (docker port or inspect) to get the assigned
host port; update any code that uses docker run -p ${port}:5432 to use automatic
publish and then read the mapped host port at runtime.
---
Duplicate comments:
In `@test/js/sql/local-sql.test.ts`:
- Around line 142-145: The catch block in test/js/sql/local-sql.test.ts
currently calls process.exit(1) after logging the error, which aborts the test
runner; replace the call to process.exit(1) with throwing the caught error
(throw error) so the test framework records the failure and runs teardown.
Locate the catch that does console.error("Error:", error) and change it to
rethrow the error instead of exiting the process.
- Around line 276-326: The Promise executor in spawnServer uses an async
function (new Promise(async (resolve, reject) => ...)) which can detach awaited
errors from reject; change the executor to a synchronous function and move any
awaits into an async IIFE or promise chains so all startup failures are routed
to reject. Concretely, in the block that creates the Bun.spawn server (and the
subsequent reader.read() that yields the url), remove the async keyword from the
Promise executor, wrap the startup awaits (e.g., the initial reader.read() that
reads url) in an immediately-invoked async function (async () => { try { const
url = decoder.decode((await reader.read()).value); resolve({ url, kill: () =>
server.kill() }); } catch (err) { reject(err); } })(); and also ensure the
onExit handler and errorReader handling call reject or set failed consistently
so no startup failure remains uncaught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e366511f-5d42-4763-ba9a-0e59e8114f01
📒 Files selected for processing (21)
test/harness.tstest/integration/mysql2/mysql2.test.tstest/js/sql/local-sql.test.tstest/js/sql/sql-mysql-bind-blob-borrow.test.tstest/js/sql/sql-mysql-bind-oob.test.tstest/js/sql/sql-mysql-column-name-digits.test.tstest/js/sql/sql-mysql-datetime-roundtrip.test.tstest/js/sql/sql-mysql.auth.test.tstest/js/sql/sql-mysql.helpers.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql-mysql.transactions.test.tstest/js/sql/sql-onconnect-onclose-throw.test.tstest/js/sql/sql-postgres-datetime-roundtrip.test.tstest/js/sql/sql-prepare-false.test.tstest/js/sql/sql.test.tstest/js/sql/tls-sql.test.tstest/regression/issue/21311.test.tstest/regression/issue/24850.test.tstest/regression/issue/26030.test.tstest/regression/issue/26063.test.tstest/regression/issue/28632.test.ts
alii
changed the title
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
test/harness.ts:1149-1153— IfisDockerEnabled()is true butensure()rejects (image pull failure, container crash, port never ready, etc.), this throws out ofresolveService→describeWithService→ the top-levelawait describeWith…(...), aborting module evaluation so no tests in the file run — including docker-independent ones (e.g. the refused-connection / mock-server cases insql-onconnect-onclose-throw.test.ts,tls-sql.test.ts,sql-mysql-datetime-roundtrip.test.ts). The olddescribeWithContainerdeliberately deferredensure()tobeforeAllso only that describe block failed; wrapping this call intry { … } catch { return null }(→describe.todo) would restore that isolation and match the "cleandescribe.todo, no errors" contract in the doc comment. (Secondary: the inline await also serializes container startup across thefor-loops insql-mysql.test.ts/mysql2.test.ts— likely fine given the CI coordinator pre-starts them in parallel, but worth confirming the trade-off was intentional.)Extended reasoning...
What happens
resolveService()(test/harness.ts:1149-1154) does:if (isDockerEnabled()) { const { ensure } = await import("./docker/index.ts"); const info = await ensure(`${spec.kind}_${variant}`); return mk(info.host, info.ports[spec.port], "docker", …); }
with no
try/catch.describeWithService()then doesconst svc = await resolveService(…)(line 1173) with notry/catcheither. Every migrated test file calls this at module top level:await describeWithPostgres(...),await describeWithMysql(...), etc.ensure()intest/docker/index.tsre-throws onensureDocker()failure, onup()failure ("Failed to start service …"), on a coordinatorok: falsereply, and onport()lookup failure ("Port did not become ready within …ms"). So when docker is available but a specific compose service fails to come up, the rejection propagates straight through to the top-levelawait, module evaluation throws, and bun:test reports the file as a load error — no tests register, including ones declared before the throwingawait(bun:test runs tests only after the module finishes loading).Why the old code didn't have this problem
describeWithContainer(still in harness.ts) was specifically designed around this: it kicks offensure()at describe-define time as a fire-and-forgetstartPromise, attachesstartPromise.catch(readyRejecter!), and onlyawaits it insidebeforeAll. The harness comment says exactly why: "Surface any rejection through ready; without a handler the runner would see an unhandled rejection before beforeAll re-throws it." A failure surfaced as that one describe block'sbeforeAllfailing; the module finished evaluating and every other test/describe in the file still ran. The oldsql-prepare-false.test.tseven had an explicittry { await dockerCompose.ensure(...) } catch { test.skip(...); return; }for the same reason — this PR removes it.Step-by-step example
- CI agent has docker;
isDockerEnabled()returnstrue. mysql_plainimage pull transiently fails (or the container's healthcheck never passes, or the published port is busy).bun test test/js/sql/sql-onconnect-onclose-throw.test.tsstarts evaluating the module.- Line 54:
await describeWithPostgres("postgres", …)succeeds; the postgres describe registers. - Line 72:
await describeWithMysql("mysql", …)→resolveService→await ensure("mysql_plain")rejects. - The rejection propagates to the top-level
await; module evaluation aborts. - The three
test.concurrent(...)cases at lines 92-180 — connection-refused / synchronous-failure scenarios that use a closed local port and need no docker — never register. Neither do the postgres tests registered in step 4, since bun:test discards a file whose module body throws.
The same applies to
tls-sql.test.ts(two mock-PostgreSQL-server tests afterawait describeWithPostgres(..., { variant: "tls" })) andsql-mysql-datetime-roundtrip.test.ts(thedescribe.eachmock-server tests). Before this PR all of those would have run regardless of compose health.On the secondary serialization point, and the counter-argument
The same inline
await ensure(...)also meansfor (const image of images) { await describeWithMysql(...) }insql-mysql.test.tsbrings upmysql_tls→mysql_plain→mysql_native_passwordone at a time, whereasdescribeWithContainerstarted them in parallel (its comment: "Kick off ensure() at describe-define time so a file with multiple describeWithContainer blocks starts all of its containers in parallel").The objection here is fair: in CI,
test/docker/coordinator.tshasprestartMap["js/sql/sql-mysql"] = ["mysql_plain", "mysql_native_password", "mysql_tls"]and fires all threeensureService()calls in parallel at shard launch before the test file loads, so by the time the sequentialawaits run the containers are already in flight via the coordinator socket and wall-clock ≈ slowest container. And the commit title "Simplify service helpers: async resolution via top-level await" suggests the parallelism trade-off was conscious. So the serialization is a nit at most, mentioned only so the author can confirm it's intentional for the local-dev cold-start case the coordinator doesn't cover.But that mitigation does not apply to the error-isolation issue: when the coordinator's
ensureServicefails it sends{ ok: false, error }andensureViaCoordinatorrejects — which still propagates through the unguardedawait ensure(...)at line 1153 and aborts the whole file. The "4. describe.todo" fallback in the doc comment is only reached whenisDockerEnabled()is false; the docker-enabled-but-broken path violates the PR's stated "Nothing available: cleandescribe.todo, no errors, no hangs" contract.Suggested fix
if (isDockerEnabled()) { try { const { ensure } = await import("./docker/index.ts"); const info = await ensure(`${spec.kind}_${variant}` as …); return mk(info.host, info.ports[spec.port], "docker", spec.tlsPort ? info.ports[spec.tlsPort] : undefined); } catch (e) { console.warn(`resolveService(${spec.kind}_${variant}): ensure() failed, skipping:`, e); return null; // → describe.todo(label) } }
This keeps the simpler top-level-await control flow while restoring per-describe failure isolation.
- CI agent has docker;
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/js/valkey/test-utils.ts (3)
37-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent env-var handling: require either HOST or PORT, not both.
The harness.ts
resolveServiceat line 1087 usesif (envHost || envPort)to treat either env var as an explicit override and defaults the other. This file requires both (if (envHost && envPort)), which is inconsistent with the PR's stated intent ("either host or port being set is treated as an explicit override; the other value is defaulted") and will cause valkey tests to skip when only one env var is set while postgres/mysql tests run. Match the harness behavior.🔧 Proposed fix
const envHost = process.env.BUN_TEST_REDIS_HOST; const envPort = process.env.BUN_TEST_REDIS_PORT; const envTlsPort = process.env.BUN_TEST_REDIS_TLS_PORT; let redisSource: RedisSource; - if (envHost && envPort) { + if (envHost || envPort) { redisSource = "env"; } else if (hasDocker) {🤖 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/valkey/test-utils.ts` around lines 37 - 48, The env-var handling is inconsistent: change the condition that sets redisSource from requiring both envHost and envPort to accept either one so it matches resolveService behavior; update the block that assigns redisSource (referencing envHost, envPort, redisSource, hasDocker) to use if (envHost || envPort) then redisSource = "env" (else if hasDocker => "docker" else "none"), so a single env var overrides and the other will be defaulted.
537-537: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUpdate warning message to reflect either/or env var handling.
The message "no BUN_TEST_REDIS_HOST/PORT" implies both env vars are required, but the intent (and harness.ts behavior) is that either is sufficient. Update the wording to match.
📝 Proposed wording
- console.warn("Redis is not available (no BUN_TEST_REDIS_HOST/PORT, no docker); skipping tests"); + console.warn("Redis is not available (no BUN_TEST_REDIS_HOST or BUN_TEST_REDIS_PORT, no docker); skipping 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/valkey/test-utils.ts` at line 537, The console.warn message in test/js/valkey/test-utils.ts currently implies both env vars are required; update the string in the console.warn call so it accurately reflects that either BUN_TEST_REDIS_HOST or BUN_TEST_REDIS_PORT can enable Redis (e.g. say "Redis is not available (neither BUN_TEST_REDIS_HOST nor BUN_TEST_REDIS_PORT set, and no docker); skipping tests" or similar), leaving the console.warn call as the single change.
202-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate env-provided Redis endpoint before proceeding.
The env-override path trusts the caller-provided host/port without validating that Redis is actually accessible. If the env vars are misconfigured, tests will fail with connection errors instead of a clear "cannot reach env redis" message at startup. Add a connectivity check (similar to the docker path's implicit validation via
ensure()) and fail fast with a helpful error if the endpoint is unreachable.🔍 Proposed validation
if (redisSource === "env") { const host = envHost!; const port = parseInt(envPort!, 10); const tlsPort = envTlsPort ? parseInt(envTlsPort, 10) : port; + // Validate that the env-provided Redis is accessible + try { + const testClient = new RedisClient(`redis://${host}:${port}`); + await testClient.send("PING", []); + testClient.close(); + } catch (error) { + throw new Error( + `Cannot connect to Redis at ${host}:${port} (BUN_TEST_REDIS_HOST/PORT): ${error}` + ); + } unixSocketProxy = await UnixDomainSocketProxy.create("Redis", host, port);🤖 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/valkey/test-utils.ts` around lines 202 - 214, The env-override branch currently trusts envHost/envPort without verifying connectivity; add a reachability check (similar to the docker path's ensure()) after creating unixSocketProxy via UnixDomainSocketProxy.create and before calling applyRedisEndpoint/returning containerConfig: attempt a quick Redis PING (or TCP connect) to the proxied endpoint (using unixSocketProxy.path or host:port), and if it fails, tear down the proxy (clean up unixSocketProxy), throw or log a clear error like "cannot reach env redis at <host>:<port>" so tests fail fast; only set dockerStarted=true and return containerConfig after the connectivity check succeeds.
♻️ Duplicate comments (1)
test/js/valkey/test-utils.ts (1)
205-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTLS port fallback breaks TLS tests; require explicit TLS port or skip TLS coverage.
When
BUN_TEST_REDIS_TLS_PORTis not set, line 205 defaultstlsPortto the same value as the non-TLSport. The docker path uses distinct ports (6379 vs 6380), so TLS tests expect a separate TLS-enabled port. Defaulting to the same port will cause TLS tests to attempt a TLS handshake on the non-TLS port and fail with confusing errors. Either requireenvTlsPortto be set for env-override mode (and document it), or explicitly skip TLS test coverage when it's absent.💡 Proposed fix: require TLS port
- const tlsPort = envTlsPort ? parseInt(envTlsPort, 10) : port; + if (!envTlsPort) { + console.warn( + "BUN_TEST_REDIS_TLS_PORT not set; TLS tests will fail. " + + "Set BUN_TEST_REDIS_TLS_PORT to enable TLS coverage with env-override redis." + ); + } + const tlsPort = envTlsPort ? parseInt(envTlsPort, 10) : 0;Then update TLS test groups to skip when
tlsPort === 0.🤖 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/valkey/test-utils.ts` at line 205, The current fallback sets tlsPort to the non-TLS port causing TLS tests to hit a non-TLS endpoint; change the logic around tlsPort/envTlsPort so TLS tests only run when an explicit TLS port is provided: set tlsPort = envTlsPort ? parseInt(envTlsPort, 10) : 0 (or otherwise throw when running in env-override mode), and update the TLS test groups to detect tlsPort === 0 and skip TLS coverage accordingly; reference the tlsPort variable, envTlsPort (BUN_TEST_REDIS_TLS_PORT) and the TLS test groups to locate where to apply the change.
🤖 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/harness.ts`:
- Around line 1091-1095: Wrap the docker branch in a try/catch so failures from
the dynamic import or ensure() (the call to import("./docker/index.ts") and
ensure(`${kind}_${variant}` as import("./docker/index.ts").ServiceName)) are
caught and do not throw during describe-definition; on catch return null instead
of propagating the error so the caller (describeWithService) can convert the
service to describe.todo; keep the existing call to found(info.host,
info.ports[defaultPort], "docker") in the success path.
- Around line 1087-1089: The envPort value is not validated before being
converted to Number in the env-host/port branch; update the logic around the
envHost/envPort check to parse envPort (e.g., via Number or parseInt) and if it
is not a finite integer (isNaN or !Number.isFinite/!Number.isInteger as
appropriate) fail fast with a clear error message instead of passing NaN to
found; adjust the branch that returns found(envHost ?? "127.0.0.1", envPort ?
Number(envPort) : defaultPort, "env") to validate envPort and throw or return a
descriptive parse-time error including the invalid envPort value and expected
numeric port range.
---
Outside diff comments:
In `@test/js/valkey/test-utils.ts`:
- Around line 37-48: The env-var handling is inconsistent: change the condition
that sets redisSource from requiring both envHost and envPort to accept either
one so it matches resolveService behavior; update the block that assigns
redisSource (referencing envHost, envPort, redisSource, hasDocker) to use if
(envHost || envPort) then redisSource = "env" (else if hasDocker => "docker"
else "none"), so a single env var overrides and the other will be defaulted.
- Line 537: The console.warn message in test/js/valkey/test-utils.ts currently
implies both env vars are required; update the string in the console.warn call
so it accurately reflects that either BUN_TEST_REDIS_HOST or BUN_TEST_REDIS_PORT
can enable Redis (e.g. say "Redis is not available (neither BUN_TEST_REDIS_HOST
nor BUN_TEST_REDIS_PORT set, and no docker); skipping tests" or similar),
leaving the console.warn call as the single change.
- Around line 202-214: The env-override branch currently trusts envHost/envPort
without verifying connectivity; add a reachability check (similar to the docker
path's ensure()) after creating unixSocketProxy via UnixDomainSocketProxy.create
and before calling applyRedisEndpoint/returning containerConfig: attempt a quick
Redis PING (or TCP connect) to the proxied endpoint (using unixSocketProxy.path
or host:port), and if it fails, tear down the proxy (clean up unixSocketProxy),
throw or log a clear error like "cannot reach env redis at <host>:<port>" so
tests fail fast; only set dockerStarted=true and return containerConfig after
the connectivity check succeeds.
---
Duplicate comments:
In `@test/js/valkey/test-utils.ts`:
- Line 205: The current fallback sets tlsPort to the non-TLS port causing TLS
tests to hit a non-TLS endpoint; change the logic around tlsPort/envTlsPort so
TLS tests only run when an explicit TLS port is provided: set tlsPort =
envTlsPort ? parseInt(envTlsPort, 10) : 0 (or otherwise throw when running in
env-override mode), and update the TLS test groups to detect tlsPort === 0 and
skip TLS coverage accordingly; reference the tlsPort variable, envTlsPort
(BUN_TEST_REDIS_TLS_PORT) and the TLS test groups to locate where to apply the
change.
🪄 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: d45f9105-b289-4ace-96c9-ef2f616c3489
📒 Files selected for processing (3)
test/harness.tstest/js/bun/s3/s3.test.tstest/js/valkey/test-utils.ts
Lets DB-dependent tests run in environments that have the services reachable but no docker CLI, without touching any test file. ensure(service) now resolves in order: BUN_TEST_SERVICE_<name> env var -> BUN_DOCKER_COORDINATOR socket (existing) -> docker compose. The env value is host[:PORT] for single-port services or host:INT=EXT,INT=EXT for multi-port (redis_unified, minio); host alone defaults each port to itself. The per-service ports/tls/users that were inlined in ensure()'s switch move to a serviceMeta table so the env path builds an identical ServiceInfo. describeWithContainer no longer bails on !isDockerEnabled() before the service is resolvable; it skips only when no env var, no coordinator and no docker. isDockerEnabled() itself is unchanged. down() becomes a no-op when compose was never started.
dac9899 to
b4a3d6f
Compare
alii
changed the title
There was a problem hiding this comment.
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/harness.ts`:
- Line 1009: The conditional `(concurrent && Bun.version !== "1.2.22" ?
describe.concurrent : describe)(label, () => {` contains an unexplained
magic-version check for Bun.version "1.2.22"; either remove the `Bun.version !==
"1.2.22"` clause if the underlying bug is fixed, or retain it but add a clear
comment above this line documenting what specifically fails on Bun 1.2.22 (e.g.,
failing test name/behavior), and include a link to the upstream bug/issue or PR;
update the use sites (the `describe.concurrent`/`describe` selection)
accordingly so future readers understand why the version-specific branch exists
or has been removed.
🪄 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: b0e4193e-573f-4b2c-bd7e-22775c538828
📒 Files selected for processing (2)
test/docker/index.tstest/harness.ts
There was a problem hiding this comment.
LGTM — test-harness-only change; the serviceMeta refactor is a faithful extraction of the old switch and the new env path is opt-in.
Extended reasoning...
Overview
This PR touches only test infrastructure: test/docker/index.ts and test/harness.ts. It (1) extracts the per-service ports/tls/users that were inlined in ensure()'s switch into a serviceMeta table, (2) adds an opt-in BUN_TEST_SERVICE_<name> env-var override that ensure() consults before the existing coordinator/compose paths, (3) tracks composeStarted so down() is a no-op when compose never ran, and (4) reorders describeWithContainer's gating so the env/coordinator checks short-circuit before isDockerEnabled() (which can throw on CI Linux without docker). It also drops the Bun.version !== "1.2.22" guard per the author's inline request.
What changed since my last review
The PR was substantially reworked after my earlier comments. The previous revision's resolveService() / describeWithService / localhost-probe cascade and the per-test-file changes (mysql2, valkey, s3, regression tests) are gone; the current diff is confined to the two harness files and uses a single BUN_TEST_SERVICE_<service> variable instead of separate HOST/PORT vars. All prior inline threads (mine and CodeRabbit's) are resolved and no longer apply to the current shape.
Security risks
None. This is test-harness plumbing — no production runtime code, no auth/crypto/permissions. The env-var parser only affects which host:port test suites connect to, and only when the operator explicitly sets BUN_TEST_SERVICE_*.
Level of scrutiny
Moderate-but-bounded. It's shared test infrastructure, so a regression could mis-gate many DB suites, but the default path is preserved: I cross-checked serviceMeta against the deleted switch entry-by-entry (postgres_{plain,tls,auth}, mysql_{plain,native_password,tls}, redis_{plain,unified}, minio, autobahn, squid) and the ports/tls/users are identical, so the compose path produces the same ServiceInfo as before. The harness diff is mostly de-indentation; the only semantic changes are the gating order (well-commented) and wrapping the unknown-image throw in a describe so it surfaces at test time rather than module-eval time.
Other factors
- The new
serviceFromEnv()parser has explicit, descriptive errors for empty host, malformed port specs, and ambiguous single-port on multi-port services. composeStartedcorrectly preventsdown()from shelling out when only the env/coordinator path was used.- The Buildkite failure in the timeline is an unrelated
clang++ -no-pielinker warning on freebsd/android build-rust jobs, not caused by these test-file edits. - No CODEOWNERS cover these paths.
- Bug-hunter found nothing on the current revision.
2 participants
What
Lets DB-dependent tests run in environments that have the services reachable but no docker CLI, without changing any test file. The fix is entirely in
test/docker/index.tsandtest/harness.ts.dockerCompose.ensure(service)now resolves in order:BUN_TEST_SERVICE_<service>env varBUN_DOCKER_COORDINATORsocket (existing)describeWithContainerno longer bails on!isDockerEnabled()before the service is resolvable; itdescribe.todos only when none of the three paths are available.isDockerEnabled()itself is unchanged.Env var format
BUN_TEST_SERVICE_postgres_plain=127.0.0.1:5432(or just127.0.0.1to use the default port)BUN_TEST_SERVICE_redis_unified=127.0.0.1:6379=6379,6380=6380The per-service ports/tls/users that were inlined in
ensure()'s switch move to aserviceMetatable so the env path produces aServiceInfoshaped identically to the compose path; existing callers (describeWithContainer's port lookup,valkey/test-utils.ts:redisInfo.ports[6380],s3.test.ts:minioInfo.ports[9000]) consume it unchanged.Behavior
down()is a no-op since compose never started.describe.todo.Tests using raw
if (isDockerEnabled()) { ... }(notdescribeWithContainer) keep skipping in dockerless envs as before; this PR doesn't touch them.