diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e7bf41..d37bf7b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -94,6 +94,35 @@ jobs: name: Integration tests (testcontainers) runs-on: ubuntu-latest needs: [test-unit] + # One Postgres for the whole job, rather than one container per test. + # + # `start_pg` is called from 170 places and used to boot a fresh `postgres:17` + # each time; measured on this workspace, the tests that did so were 86% of + # all test time at 12-16s apiece. The harness now clones a per-test database + # from a migrated template on this server — roughly 190ms — and falls back to + # starting its own container when `ODAL_TEST_PG_ADMIN_URL` is unset, which is + # what a bare `cargo nextest run` still does. + # + # Docker is still needed: the plugin-host suite uses it, and the migration + # tests (`start_pg_before`) deliberately keep starting their own server, + # because a test of a migration needs one the migration has not been applied + # to. + services: + postgres: + image: postgres:17 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: test + POSTGRES_DB: postgres + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 5s + --health-timeout 5s + --health-retries 12 + env: + ODAL_TEST_PG_ADMIN_URL: postgres://postgres:test@127.0.0.1:5432/postgres steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: diff --git a/crates/dpp-dal/src/test_harness.rs b/crates/dpp-dal/src/test_harness.rs index fad4107..d84d8dc 100644 --- a/crates/dpp-dal/src/test_harness.rs +++ b/crates/dpp-dal/src/test_harness.rs @@ -43,6 +43,38 @@ const POSTGRES_IMAGE: (&str, &str) = ("postgres", "17"); /// that window fails, which is why every copy of this harness carried a sleep. const POST_INIT_SETTLE: std::time::Duration = std::time::Duration::from_millis(1500); +/// Point this at a running Postgres and [`start_pg`] will clone a database on +/// it instead of starting a container. +/// +/// # Why this exists +/// +/// [`start_pg`] started a fresh `postgres:17` per call, and it is called from +/// 61 places. Measured across the workspace, **171 tests consumed 86% of all +/// test time** at 12–16s apiece — almost all of it container boot, the settle +/// above, and re-running every migration. +/// +/// The obvious fix — start one container and share it in a `OnceLock` — does +/// not work: **nextest runs each test in its own process**, confirmed by two +/// tests in one binary reporting two PIDs. So the server has to outlive the +/// test process and be found through the environment. +/// +/// Set it to a superuser URL on a database that already exists (`.../postgres` +/// is fine). The harness does the rest, idempotently and safely across +/// concurrent test processes. +/// +/// Unset, everything behaves exactly as before — a bare `cargo nextest run` +/// still works, just slowly. +const SHARED_ADMIN_URL_ENV: &str = "ODAL_TEST_PG_ADMIN_URL"; + +/// The migrated database every per-test database is cloned from. +const TEMPLATE_DB: &str = "odal_test_template"; + +/// Advisory-lock key serialising template creation across test processes. +/// +/// Arbitrary but fixed. Postgres advisory locks are per-cluster, which is +/// exactly the scope needed: many processes, one server, one template. +const TEMPLATE_LOCK_KEY: i64 = 0x0DA1_7E57_0DA1_7E57_u64 as i64; + /// A running Postgres with the app role provisioned and no migrations applied. /// /// The building block. Suites that want the ordinary arrangement should call @@ -67,7 +99,12 @@ pub struct TestPg { pub admin_url: String, /// Application-role URL, for a suite that opens its own pool. pub app_url: String, - _container: ContainerAsync, + /// The container this test owns, when it started one. + /// + /// `None` on the shared-server path: that server outlives the process, so + /// there is nothing here to keep alive. Held only so dropping `TestPg` + /// stops a container the test did start. + _container: Option>, } /// Start Postgres and provision the `odal_app` role. No migrations. @@ -121,6 +158,12 @@ pub async fn start_pg_raw() -> RawPg { /// [`PgDal`] then connects as the app role without re-running them, which /// mirrors the ops workflow. pub async fn start_pg() -> TestPg { + if let Ok(admin_url) = std::env::var(SHARED_ADMIN_URL_ENV) + && !admin_url.trim().is_empty() + { + return clone_from_template(admin_url.trim()).await; + } + let raw = start_pg_raw().await; PgDal::migrate(&raw.admin_url) @@ -133,10 +176,128 @@ pub async fn start_pg() -> TestPg { dal, admin_url: raw.admin_url, app_url: raw.app_url, - _container: raw.container, + _container: Some(raw.container), + } +} + +/// Swap the database name in a Postgres URL. +fn with_database(url: &str, database: &str) -> String { + let base = url.split('?').next().unwrap_or(url); + let trimmed = base.trim_end_matches('/'); + match trimmed.rfind('/') { + Some(i) => format!("{}/{database}", &trimmed[..i]), + None => format!("{trimmed}/{database}"), } } +/// Give this test its own database, cloned from the migrated template. +/// +/// `CREATE DATABASE ... TEMPLATE` is a file copy inside Postgres — measured at +/// roughly 190ms here, against 12–16s to boot a container and migrate it. +async fn clone_from_template(admin_url: &str) -> TestPg { + ensure_template(admin_url).await; + + // Short and unique. Postgres caps identifiers at 63 bytes, and a full UUID + // with its hyphens would need quoting everywhere it appears in a log. + let suffix = uuid::Uuid::now_v7().simple().to_string(); + let db = format!("odal_test_{}", &suffix[..16]); + + let admin = connect_admin(admin_url).await; + // Identifiers cannot be bound as parameters, and this name is built from a + // UUID above rather than from anything a caller supplies. + sqlx::raw_sql(sqlx::AssertSqlSafe(format!( + r#"CREATE DATABASE "{db}" TEMPLATE "{TEMPLATE_DB}""# + ))) + .execute(&admin) + .await + .unwrap_or_else(|e| panic!("clone {TEMPLATE_DB} into {db}: {e}")); + admin.close().await; + + let test_admin_url = with_database(admin_url, &db); + let app_url = app_url_for(&test_admin_url); + let dal = PgDal::connect(&app_url).await.expect("app connect"); + + TestPg { + dal, + admin_url: test_admin_url, + app_url, + _container: None, + } +} + +/// The app-role URL for a database, derived from its superuser URL. +/// +/// The role and password are the harness's own fixed pair, matching what +/// [`start_pg_raw`] provisions — not anything read from the environment. +fn app_url_for(admin_url: &str) -> String { + let after_scheme = admin_url.split("://").nth(1).unwrap_or(admin_url); + let host_and_db = after_scheme + .split_once('@') + .map_or(after_scheme, |(_, rest)| rest); + format!("postgres://odal_app:test@{host_and_db}") +} + +/// Create the `odal_app` role and the migrated template, once per server. +/// +/// Guarded by a Postgres advisory lock rather than a process-local `Once`, +/// because the racing parties are separate processes: nextest runs every test +/// in its own. The first to take the lock builds the template and the rest wait +/// and then find it already there. +async fn ensure_template(admin_url: &str) { + let admin = connect_admin(admin_url).await; + + sqlx::query("SELECT pg_advisory_lock($1)") + .bind(TEMPLATE_LOCK_KEY) + .execute(&admin) + .await + .expect("take template lock"); + + let exists: Option = sqlx::query_scalar("SELECT 1 FROM pg_database WHERE datname = $1") + .bind(TEMPLATE_DB) + .fetch_optional(&admin) + .await + .expect("look up template"); + + if exists.is_none() { + // Provisioned exactly as `ops/bootstrap/pg-init.sh` does, so a suite + // meets the same privilege boundary a deployed node does. Ignore the + // duplicate error: the role is cluster-wide and may predate us. + let _ = sqlx::query("CREATE ROLE odal_app LOGIN PASSWORD 'test'") + .execute(&admin) + .await; + + sqlx::raw_sql(sqlx::AssertSqlSafe(format!( + r#"CREATE DATABASE "{TEMPLATE_DB}""# + ))) + .execute(&admin) + .await + .expect("create template database"); + + // Migrate through its own pool, then close it. `CREATE DATABASE ... + // TEMPLATE` refuses while anything is connected to the template, so + // leaving this pool open would break every clone that follows. + let template_url = with_database(admin_url, TEMPLATE_DB); + PgDal::migrate(&template_url) + .await + .expect("migrate template database"); + } + + sqlx::query("SELECT pg_advisory_unlock($1)") + .bind(TEMPLATE_LOCK_KEY) + .execute(&admin) + .await + .expect("release template lock"); + admin.close().await; +} + +async fn connect_admin(url: &str) -> sqlx::PgPool { + sqlx::postgres::PgPoolOptions::new() + .max_connections(1) + .connect(url) + .await + .unwrap_or_else(|e| panic!("admin connect to {url}: {e}")) +} + /// Start Postgres and apply migrations in order, stopping **before** the first /// whose filename begins with `stop_before`. /// diff --git a/crates/dpp-vault/tests/helpers/mod.rs b/crates/dpp-vault/tests/helpers/mod.rs index d7d5310..9044583 100644 --- a/crates/dpp-vault/tests/helpers/mod.rs +++ b/crates/dpp-vault/tests/helpers/mod.rs @@ -11,16 +11,11 @@ use std::sync::Arc; use async_trait::async_trait; use base64::Engine; -use testcontainers::{ - GenericImage, ImageExt, - core::{WaitFor, ports::ContainerPort}, - runners::AsyncRunner, -}; use dpp_dal::pg::{ PgApiKeyRepo, PgAuditRepo, PgDal, PgEvidenceDossierRepo, PgOperatorConfigRepo, PgPassportRepo, PgRegistryIdentityRepo, PgRegistrySyncRepo, PgRegistryTransferRepo, PgScanTelemetryRepo, - PgSealOutboxRepo, PgWebhookRepo, sqlx, + PgSealOutboxRepo, PgWebhookRepo, }; use dpp_domain::{ DppError, GhostArchive, GhostRegistrySync, @@ -106,55 +101,33 @@ impl AuthProvider for TestAuthProvider { // PostgreSQL container // --------------------------------------------------------------------------- -/// A running PostgreSQL testcontainer together with an app-role PgDal ready for use. +/// A database ready for use, and whatever is keeping it alive. +/// +/// The name is kept because 109 call sites across 27 files use it. What it +/// holds is no longer necessarily a container — see [`start_postgres`]. pub struct PgContainer { pub dal: PgDal, - _container: testcontainers::ContainerAsync, + /// Held so the database outlives the test. On the shared-server path this + /// owns nothing; on the fallback path it owns the container. + _pg: dpp_dal::test_harness::TestPg, } -/// Start a fresh postgres:17 container, provision the `odal_app` role, run -/// migrations, and return an app-role `PgDal`. +/// A migrated database with an app-role `PgDal` connected to it. +/// +/// Delegates to the shared harness. This used to start its own `postgres:17` +/// per call — a **ninth** copy of a harness that had already been consolidated, +/// missed by `just harness-check` only because it was spelled `start_postgres` +/// rather than `start_pg`. That gate now matches on starting a Postgres +/// container at all, not on a function name. +/// +/// The cost was not small: these 109 call sites were the bulk of the 171 tests +/// that consumed 86% of all test time, at 12-16s each. Through the shared +/// harness the same tests take one to two seconds. pub async fn start_postgres() -> PgContainer { - let image = GenericImage::new("postgres", "17") - .with_exposed_port(ContainerPort::Tcp(5432)) - .with_wait_for(WaitFor::message_on_stderr( - "database system is ready to accept connections", - )) - // POSTGRES_USER/PASSWORD/DB are the official Postgres image's required - // env vars for this throwaway testcontainer — NOT the app's - // DATABASE_POSTGRES_PASS / DATABASE_APP_PASS scheme. - .with_env_var("POSTGRES_USER", "postgres") - .with_env_var("POSTGRES_PASSWORD", "test") - .with_env_var("POSTGRES_DB", "odal"); - - let container = image.start().await.expect("start postgres container"); - let port = container - .get_host_port_ipv4(5432) - .await - .expect("mapped port"); - let admin_url = format!("postgres://postgres:test@127.0.0.1:{port}/odal"); - - // PG restarts once during init — give it a moment. - tokio::time::sleep(std::time::Duration::from_millis(1500)).await; - - let admin = sqlx::postgres::PgPoolOptions::new() - .max_connections(1) - .connect(&admin_url) - .await - .expect("admin connect"); - sqlx::query("CREATE ROLE odal_app LOGIN PASSWORD 'test'") - .execute(&admin) - .await - .expect("create app role"); - - PgDal::migrate(&admin_url).await.expect("apply migrations"); - - let app_url = format!("postgres://odal_app:test@127.0.0.1:{port}/odal"); - let dal = PgDal::connect(&app_url).await.expect("app connect"); - + let pg = dpp_dal::test_harness::start_pg().await; PgContainer { - dal, - _container: container, + dal: pg.dal.clone(), + _pg: pg, } } diff --git a/justfile b/justfile index 9316164..be61ca6 100644 --- a/justfile +++ b/justfile @@ -57,7 +57,20 @@ check-integration: bash scripts/check-integration.sh # Run the Docker-backed integration tiers (dal, vault, plugin-host, node) +# +# One shared Postgres for the whole run rather than one container per test. +# The 171 tests that started their own were 86% of all test time; the harness +# now clones a per-test database from a migrated template on this server +# instead, at roughly 190ms against 12-16s. +# +# `just test-integration-isolated` keeps the old container-per-test behaviour, +# which is what a bare `cargo nextest run` still does. test-integration: + bash scripts/shared-test-pg.sh just _test-integration-tiers + +# The tiers themselves. Runs against whatever `ODAL_TEST_PG_ADMIN_URL` names, +# or container-per-test when it is unset. +_test-integration-tiers: #!/usr/bin/env bash set -euo pipefail cargo nextest run -p dpp-dal --features integration-tests @@ -450,3 +463,11 @@ harness-check: # attributable to one crate. Run `just check` before pushing regardless. test-changed BASE="origin/main": bash scripts/test-changed.sh {{ BASE }} + +# The integration tiers with a container per test — the pre-sharing behaviour. +# +# Kept because it is the only arrangement that proves the fallback path still +# works, and because a suspected cross-test interaction is worth re-running +# under full isolation before believing it. +test-integration-isolated: + just _test-integration-tiers diff --git a/scripts/harness-check.sh b/scripts/harness-check.sh index 84ef0c8..f9d9ad3 100644 --- a/scripts/harness-check.sh +++ b/scripts/harness-check.sh @@ -16,8 +16,14 @@ set -euo pipefail # "||" +# +# The Postgres rule matches on **starting a container**, not on a function name. +# Naming it `^async fn start_pg` let a ninth copy sit in +# `dpp-vault/tests/helpers/mod.rs` for the length of the consolidation, spelled +# `start_postgres` — 109 call sites and the bulk of the workspace's slowest +# tests, invisible to a gate written to catch exactly the copy already found. rules=( - "crates/dpp-dal/src/test_harness.rs|^async fn start_pg|use dpp_dal::test_harness::{start_pg, start_pg_raw, start_pg_before}" + "crates/dpp-dal/src/test_harness.rs|GenericImage::new\\(\"postgres\"|use dpp_dal::test_harness::{start_pg, start_pg_raw, start_pg_before}" "crates/dpp-dal/src/in_memory_repo.rs|^impl PassportRepository for InMemoryPassportRepo|use dpp_dal::in_memory_repo::InMemoryPassportRepo" ) diff --git a/scripts/shared-test-pg.sh b/scripts/shared-test-pg.sh new file mode 100644 index 0000000..9fe97cd --- /dev/null +++ b/scripts/shared-test-pg.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# Run a command against one shared Postgres instead of a container per test. +# +# `start_pg` used to boot a fresh `postgres:17` on every call, from 170 call +# sites. Measured on this workspace, the 171 tests that did so consumed **86% of +# all test time** at 12-16s apiece — nearly all of it container boot, a 1500ms +# settle, and re-running every migration for a schema that never varies. +# +# Sharing one server in-process is not possible: nextest runs each test in its +# own process, so the server has to outlive the process and be discovered +# through the environment. This starts it, points `ODAL_TEST_PG_ADMIN_URL` at +# it, and removes it afterwards however the command exits. +# +# The harness clones a per-test database from a migrated template on that +# server, so tests stay isolated. Measured: ~190ms per clone against 12-16s per +# container. +# +# Suites that must control which migrations run — `start_pg_before` — still +# start their own container, and should: a test of a migration needs a server +# the migration has not been applied to. +# +# Usage: bash scripts/shared-test-pg.sh +set -euo pipefail + +container="odal-shared-test-pg-$$" +port="${ODAL_TEST_PG_PORT:-55432}" + +cleanup() { + docker rm -f "$container" > /dev/null 2>&1 || true +} +trap cleanup EXIT + +echo "shared-test-pg: starting $container on port $port" +MSYS_NO_PATHCONV=1 docker run -d --rm \ + --name "$container" \ + -e POSTGRES_USER=postgres \ + -e POSTGRES_PASSWORD=test \ + -e POSTGRES_DB=postgres \ + -p "$port:5432" \ + postgres:17 > /dev/null + +# Poll rather than sleep a fixed interval: the fixed 1500ms every copy of the +# old harness carried was a guess that had to be right on the slowest machine +# and was wasted on every other one. +ready=false +for _ in $(seq 1 60); do + if MSYS_NO_PATHCONV=1 docker exec "$container" pg_isready -U postgres > /dev/null 2>&1; then + ready=true + break + fi + sleep 1 +done + +if [ "$ready" != true ]; then + echo "shared-test-pg: postgres did not become ready" >&2 + exit 1 +fi + +export ODAL_TEST_PG_ADMIN_URL="postgres://postgres:test@127.0.0.1:$port/postgres" +echo "shared-test-pg: ODAL_TEST_PG_ADMIN_URL is set; running: $*" + +"$@"