OpenFn / OpenFn/apollo

Harden the bun DB tests

Open
#557 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

testing
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
  1. 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.

  2. They check the wrong variable.
    The check looks at POSTGRES_URL, but client auth reads the clients DB from APOLLO_CLIENTS_DB_URL (and only falls back to POSTGRES_URL). So an environment shaped like prod, with just APOLLO_CLIENTS_DB_URL set, would skip these tests even though a clients DB is sitting right there.

  3. They write to whatever the variable points at, and the cleanup is fragile.
    beforeAll runs runMigrations() 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 points POSTGRES_URL at their own dev database gets it migrated and written to. Cleanup is a DELETE FROM lightning_clients WHERE name LIKE 'client-test-%' in afterAll (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_test database
    They should never run against whatever POSTGRES_URL or APOLLO_CLIENTS_DB_URL happens to hold. In test mode 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) in sql.begin and roll it back. Nothing is ever committed, so there's nothing to clean up afterwards, even if a run is killed. This drops the fragile DELETE ... 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 a SELECT 1 and 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, then POSTGRES_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, and auth/client/commands.test.ts run against a dedicated apollo_test database chosen by test mode (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 to POSTGRES_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 1 probe): 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 23505 unique-violation test is handled (a savepoint, or kept outside the wrapped transaction with its own cleanup) and documented.
  • Depends on the APOLLO_ENV work from #555 : this issue uses it rather than re-reading the database source from scattered process.env lookups. The getDb() singleton is pointed at the test database before first use (or through an explicit injection point), not by creating and dropping a database per describe.
  • bun test still runs offline (no test database reachable), with these suites skipped.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.