mirror of
https://github.com/block/buzz.git
synced 2026-11-06 01:11:33 +00:00
b7b545f090
## Summary `buzz-db` tests that create their own databases or roles dropped them as the last line of the test body. A failed assertion or a timeout skipped that line and left the database or role on the shared Postgres server. Seven files carried their own copy of `create_scratch_db_through` and its drop helper. - Adds `ScratchDb` and `ScratchRole` guards to `crates/buzz-db/src/test_support.rs`. Their `Drop` runs `DROP DATABASE IF EXISTS ... WITH (FORCE)` or `DROP ROLE IF EXISTS` on a fresh admin connection, on a separate thread with its own runtime. `ScratchSchema` already works this way, so the guards work under both runtime flavours and while a panicking test unwinds. `WITH (FORCE)` ends any sessions still open on the database, so pools need not be closed first. - A failed drop prints a warning instead of panicking, because a panic during unwinding would abort the test binary. - The seven per-file helpers and every end-of-test `DROP DATABASE` in `buzz-db` now use the guard. The replica-fence masked-activity test's least-privilege role uses `ScratchRole`. Admin pools and `close()` calls that only served cleanup are removed. Test behaviour is otherwise unchanged. - `crates/buzz-db/TESTING.md` now points new tests at the guards. - The second commit fixes a comment in `armed_pool_rejects_old_channel_inserts_through_public_api` that still described the old end-of-test drop. **Existing leftovers need dropping by hand.** This change stops new leftovers but does not remove the ones earlier failed runs already left in dev `buzz-postgres` containers. There were 24 on the author's machine, named `stall_r_*`, `buzz_admin_*`, `floor_probe_*`, `fence_*` and `cxl_*`. List them, check the list, then drop each one: ```sql SELECT datname FROM pg_database WHERE datname ~ '^(stall_r|buzz_admin|floor_probe|fence|cxl)_'; DROP DATABASE "<name>" WITH (FORCE); ``` The guards are private to `buzz-db`. `buzz-push-gateway`, `buzz-admin` and `buzz-relay` tests still create and drop scratch databases by hand, which could get a shared copy of the guards in a follow-up. ### Related issue None found. This continues the test-cleanup work in #8727. ### Testing - Ran the 70 affected `buzz-db` tests against the local `buzz-postgres` container: 68 passed. Neither failure comes from this change. `cluster_global_probe_commits_tokens_and_sessions_prove_coverage` hit the known `MaskedActivity { masked: 1 }` flake under load and passed when run alone. Because it panicked while holding a `ScratchDb`, it also showed a panicking test leaves nothing behind. `serving_lease_statement_timeout_cancels_slow_statement` failed with `VersionMismatch(56)`, because the shared dev database already has another branch's migration 56 applied. - After the run, the server's database and role lists matched the snapshot taken before it. - A temporary test panicked while holding an open transaction, an exclusive table lock and a live session as the scratch role. It left no database or role behind under either runtime flavour and printed no drop warnings. That test is not part of this PR. - `cargo fmt --check` and `cargo clippy -p buzz-db --all-targets -- -D warnings` pass. The tests ran before the commits were cherry-picked onto current main. Only clippy was re-run afterwards. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
79 lines
3.9 KiB
Markdown
79 lines
3.9 KiB
Markdown
# PostgreSQL-backed tests in buzz-db
|
|
|
|
The dedicated PostgreSQL CI lane discovers tests and Cargo packages by
|
|
structure rather than by exact lists. Follow this checklist so a new database
|
|
test is run automatically and remains safe under parallel execution.
|
|
|
|
## Adding a test
|
|
|
|
1. Put the test in a module whose name ends in `postgres_tests`.
|
|
2. Mark it `#[ignore = "requires Postgres"]` so infrastructure-free unit-test
|
|
jobs stay fast.
|
|
3. Connect through `crate::test_support::database_url()`. The CI wrapper sets
|
|
this helper's environment to a unique database for each test process; never
|
|
hard-code the shared development database.
|
|
4. Keep tests that need infrastructure beyond PostgreSQL and Redis in an
|
|
`external_infra*_tests` module. The PostgreSQL lane excludes those tests.
|
|
5. Run `scripts/test-postgres-test-discovery.sh` after adding or moving the
|
|
test. The same guard runs in CI immediately after changed-path detection.
|
|
|
|
The wrapper isolates destructive tests by dropping the entire per-test
|
|
database after the process exits. It does not `DELETE` rows or `TRUNCATE`
|
|
shared tables, so tests may run concurrently without coordinating cleanup.
|
|
|
|
The wrapper drops only that per-test database. A test that needs another
|
|
database or role on the server creates it with `test_support::ScratchDb` or
|
|
`test_support::ScratchRole`, whose `Drop` removes it even when the test
|
|
panics. Do not end a test with a manual `DROP DATABASE` or `DROP ROLE`: a
|
|
failed assertion skips it and leaves the object behind.
|
|
|
|
## Choose the schema intentionally
|
|
|
|
Most tests use the committed desired-state schema from `schema/schema.sql`,
|
|
a manifest that includes one `schema/tables/public/<table>.sql` per table plus
|
|
its types, functions, partitions, triggers, and seed rows.
|
|
That is the default and is appropriate for data-access behavior.
|
|
|
|
Tests in `migration::postgres_tests` receive an empty database and own the
|
|
embedded migration lifecycle. A test outside that module that intentionally
|
|
depends on migration-created triggers or seed rows must prefix its function
|
|
name with `migration_schema_`; it also receives an empty database with
|
|
`BUZZ_TEST_SCHEMA_MODE=migration`.
|
|
|
|
Helpers that normally run migrations honor `BUZZ_TEST_SCHEMA_MODE=desired` in
|
|
the default lane. Do not rerun migrations against a desired-state database.
|
|
When behavior should match in both schema paths, add explicit desired-state and
|
|
migration-applied coverage rather than making the bootstrap implicit.
|
|
|
|
Tests that inspect cluster-wide PostgreSQL state or open least-privilege
|
|
sessions include `cluster_global_` in the function name. Migration-backed cases
|
|
use `migration_schema_cluster_global_`. Nextest serializes this small group
|
|
because separate databases still share `pg_stat_activity` and roles.
|
|
|
|
## Run the lane locally
|
|
|
|
Start native PostgreSQL and Redis, activate Hermit, and run:
|
|
|
|
```bash
|
|
. ./bin/activate-hermit
|
|
scripts/test-postgres-test-discovery.sh
|
|
scripts/postgres-test-run.sh
|
|
```
|
|
|
|
Set `BUZZ_POSTGRES_ADMIN_URL` to a PostgreSQL maintenance database owned by a
|
|
role that can create and drop databases. Set `PGHOST`, `PGPORT`, `PGUSER`, and
|
|
`PGPASSWORD` for desired-state bootstrap, plus `REDIS_URL` for Redis-backed
|
|
tests. The complete privilege-boundary inventory also needs `CREATEROLE` and
|
|
membership in `pg_read_all_stats`, or an ephemeral superuser as CI uses.
|
|
|
|
The runner creates one desired-state source database per invocation and clones
|
|
it for ordinary tests. Migration-mode tests start empty. Cleanup retries
|
|
transient disconnect races before reporting a warning.
|
|
|
|
Invite upgrade tests start from the complete schema through migration 0074,
|
|
including channel join roles, entity deletion approval, the unlisted channel setting and the user deletion target and fences. They exercise the
|
|
invitation schema expansion (migration 0075) through both migration and
|
|
desired-state paths against populated
|
|
data, retaining a non-default channel join role. Historical repair is validated
|
|
separately from the application/schema change.
|