Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 882cf707cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `CREATE DATABASE ${quotePgIdentifier(database)} TEMPLATE ${quotePgIdentifier( | ||
| migratedTestDatabaseTemplate, | ||
| )}`, | ||
| ); |
There was a problem hiding this comment.
Retry clone when test DB name already exists
This CREATE DATABASE ... TEMPLATE ... path fails immediately on already exists, but test DB names are still randomly generated (prepareTestDB), so long realm-server runs can hit birthday-collision duplicates and now abort instead of recovering. In environments running many tests/modules, this introduces intermittent failures; handle duplicate-name errors by regenerating a name and retrying the clone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This matches our current setup, it can be managed at a later date
Host Test Results 1 files ± 0 1 suites ±0 2h 29m 17s ⏱️ + 55m 27s For more details on these errors, see this check. Results for commit 79ce1ce. ± Comparison against base commit 61ceac3. This pull request removes 1 and adds 18 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
I think we also use a temp DB for the isolated realm server in the matrix tests, no? don't we still need the cleanup test db's script for that? |
There was a problem hiding this comment.
Pull request overview
This PR updates realm-server’s test infrastructure to use an ephemeral Postgres container seeded from a cached, pre-migrated PGDATA snapshot, avoiding per-run cleanup of test databases and reducing test startup time.
Changes:
- Added scripts to build/cache a seeded Postgres data snapshot and to start/stop a dedicated test Postgres container from that snapshot.
- Updated tests to clone per-test databases from a migrated template DB instead of running migrations via
autoMigrate. - Removed the legacy
remove-test-dbs.shcleanup step and updated test scripts to use the new container-based flow.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/realm-server/tests/scripts/wait-for-container-pg.sh | Adds readiness + SQL round-trip checks for Postgres containers. |
| packages/realm-server/tests/scripts/test-pg-config.sh | Centralizes container names, ports, and cache paths for test Postgres. |
| packages/realm-server/tests/scripts/stop-test-pg.sh | Stops/removes the test Postgres container. |
| packages/realm-server/tests/scripts/start-test-pg.sh | Starts the test Postgres container from a seeded PGDATA tar on tmpfs. |
| packages/realm-server/tests/scripts/prepare-test-pg.sh | Computes migration fingerprint; rebuilds seed snapshot if needed; starts test Postgres. |
| packages/realm-server/tests/scripts/create_seeded_db.sh | Builds a seeded DB cluster by running migrations and snapshotting PGDATA. |
| packages/realm-server/tests/scripts/boot_preseeded.sh | Container entrypoint to unpack seeded PGDATA into tmpfs and start Postgres with tuned settings. |
| packages/realm-server/tests/queue-test.ts | Switches to template-cloned DB setup via createTestPgAdapter(). |
| packages/realm-server/tests/helpers/index.ts | Adds template-clone DB creation logic and createTestPgAdapter() helper. |
| packages/realm-server/tests/billing-test.ts | Switches to template-cloned DB setup via createTestPgAdapter(). |
| packages/realm-server/tests/.test-pg-cache/.gitignore | Ensures the seed cache directory contents are ignored by git. |
| packages/realm-server/scripts/remove-test-dbs.sh | Removes the old Docker-based “drop all test_db_*” cleanup script. |
| packages/realm-server/package.json | Updates test scripts to run prepare-test-pg.sh and use the new test Postgres port. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while [ "$attempt" -le "$max_attempts" ]; do | ||
| if cid="$(start_container 2>&1)"; then | ||
| break | ||
| fi |
There was a problem hiding this comment.
cid="$(start_container 2>&1)" merges stderr into the container id on success if Docker prints warnings, and it also means $cid may contain an error string rather than an id. Since $cid is later passed to wait-for-container-pg.sh for docker inspect/logs, a non-id value can cause misleading failures. Capture stdout (container id) separately from stderr, and pass a stable identifier (container name or real id) to the wait/log helpers.
| import { Client as PgClient } from 'pg'; | ||
|
|
||
| const testRealmURL = new URL('http://127.0.0.1:4444/'); | ||
| const testRealmHref = testRealmURL.href; | ||
| const migratedTestDatabaseTemplate = 'boxel_migrated_template'; |
There was a problem hiding this comment.
This file imports pg directly (Client as PgClient). @cardstack/realm-server doesn't list pg / @types/pg as direct deps, so this relies on a transitive dependency from another workspace package, which can break if the dependency graph changes. Add pg (and @types/pg if needed) as a direct devDependency of @cardstack/realm-server, or avoid the direct pg import by exposing this functionality via @cardstack/postgres.
| "test": "./tests/scripts/prepare-test-pg.sh && LOG_LEVELS=\"*=error,prerenderer-chrome=silent,pg-adapter=warn,realm:requests=warn${LOG_LEVELS:+,}${LOG_LEVELS}\" NODE_NO_WARNINGS=1 PGPORT=55436 STRIPE_WEBHOOK_SECRET=stripe-webhook-secret STRIPE_API_KEY=stripe-api-key qunit --require ts-node/register/transpile-only tests/index.ts", | ||
| "test-module": "./tests/scripts/prepare-test-pg.sh && LOG_LEVELS=\"*=error,prerenderer-chrome=silent,pg-adapter=warn,realm:requests=warn${LOG_LEVELS:+,}${LOG_LEVELS}\" NODE_NO_WARNINGS=1 PGPORT=55436 STRIPE_WEBHOOK_SECRET=stripe-webhook-secret STRIPE_API_KEY=stripe-api-key qunit --require ts-node/register/transpile-only --module ${TEST_MODULE} tests/index.ts", |
There was a problem hiding this comment.
The updated test / test-module scripts start (and potentially rebuild) a Postgres container but never stop it after the test run. This can leave a running container and an occupied port on developer machines when tests fail or are interrupted. Consider adding a trap/finally cleanup in the shell script, or chaining a ./tests/scripts/stop-test-pg.sh step (or a posttest script) to ensure the container is removed reliably.
Good point, I notice we never call it from that flow so those dbs would only have been removed when someone ran the realm server tests. I'll either bring it back for now (and move it to the matrix tests) or also have the matrix tests use an ephemeral test postgres - they don't need persistence either (and ideally the whole snapshot/restore side would be pretty good there, particularly later as we add in support for storing snapshots across test runs. |
Instead of storing test databases alongside the development database, this spins up a new container. It will create a template with all the migrations already applied and save this if it doesn't exist (takes a few seconds) and then will boot up a postgres container with the new data.
The immediate impact of this is that tests will no longer need to run a slow "removing test dbs" step, and tests will be a bit faster.
This is an initial step however to using templates as cached pre-seeded databases for much faster realm server tests.