Make the server test suite provision its own Postgres - #922
Merged
Conversation
~39 of the server's test files open a database in beforeAll, and apps/server/test/db/setup.ts defaults TEST_DATABASE_URL to 127.0.0.1:5433. Nothing in the repo starts a Postgres there, so locally the suite failed 39 suites with ECONNREFUSED while the non-DB tests still reported as passing — a red suite that reads like a product regression but is only a missing container. The only thing making `pnpm run test` work was whatever Postgres happened to be listening on that port, so the signal broke silently whenever that container went away. scripts/server-tests-isolated.sh now provisions a Postgres on a free port and tears it down on exit. A preset TEST_DATABASE_URL still wins untouched, which keeps CI (and anyone pointing the suite at an existing database) on exactly the path it uses today; that branch now echoes which database it picked so a stale exported URL cannot silently reproduce the original failure. Per-run databases also get restart: "no" — the compose default of unless-stopped is right for the long-lived dev database but means a run killed before its teardown leaves a container Docker resurrects on every daemon start. e2e-isolated.sh gets the same treatment; three orphans from earlier e2e runs are currently sitting on this machine. Stale servertest containers older than a day are swept at startup. Also add jobs-add-dialog.test.tsx, covering the create-job flow, which had none: the exact AddJobConfig payload (trimming, minute->ms conversion, schedule trim-to-null, autoArchive inversion, worktree branch gating, toggle passthrough), the effectiveEnabled gate that forces enabled false when the schedule is blank, model reset on agent-type change, canAdd gating, and the submit outcomes (cwd history only on success, submit error shown then cleared). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why
~39 of the server's test files open a database in
beforeAll, andapps/server/test/db/setup.tsdefaultsTEST_DATABASE_URLto127.0.0.1:5433. Nothing in the repo starts a Postgres there, so locally the suite failed 39 suites withECONNREFUSEDwhile the non-DB tests still reported as passing — a red suite that reads like a product regression but is only a missing container.This run hit it:
pnpm run testcame back with 39 failed suites. With a database supplied, the same commit is 149 files / 2528 tests green. The only thing that had been making the documented pre-completion check work locally was whatever Postgres happened to be listening on 5433 (an ambientdispatch-coverage-pgcontainer that exited when the machine last restarted). No doc or script in the repo mentioned it.What changed
scripts/server-tests-isolated.shprovisions a Postgres on a free port via the repo compose file and tears it down on exit.apps/server'stest,test:coverage, andtest:watchgo through it.TEST_DATABASE_URLstill wins untouched — that is the CI path (ci.yml,release.yml, andrelease-signing-smoke.ymlall export it) and the escape hatch for pointing the suite at an existing database. That branch now echoes which database it picked, so a stale exported URL can't silently reproduce the originalECONNREFUSEDwall.docker compose lsrather thandocker compose version, so a stopped daemon gets the same actionable message as a missing plugin instead of a raw daemon error mid-run.Container leak. Per-run databases now get
restart: "no". The compose default ofunless-stoppedis right for the long-lived dev database but means a run killed before its teardown leaves a container Docker resurrects on every daemon start — three orphans from earlier e2e runs are sitting on this machine right now.e2e-isolated.shgets the same treatment, and staledispatch-postgres-servertest-*containers older than a day are swept at startup (scoped to that prefix, so it can never touch a dev database, an e2e run, or a concurrent server-suite run).apps/web/src/components/app/jobs-add-dialog.test.tsx(21 tests) covers the create-job flow, which had none. It renders the real field tree and pins the exactAddJobConfigpayload: trimming, minute→ms conversion, schedule trim-to-null,autoArchive: !keepAgent, worktree branch gating, toggle passthrough; theeffectiveEnabledgate that forcesenabled: falsewhen the schedule is blank even with the switch on; model reset on agent-type change;canAddgating; and submit outcomes (cwd history recorded only on success, submit error shown then cleared).Validation
pnpm run check,pnpm run finalize:web— cleanpnpm run test— server 149 files / 2528 tests, web 56 files / 726 tests, extension 60, scripts 4pnpm run test:e2e— 178 passed, 12 skipped;pnpm run test:e2e:live— 11 passed, zero leaked tmux sessionsTEST_DATABASE_URLpreset → no container provisioned; failing run → exit 1 with clean teardownjobs-add-dialog.tsx: 19/19 killedReview
One review agent inspected the diff: 0 must-fix, 3 should-fix, 5 nits. All three should-fix and 4 nits applied. The notable one:
openAdvanced()was inert — the advanced panel is only collapsed via CSS plusaria-hidden, andgetByLabelText/getByTitleignorearia-hidden, so a broken disclosure would have kept the suite green (proven by a survivingsetAdvancedOpen(false)mutant). Those queries are now role queries, plus a test for the disclosure itself; that mutant now fails 6 tests.🤖 Generated with Claude Code