test(sql): use describeWithContainer in sql-prepare-false so it skips on the darwin agent where docker hangs - #33986
Conversation
…s on darwin agents where the docker client hangs sql-prepare-false.test.ts was the only sql test that called dockerCompose.ensure() directly from an async describe without the isDockerEnabled()/describeWithContainer guard. On darwin-x64-mini-1 (added to the test-darwin queue around build 70660) the docker client is on PATH but talking to it hangs, so ensure() blocked for the full 180s runner timeout and the file was killed with nothing but the banner printed. Every other describeWithContainer/isDockerEnabled-guarded sql test on the same agent skips in under 100ms. Also drops the afterAll(dockerCompose.down()) which tore down the whole compose project for later files on the shard.
|
Updated 9:52 AM PT - Jul 11th, 2026
⏳ @robobun, your commit c992b92 is still building in |
WalkthroughChangesSQL prepare-false test harness
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical refactor of a test file onto the shared describeWithContainer harness pattern.
What was reviewed:
- Verified the new usage matches
describeWithContainer's signature intest/harness.ts:1089and the sibling pattern inpostgres-binary-numeric.test.ts(await container.readybefore readinghost/port). - Confirmed
optionswas correctly converted to a thunk socontainer.host/container.portare read afterbeforeAllpopulates them via the live getters. - Confirmed the dropped
afterAll(dockerCompose.down())is intentional and consistent with otherdescribeWithContainercallers, which leave the shared compose project up.
Extended reasoning...
Overview
This PR touches a single test file, test/js/sql/sql-prepare-false.test.ts, converting it from a hand-rolled async describe + dockerCompose.ensure() pattern to the shared describeWithContainer("...", { image: "postgres_plain" }, container => ...) helper that every other container-backed SQL test in test/js/sql/ already uses. The 8 test bodies are unchanged aside from adding await container.ready and calling options() as a thunk instead of referencing a captured object.
Security risks
None. This is a test-infrastructure-only change with no production code, no new inputs, and no auth/crypto surface.
Level of scrutiny
Low. The change is mechanical alignment with an established, widely-used harness helper. I read describeWithContainer in test/harness.ts to confirm: it gates on isDockerEnabled() and falls back to describe.todo (fixing the darwin hang), exposes host/port via live getters populated in beforeAll, and provides a ready promise — all of which the new code uses correctly. The options closure was correctly converted to a thunk so it reads the getters after beforeAll runs, not at describe time when port is still 0.
Other factors
- The removed
afterAll(dockerCompose.down())was actively harmful (tore down the whole compose project for later tests on the same shard); no otherdescribeWithContainercaller does teardown, so removing it is correct. - The PR description includes verified local run output showing all 8 tests still pass against a real postgres.
- No prior reviewer comments to address; timeline only contains the robobun build link.
|
Confirmed on CI: on The only red in the build is |
main added its own pgParseComplete/pgBindComplete/pgParameterDescription and a two-arg pgReadFrontendMessages in #33962; drop the duplicates this PR had introduced (keeping pgCloseComplete and pgReadCString, which main does not have) and rewrite the mock server to handle StartupMessage inline like the other fault-injection fixtures now do. Switch the real-server test to describeWithContainer (harness), matching the shape #33986 moved sql-prepare-false to so the test skips correctly on agents where docker is unavailable.
2 participants
What
Fixes
test/js/sql/sql-prepare-false.test.tsgoing red on darwin x64 CI withtimeout(and occasionallycrash reported) since thedarwin-x64-mini-1agent joined thetest-darwinqueue around build 70660.Seen in builds 70820, 70844, 70855, 70866, 70909, 70964, 70976, 71002, 71019, 71040, 71172, 71187, 71234, 71293, 71443, 71453, 71495, 71701, 71806, 71828, 71934 (always on
darwin-x64-mini-1-1, passes on every other darwin agent).Cause
sql-prepare-false.test.tswas the only file intest/js/sql/that calleddockerCompose.ensure("postgres_plain")directly from an asyncdescribeinstead of going through theisDockerEnabled()/describeWithContainerguard that every other container-backed sql test uses.On
darwin-x64-mini-1the docker client is on PATH but talking to it blocks: the valkey tests'Bun.spawnSync([docker, "info"], {timeout: 5_000})hits its 5s timeout on the same agent, whileisDockerEnabled()(which goes through nodeexecSync) returnsfalsein under 100ms. Soensure()'sBun.spawn(["docker", "version"])/compose uppath hung until the runner's 180s per-file timeout killed the process with nothing printed after thebun testbanner.From build 71293's darwin-x64-mini-1 shard:
while on the same shard every neighboring
describeWithContainer-guarded file skips in under 100ms:In build 71934 the non-zero exit also tripped the runner's crash-report drain, so stale intentional crashes from
run-crash-handler.test.ts(crashByPanic,0xDEADBEEF,outOfMemory) and a bundlernative-plugincrash were attributed to this file and it surfaced ascrash reportedinstead oftimeout. That is the known limitation commented atscripts/runner.node.mjs:1453.Fix
Move the file onto
describeWithContainer("...", { image: "postgres_plain" }, container => ...), the same pattern used bypostgres-binary-numeric.test.ts,postgres-prepared-pipeline-reorder.test.ts,postgres-simple-query-pipeline.test.tsand the other recent postgres tests. That helper already short-circuits on!isDockerEnabled()with adescribe.todo, which is what every other sql test on that agent does today.Also drops the
afterAll(dockerCompose.down())which tore down the whole compose project (all services) for later files on the same shard;describeWithContainerintentionally leaves the shared containers up.Verification
All 8 cases still pass against a real postgres:
#31671 also touches this file for a different reason (making
isDockerEnabled()throw on macOS CI when docker is absent); that change stacks cleanly on top of this one sincedescribeWithContaineralready routes throughisDockerEnabled().The test was added in #27952; the hang was exposed when
darwin-x64-mini-1joined the fleet.[stamp-90s] gate passed · iteration 0 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file