mirror of
https://github.com/block/buzz.git
synced 2026-10-27 12:40:30 +00:00
## Why PR #6660 introduced four PostgreSQL-backed persistence tests and a focused CI selector, but an exact test-name list does not automatically cover future database tests. Broad ignored-test execution also exposed shared-schema races and ambiguity between desired-state and migration-applied schema expectations. ## What - Establish a discoverable convention: PostgreSQL unit modules use `postgres_tests`, PostgreSQL integration binaries use a `postgres_` prefix, and unrelated external-infrastructure tests use an `external_infra_` prefix. - Add a dedicated nextest PostgreSQL profile and archive-backed CI job covering the relevant crates without enumerating test names. - Create a run-scoped desired-state source database and a unique PostgreSQL database per test process for parallel-safe isolation, with cleanup on success, failure, or interruption. - Route destructive migration and migration-parity tests to clean `template0` databases while desired-state tests clone the desired-state database. - Document discovery, schema modes, required role privileges, and a bounded portable local runner. - Keep the existing infrastructure-free unit-test jobs unchanged. This is intentionally limited to test and CI harness behavior. It does not move production database code, change persistence semantics, or implement issue #20's broader shared test-utility refactor. ## Risk Low production risk because all changes are confined to tests, CI configuration, documentation, and test harness scripts. Remaining operational risks: - Cleanup retries `dropdb --force` five times across roughly four seconds. Exhaustion warns with the database name but deliberately does not mask the test result; individual PostgreSQL diagnostics are suppressed. - The seven-package boundary is duplicated between the runner and CI archive and must remain synchronized if PostgreSQL tests move to a new crate. - Portable SHA fallback branches were exercised on Linux; no macOS Blox workstation was needed for this Linux CI artifact. - GitHub's unchanged generic Unit Tests job currently fails while cold-linking `buzz-voice` because `sherpa-onnx-c-api` is absent. The branch changes no voice/build/toolchain inputs; the exact parent previously passed that job, and Blox passes the same infrastructure-free suite (1,450/1,450). A prior-head one-job retry and the final-head run both reproduced the hosted-runner failure. ## Verification Author Blox workstation `2020088`: - `cargo fmt --all -- --check` - `just clippy` - `just test-unit`: 1,450 passed; PostgreSQL tests remain skipped in fast jobs - PR #6660 focused tests: 4/4 passed through the final runner - Full lane: 282/282 passed in 10.087s with a non-superuser role limited to `CREATEDB`, `CREATEROLE`, and `pg_read_all_stats` - 272 desired-state tests - 10 migration-applied tests - 6 explicitly filtered external-infrastructure tests - Cleanup fault injection: a deliberately underprivileged 282-test run produced the expected 277 passes and 5 failures, and the post-run catalog audit found zero lane databases after retry cleanup. The restored successful run also left zero lane databases. GitHub exact final head `92c231e48f299ea2af23817763f5ae80ac013d68`: - [PostgreSQL Tests](https://github.com/block/buzz/actions/runs/32789030203/job/97630827823): 282/282 passed across 10 binaries in 98.886s; 1,240 skipped, including 6 via the profile filter - The shared relay/PostgreSQL archive predecessor completed successfully - [Unit Tests](https://github.com/block/buzz/actions/runs/32789030203/job/97626930420): unrelated `sherpa-onnx-c-api` native-link failure described above - Independent exact-head review on separate Blox workstation `2022762`: no substantive findings after all initial findings were addressed. The reviewer independently verified cleanup retry behavior, exit-status preservation, shell portability, syntax, and a clean exact-head worktree. ## References - Stacked on #6660 at exact head `561de54be4d9c2b622b7c2aa5b61bc3068f47e2a` - https://github.com/TheSentinel454/buzz/issues/20 - https://github.com/TheSentinel454/buzz/issues/20#issuecomment-5398526792 Generated with Codex. ## Update — August 24, 2026 review follow-up - Centralized the repeated PostgreSQL test URL resolution in crate-local, test-only helpers without introducing the broader shared utility refactor from issue #20. - Restored descriptive hybrid/Redis test function names. Structural `external_infra_*_tests` modules now own exclusion, and the nextest filter only recognizes module path segments. - Added a three-second source guard that scans every Rust file and fails CI when an ignored PostgreSQL test would be omitted or an external-infrastructure test would be included. Fixture tests cover accepted modules/binaries and both failure modes. - Removed the two implementation-plan documents. Performance profiling on Blox workstation `2027352` (same prebuilt archive and cargo-nextest 0.9.143 for every comparison): - Current per-test database model, 8 workers: 10.15–10.40s across three runs. The 283 `createdb` calls and 284 `dropdb` calls consumed 28.85–29.46 aggregate seconds, about 39–40% of aggregate test-process duration. - Reusing one database per worker without cleanup: 6.55–6.84s, about 35% faster, but all three runs failed because global matcher-queue tests inherited a quiesced community from an earlier test; the failing test varied with scheduling. - Reusing workers with `TRUNCATE … RESTART IDENTITY CASCADE`: all 282 tests passed, but total time regressed to 10.47–10.67s. The 265 truncate calls consumed 24.34–25.45 aggregate seconds, so truncation merely replaced most clone/drop cost. - Raising concurrency to 16 workers reduced one isolated-database run to 8.79s, but 24 workers exposed a cluster-global `pg_stat_activity` race. The profile remains at the proven-safe 8 workers. Decision: retain unique per-test databases. Harness cleanup uses `dropdb --if-exists --force`; it drops the database rather than truncating tables or deleting rows. Verification at local branch head `60086c1f6ee34a193c342255a5e8e6293b60e988`: - Discovery inventory: exactly 282 intended tests; all 6 hybrid/Redis tests present and structurally excluded. - PR #6660 focused persistence tests: 4/4 passed. - Full PostgreSQL lane: 282/282 passed three times; 1,240 skipped each run; wall time 9.72–9.96s without profiling shims. - Non-ignored tests with a desired-state database: 1,234/1,234 passed; one mesh-demo timeout from the first run passed immediately in isolation. - `cargo fmt --all -- --check` and all-target/all-feature clippy for `buzz-db`, `buzz-deletion`, and `buzz-relay` passed. Final independent re-review at exact head `42097c0136ab3fa8f0efe5720ae829c04575b9b6` on separate Blox workstation `2028455`: no substantive residual findings. The reviewer independently exercised ordinary strings, zero/one/three-hash raw strings, line/block comments, and string-contained lookalikes, then reran the full 353-file scan, shell syntax, Python AST parsing, and diff check. Independent review found that the source guard's initial regular expression could miss raw-string ignore reasons and treat a commented attribute as real. Final head `42097c0136ab3fa8f0efe5720ae829c04575b9b6` parses valid ordinary/raw Rust string literals only at attributes found in comment-sanitized source; regression fixtures cover both cases. The fixture suite, Python compilation, shell syntax, diff check, and full 353-file repository scan pass on Blox. ## Update — August 25, 2026 restack PR #6660 merged, so this follow-up was rebased from its former exact parent onto current `origin/main` (`8d2d0ff5ad42733e9949442c4b6358d0ba87f9a8`). The final candidate head is `05dcc2ab28948e3ab79bb44839a69f2ba44648a2`; the PR no longer carries #6660's pre-squash history. Fresh exact-head verification on Blox workstation `2028572`: - Discovery guard passed across 353 Rust files; inventory remained exactly 282 intended PostgreSQL tests. - PR #6660 focused persistence tests: 4/4 passed. - Full PostgreSQL lane: 282/282 passed three times; 1,243 skipped each run; wall time 9.70–9.87s. - `cargo fmt --all -- --check` and all-target/all-feature clippy for `buzz-db`, `buzz-deletion`, and `buzz-relay` passed. ## Update — August 25, 2026 final hosted verification - [GitHub PostgreSQL Tests](https://github.com/block/buzz/actions/runs/32801679226/job/97667835080): 282/282 passed in 53.787s; 1,243 skipped. - The final guard scanned all 353 Rust files successfully before the hosted lane ran. - The shared relay/archive prerequisite and infrastructure-free Unit Tests job both passed at exact final head `42097c0136ab3fa8f0efe5720ae829c04575b9b6`. --------- Signed-off-by: Luke Tornquist <tornquist@squareup.com> Signed-off-by: tornquist <tornquist@squareup.com>
93 lines
2.8 KiB
Bash
Executable File
93 lines
2.8 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# Give each nextest process its own PostgreSQL database. The test binary and
|
|
# arguments supplied by nextest must always be executed by this wrapper.
|
|
set -euo pipefail
|
|
|
|
: "${NEXTEST_RUN_ID:?nextest must provide NEXTEST_RUN_ID}"
|
|
: "${NEXTEST_BINARY_ID:?nextest must provide NEXTEST_BINARY_ID}"
|
|
: "${NEXTEST_TEST_NAME:?nextest must provide NEXTEST_TEST_NAME}"
|
|
: "${NEXTEST_ATTEMPT_ID:?nextest must provide NEXTEST_ATTEMPT_ID}"
|
|
: "${BUZZ_POSTGRES_ADMIN_URL:?setup must provide BUZZ_POSTGRES_ADMIN_URL}"
|
|
: "${BUZZ_POSTGRES_DESIRED_TEMPLATE:?setup must provide BUZZ_POSTGRES_DESIRED_TEMPLATE}"
|
|
|
|
resolve_pg_command() {
|
|
local name="$1"
|
|
local candidate
|
|
if [[ -n "${PG_BIN_DIR:-}" ]]; then
|
|
candidate="${PG_BIN_DIR}/${name}"
|
|
else
|
|
candidate="$(command -v "$name" || true)"
|
|
fi
|
|
if [[ -z "$candidate" || ! -x "$candidate" ]]; then
|
|
echo "required PostgreSQL client is not executable: ${candidate:-$name}" >&2
|
|
exit 1
|
|
fi
|
|
printf '%s\n' "$candidate"
|
|
}
|
|
|
|
sha256_hex() {
|
|
if command -v sha256sum >/dev/null 2>&1; then
|
|
sha256sum | awk '{print $1}'
|
|
elif command -v shasum >/dev/null 2>&1; then
|
|
shasum -a 256 | awk '{print $1}'
|
|
elif command -v openssl >/dev/null 2>&1; then
|
|
openssl dgst -sha256 | awk '{print $NF}'
|
|
else
|
|
echo "a SHA-256 utility (sha256sum, shasum, or openssl) is required" >&2
|
|
return 1
|
|
fi
|
|
}
|
|
|
|
createdb="$(resolve_pg_command createdb)"
|
|
dropdb="$(resolve_pg_command dropdb)"
|
|
|
|
identity="${NEXTEST_RUN_ID}:${NEXTEST_BINARY_ID}:${NEXTEST_TEST_NAME}:${NEXTEST_ATTEMPT_ID}"
|
|
database_hash="$(printf '%s' "$identity" | sha256_hex)"
|
|
database_hash="${database_hash:0:24}"
|
|
database="buzz_nt_${database_hash}"
|
|
schema_mode="desired"
|
|
source_database="$BUZZ_POSTGRES_DESIRED_TEMPLATE"
|
|
|
|
# A leading separator lets the same patterns cover root and nested test paths.
|
|
qualified_test_name="::$NEXTEST_TEST_NAME"
|
|
# These tests own the migration lifecycle and intentionally begin empty.
|
|
case "$qualified_test_name" in
|
|
*::migration::postgres_tests::* | *::migration_schema_*)
|
|
schema_mode="migration"
|
|
source_database="template0"
|
|
;;
|
|
esac
|
|
|
|
cleanup() {
|
|
local attempt
|
|
for attempt in 1 2 3 4 5; do
|
|
if "$dropdb" --if-exists --force \
|
|
--maintenance-db="$BUZZ_POSTGRES_ADMIN_URL" \
|
|
"$database" >/dev/null 2>&1; then
|
|
return 0
|
|
fi
|
|
if [[ "$attempt" -lt 5 ]]; then
|
|
sleep 1
|
|
fi
|
|
done
|
|
echo "warning: failed to remove isolated PostgreSQL test database after 5 attempts: $database" >&2
|
|
return 0
|
|
}
|
|
trap cleanup EXIT
|
|
trap 'exit 129' HUP
|
|
trap 'exit 130' INT
|
|
trap 'exit 143' TERM
|
|
|
|
"$createdb" \
|
|
--maintenance-db="$BUZZ_POSTGRES_ADMIN_URL" \
|
|
--template="$source_database" \
|
|
"$database"
|
|
|
|
database_url="${BUZZ_POSTGRES_ADMIN_URL%/*}/$database"
|
|
export DATABASE_URL="$database_url"
|
|
export TEST_DATABASE_URL="$database_url"
|
|
export BUZZ_TEST_DATABASE_URL="$database_url"
|
|
export BUZZ_TEST_SCHEMA_MODE="$schema_mode"
|
|
|
|
"$@"
|