Harden the bun DB tests
Nobody has claimed this yet.
- Dominant language
- Jupyter Notebook
- Stars
- 5
- Forks
- 10
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 17
Description
Summary
The live-DB tests decide whether to run by checking that an environment variable is set, not whether a database is actually reachable.
They also check the wrong variable, and they only clean up after themselves on a tidy exit.
On a fresh clone, or in something shaped like prod, that means the tests can run against the wrong database, fail on connect instead of skipping, or leave rows behind.
The fix is the standard one frameworks already use: a dedicated test database chosen by the environment mode, with transactional rollback for isolation.
The problems
-
They check presence, not availability.
All three suites decide whether to run by checking!!process.env.POSTGRES_URL(platform/test/db.test.ts:8,platform/test/auth/client/store.test.ts:16,platform/test/auth/client/commands.test.ts:145). If the URL is set but wrong or unreachable, the suite runs anyway and fails on connect instead of skipping. -
They check the wrong variable.
The check looks atPOSTGRES_URL, but client auth reads the clients DB fromAPOLLO_CLIENTS_DB_URL(and only falls back toPOSTGRES_URL). So an environment shaped like prod, with justAPOLLO_CLIENTS_DB_URLset, would skip these tests even though a clients DB is sitting right there. -
They write to whatever the variable points at, and the cleanup is fragile.
beforeAllrunsrunMigrations()and the tests write rows into the target DB (commands.test.ts:154,store.test.ts:24,db.test.ts:28-29). A developer who pointsPOSTGRES_URLat their own dev database gets it migrated and written to. Cleanup is aDELETE FROM lightning_clients WHERE name LIKE 'client-test-%'inafterAll(commands.test.ts:158,store.test.ts:29), and it only runs on a clean exit, so a killed or crashed run leaves the rows behind.
Why it matters. The only thing standing between a contributor and an accidentally migrated, mutated real database is having POSTGRES_URL set, which is the normal local case. And in the setup we actually want for prod, the tests just quietly skip.
The approach: a standard setup, not a bespoke one
Ecto, Rails, Django etc. all solve these three problems the same way.
Tests run against a dedicated, named test database, and that database is chosen by an explicit environment mode. Tests keep out of each other's way by running inside a transaction that gets rolled back, rather than by deleting rows or building and tearing down whole databases.
CI already does the database half of this. So the work here is to make the local test path do what CI already does, building on the APOLLO_ENV wiring from #555 (see Sequencing below).
What that looks like in practice:
- Point the suites at a dedicated
apollo_testdatabase
They should never run against whateverPOSTGRES_URLorAPOLLO_CLIENTS_DB_URLhappens to hold. Intestmode the database is always the test one, and a real dev or prod database is left untouched even if its URL is set. The environment mode provides this rule and this issue uses it. - Isolate tests with transactional rollback, not row deletion
Wrap each test (or each suite) insql.beginand roll it back. Nothing is ever committed, so there's nothing to clean up afterwards, even if a run is killed. This drops the fragileDELETE ... LIKE 'client-test-%'entirely. There's one exception: the test that deliberately triggers a unique violation (23505) aborts its own transaction, so it either needs a savepoint or has to run outside that wrapped transaction with its own small cleanup. Flag that in the implementation. - Make the skip check a real availability test
Probe the test database with aSELECT 1and skip cleanly when it can't be reached, instead of just checking that an environment variable is set. A URL that's set but wrong or unreachable then skips, rather than failing partway through. - Read the connection from
clientsDbUrl()
(APOLLO_CLIENTS_DB_URL, thenPOSTGRES_URL), so the check matches how the running app picks its database. That fixes the bug where the tests read the wrong variable, whatever isolation approach we settle on.
One thing to watch: getDb() is a process-wide singleton
platform/src/db/index.ts:11 builds one SQL handle from clientsDbUrl() on first use and reuses it. Every caller (runMigrations(), the store and command functions) goes through getDb(), and there's no way to hand it a different connection. Bun also runs all the test files in one process, so they share that single handle and a single process.pid.
That leaves two ways to point the tests at the test database. Either the environment mode resolves the test database before anything calls getDb(), which is cleaner since the mode owns the URL, or we add a way to pass an explicit connection into getDb() and runMigrations().
Creating and dropping a database inside each describe block won't work across the three suites in any case: Postgres has no CREATE DATABASE IF NOT EXISTS, and DROP DATABASE needs the connection pool closed first. If we ever want a throwaway database, it has to be one setup step for the whole run, not one per suite. That's why the earlier "throwaway database per run" idea was dropped in favour of a stable test database.
APOLLO_ENC_KEY doesn't come into this. Each test already makes its own encryption key with randomBytes(32) (for example commands.test.ts:33, store.test.ts:38). Generating a test encryption key is part of the environment modes work, so this issue doesn't need it.
Acceptance criteria
- Live-DB suites in
db.test.ts,auth/client/store.test.ts, andauth/client/commands.test.tsrun against a dedicatedapollo_testdatabase chosen bytestmode (the same database CI uses), and never against a developer's dev or prod database even when its URL is set. - The connection comes from
clientsDbUrl()(APOLLO_CLIENTS_DB_URL, falling back toPOSTGRES_URL), matching how the running app resolves its database, which fixes the bug where the tests read the wrong variable. - The skip check reflects real availability (for example a
SELECT 1probe): a URL that's set but unreachable or wrong skips the suite cleanly rather than failing on connect. - Tests isolate with transactional rollback (nothing committed), so a killed or crashed run leaves no rows behind. The
DELETE ... WHERE name LIKE 'client-test-%'cleanup is removed. - The
23505unique-violation test is handled (a savepoint, or kept outside the wrapped transaction with its own cleanup) and documented. - Depends on the
APOLLO_ENVwork from #555 : this issue uses it rather than re-reading the database source from scatteredprocess.envlookups. ThegetDb()singleton is pointed at the test database before first use (or through an explicit injection point), not by creating and dropping a database perdescribe. -
bun teststill runs offline (no test database reachable), with these suites skipped.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with platform/test/db.test.ts, platform/test/auth/client/store.test.ts, and platform/test/auth/client/commands.test.ts, then trace clientsDbUrl() and the singleton in platform/src/db/index.ts after #555's APOLLO_ENV work. Run bun test with and without a reachable database to verify clean skipping. Done means all three suites use the dedicated apollo_test database, roll back changes including the 23505 case, and no longer delete rows by name.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- bun, postgresql, typescript
- Domain
- databases, testing
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100