diff --git a/Makefile b/Makefile index eae8b26..c363ecf 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help dev-up dev-up-otel dev-down dev-logs dev-status dev-reset test test-all ci ci-clean e2e-test clean verify-stack create-test-account mobile-full-setup +.PHONY: help dev-up dev-up-otel dev-down dev-logs dev-status dev-reset test test-all test-db-prepare test-audit ci ci-clean e2e-test clean verify-stack create-test-account mobile-full-setup # Default target - show help .DEFAULT_GOAL := help @@ -127,11 +127,20 @@ test: ## Run fast unit/integration tests (skips slow E2E tests) @sleep 3 @echo "$(GREEN)Running migrations on test database...$(RESET)" @goose -dir internal/db/migrations postgres "postgresql://$(POSTGRES_TEST_USER):$(POSTGRES_TEST_PASSWORD)@localhost:$(POSTGRES_TEST_PORT)/$(POSTGRES_TEST_DB)?sslmode=disable" up || true + @# Provisions the template database that testkit.DB clones per test, and + @# sweeps clones orphaned by killed runs. Additive: the goose line above + @# still migrates the shared database the not-yet-migrated tests use. + @./scripts/test-db-prepare.sh @echo "$(GREEN)Running fast tests (use 'make e2e-test' for E2E tests)...$(RESET)" @# -p 1 runs packages sequentially: the integration suite's setup wipes @# shared test-DB tables (unscoped DELETEs), so package-parallel runs race @# and randomly kill other packages' fixtures (jetstream DB tests above all). - @go test -p 1 ./cmd/... ./internal/... ./tests/... -short -v + @# + @# -parallel comes from the server's max_connections, because every test + @# under t.Parallel() holds its own clone pool. Inert until phase 3 enables + @# parallelism; wired now so the ceiling is never discovered by hitting it. + @go test -p 1 -parallel $$(./scripts/test-db-prepare.sh --print-parallel) \ + ./cmd/... ./internal/... ./tests/... -short -v @echo "$(GREEN)✓ Tests complete$(RESET)" e2e-test: ## Run automated E2E tests (requires: make dev-up + make run in another terminal) @@ -177,6 +186,12 @@ test-db-reset: ## Reset test database @goose -dir internal/db/migrations postgres "postgresql://$(POSTGRES_TEST_USER):$(POSTGRES_TEST_PASSWORD)@localhost:$(POSTGRES_TEST_PORT)/$(POSTGRES_TEST_DB)?sslmode=disable" up || true @echo "$(GREEN)✓ Test database reset$(RESET)" +test-db-prepare: ## Create or refresh the template database that testkit.DB clones per test + @./scripts/test-db-prepare.sh + +test-audit: ## Count test-suite invariant violations (warn only; -v for file:line) + @./scripts/test-audit.sh + test-db-stop: ## Stop test database @docker-compose -f docker-compose.dev.yml --env-file .env.dev --profile test stop postgres-test @echo "$(GREEN)✓ Test database stopped$(RESET)" diff --git a/docker-compose.ci.yml b/docker-compose.ci.yml index c2a6a5f..533cff8 100644 --- a/docker-compose.ci.yml +++ b/docker-compose.ci.yml @@ -88,7 +88,13 @@ services: postgres-test: image: postgres:15 network_mode: "service:netns" - command: ["-p", "5434"] + # max_connections is raised from the default 100 because tests/testkit + # clones a database per test and each clone opens its own small pool. The + # ceiling on `go test -parallel` is derived from this number + # (testkit.ParallelBudget), so the two move together instead of the suite + # discovering the limit as "sorry, too many clients already" in whichever + # test happened to be unlucky. + command: ["-p", "5434", "-c", "max_connections=200"] environment: POSTGRES_DB: coves_test POSTGRES_USER: test_user diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index c6d9f5e..646c87a 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -38,6 +38,11 @@ services: postgres-test: image: postgres:15 container_name: coves-test-postgres + # Matches docker-compose.ci.yml: tests/testkit clones a database per test, + # each with its own pool, and testkit.ParallelBudget derives the safe + # `go test -parallel` value from this number. See the CI file for the full + # reasoning. + command: ["-c", "max_connections=200"] ports: - "${POSTGRES_TEST_PORT:-5434}:5432" environment: diff --git a/loop_state.md b/loop_state.md index c84a44e..8efe0f0 100644 --- a/loop_state.md +++ b/loop_state.md @@ -39,7 +39,7 @@ Stop the loop when every task is done, or on any blocked task. |---|------|-------|---|--------|--------|-------| | 1 | Gate green as imported: run `make ci`, fix what surfaces, record baseline timing + allowlist | 0 ⛩ | S | done | (no diff) | GREEN FIRST RUN: 3307 tests, 3288 pass, 0 fail, 19 skips (all allowlisted, 21 entries → 2 unused are ~conditional), 2.2 min WARM caches. No fixes → no review stream | | 2 | Move public-network tests to tests/live/ (+`live` tag, move-only); flip compose nets `internal: true` + module-cache pre-pull; cold-cache egress-blocked `make ci` green | 0 ⛩ | S | done | (see git log) | COLD egress-blocked GREEN 2:21; warm 1:57. 16 funcs → tests/live (4 files + helpers). Egress block found 4 runtime deps, not 1: Turnstile URL hardcoded (→ WithSiteverifyURL, dev-gated env), PLC healthcheck redirected to public web (→ /_health), 3 blueskypost tests dialed public.api.bsky.app (→ blueskyAPI seam), DNS-dependent 404 test (→ DID). Review: Codex "good" + Opus "safe as-is"; 9 fixes applied (unfurl E2E de-mocked via httptest OG, gate vets -tags live, non-dev override warns, prod default pinned, stub 127.0.0.1-only, GOPROXY=off, 404/500 handler tests, golden 200 parse, live cache purge+method asserts). 3281 tests / 14 allowlisted skips | -| 3 | testkit core: db.go (template-clone, advisory lock), wait.go (WaitFor/Holds, terminal errs), fixtures.go (UniqueID run-prefix), scripts/test-db-prepare.sh, scripts/test-audit.sh (warn mode) + testkit's own tests | 1 | S | pending | | dependency rule: testkit imports NO internal/core/* | +| 3 | testkit core: db.go (template-clone, advisory lock), wait.go (WaitFor/Holds, terminal errs), fixtures.go (UniqueID run-prefix), scripts/test-db-prepare.sh, scripts/test-audit.sh (warn mode) + testkit's own tests | 1 | S | done | (see git log) | 47 testkit tests, -race -shuffle clean; clone ~30ms (cheaper than spec est). Review: Codex needs-work (5 high) + Opus not-safe-as-is (2 high) → 15-item batch applied: template-drop name rail (found a panic-in-error-path bug), drop-before-close teardown, 55006 retry, sweep de-FORCEd + error-accumulating, wait-primitive deadline contract, max_connections=200 + ParallelBudget (CI: -parallel 53, inert until ph.3), bounded lock wait w/ holder diagnostics, grep -H audit fix. ADJUDICATION: Codex RIGHT / Opus WRONG on advisory-lock leak (async cancel can grant lock after Go sees error; fixed via Conn.Raw(ErrBadConn) eviction). make ci GREEN 3347 tests / 14 allowlisted skips | | 4 | testkit pds.go (absorb 4 factories, createPDSAccount×2, XRPC clients), firehose.go (generic cursor-gated), appview.go; fix 5 handle-collision sites | 1 ⛩ | S | pending | | then FULL PANEL review of tests/testkit (incl. pragma:security — PDS creds) | | 5 | Kill the lies: delete 6 debt tests; lexicon validator stops generating defs-only subtests (retire 8 allowlist entries); move 2 ratelimit files to internal/api/middleware (T0); fold tests/unit into internal/core/communities | 2 | M | pending | | | | 6 | Split multi-tier files by test func (manifest in commit msg); add build tags in place; retarget Makefile to tags; delete -short/testing.Short(); delete test-all | 2 ⛩ | S | pending | | identity_resolution, bluesky_post (what's left post-task-2), post_unfurl | @@ -105,3 +105,29 @@ Stop the loop when every task is done, or on any blocked task. stage 1b `go vet -tags live` so the live tier can't rot invisibly. - **Docker flake**: containerd "failed to prepare extraction snapshot" can appear once on build; retry clears it — don't chase it as a regression. +- **From task 3 (testkit — later phases MUST know)**: testkit.DB(t) is the + ONLY sanctioned DB path for migrated tests; template `coves_test_template` + guarded by validateTemplateName (won't drop non-test-shaped names). + `sql.DB.Close` WAITS on in-flight queries — always DROP (bounded ctx, + FORCE) before Close in teardown. `Conn.Raw(func(any) error { return + driver.ErrBadConn })` is the ONLY way to evict a session from a + database/sql pool — required on every error path of anything holding + session state (advisory locks, SET LOCAL, temp tables). CREATE DATABASE + ... TEMPLATE can hit SQLSTATE 55006 for a few ms after a template + connection closes (async backend exit) — cloneTemplate retries 3×. + Advisory lock key ('COVE',1) on the maintenance DB (coves_test); template + peeks need EXCLUSIVE (a source connection blocks cloning). goose: testkit + uses NewProvider (no globals); hazard only if a package mixes legacy + goose.Up with testkit in one binary (tasks 7-8 watch for it). + testkit.ParallelBudget wired into both go test invocations via + test-db-prepare.sh --print-parallel (CI computes 53); inert until phase 3 + drops -p 1. Dev postgres-test won't see max_connections=200 until + recreated FROM THE MAIN CHECKOUT (worktree compose project-name mismatch + vs pinned container_name); CI unaffected. Audit baseline 911: sleep 60 + (ph.3-4), skip 280 (ph.2), Short 162 (ph.2), dialer 30 (ph.4), host:port + 288 (ph.3-4), public-hosts 91 (ph.2, ~91 exemption annotations needed — + big three: blueskypost/url_parser_test 19, aggregator_registration_test + 14, jetstream/user_consumer_test 10). Testkit tests deliberately untagged + until task 6. REVIEWER CALIBRATION: on the lock-leak disagreement Codex + was right, Opus wrong (missed the failure path, traced only cancellation) + — weight Codex on DB/concurrency semantics. diff --git a/scripts/ci-runner.sh b/scripts/ci-runner.sh index cd60f05..0d45a21 100755 --- a/scripts/ci-runner.sh +++ b/scripts/ci-runner.sh @@ -74,6 +74,37 @@ go vet -tags live ./tests/live/... echo " ✓ tests/live compiles" echo +# --------------------------------------------------------------------------- +# 1c. Violation audit (advisory) +# --------------------------------------------------------------------------- +# docs/TEST_ARCHITECTURE.md §3.6.3. The suite is mid-migration and every count +# below is scheduled against a phase, so this reports and never judges — the +# `|| true` is belt-and-braces on top of the script's own exit 0. It becomes the +# hard lint gate in the final phase, when the counts are zero. +bash /src/scripts/test-audit.sh || true + +# --------------------------------------------------------------------------- +# 1d. Test template database +# --------------------------------------------------------------------------- +# Provisions the migrated template that testkit.DB clones per test. Done here, +# once, rather than inside the test binaries: `go test` runs packages as +# separate processes and N of them racing to create the same database is a +# built-in flake. +# +# This ADDS a database. The legacy path — tests/integration running goose +# against the shared coves_test database — is untouched and still works. +echo "▶ Preparing the test template database..." +go run ./tests/testkit/cmd/testdbprepare + +# The connection budget, derived from the server's max_connections rather than +# guessed: every test running under t.Parallel() holds its own clone pool. +# Nothing uses t.Parallel() outside tests/testkit yet, so this is inert today — +# it is wired now so that the phase enabling parallelism does not also have to +# discover the ceiling by exhausting it. +TEST_PARALLEL=$(bash /src/scripts/test-db-prepare.sh --print-parallel) +echo " ✓ connection budget allows -parallel $TEST_PARALLEL" +echo + # --------------------------------------------------------------------------- # 2. Run the suite # --------------------------------------------------------------------------- @@ -119,7 +150,7 @@ progress_pid=$! trap 'kill "$progress_pid" 2>/dev/null || true' EXIT set +e -go test -json -p 1 -count=1 -timeout "$TEST_TIMEOUT" \ +go test -json -p 1 -parallel "$TEST_PARALLEL" -count=1 -timeout "$TEST_TIMEOUT" \ ./cmd/... ./internal/... ./tests/... \ >>"$RAW" 2>&1 set -e diff --git a/scripts/test-audit.sh b/scripts/test-audit.sh new file mode 100755 index 0000000..4411a67 --- /dev/null +++ b/scripts/test-audit.sh @@ -0,0 +1,199 @@ +#!/usr/bin/env bash +# Counts test-suite invariant violations. Progress meter first, lint gate second. +# +# WHAT THIS IS FOR +# +# docs/TEST_ARCHITECTURE.md §3.6.3. Every category below is something the +# refactor is removing, and each has a phase that must drive it to zero. Running +# this after every change turns "will the final enforcement flip pass?" into a +# number you can watch instead of a hope. +# +# WARN MODE. This always exits 0. It prints a table and nothing else, so it can +# sit in the CI pipeline from day one without failing builds for violations +# that are scheduled to be fixed three phases from now. The final phase flips it +# to a hard failure, at which point every count must already be zero. +# +# THESE ARE TRIPWIRES, NOT PROOFS. Every check here is a grep, and a grep is +# bypassable by construction — a URL built by concatenation, a bare IP literal, +# a sleep hidden behind a helper. They catch drift cheaply. The actual guarantee +# that tests never reach the public network is the egress-blocked CI network +# (docker-compose.ci.yml, `internal: true`). +# +# SCOPE. "test code" means every *_test.go file under cmd/, internal/ and +# tests/, plus every .go file under tests/ — the shared helpers in +# tests/integration are test code too, and some of the worst offenders live +# there rather than in files ending _test.go. +# +# Whole-line comments are not counted. This tree explains itself at length, and +# a comment that mentions localhost:5434 while describing the stack is not a +# hardcoded endpoint. It is a deliberate undercount at the margin: a violation +# trailing a comment on the same line still counts, because the code is there. +# +# Usage: +# scripts/test-audit.sh summary table +# scripts/test-audit.sh -v table plus every offending file:line +set -uo pipefail + +REPO_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) +cd "$REPO_ROOT" + +VERBOSE=0 +if [[ ${1:-} == "-v" || ${1:-} == "--verbose" ]]; then + VERBOSE=1 +fi + +CYAN='\033[36m' +YELLOW='\033[33m' +GREEN='\033[32m' +RESET='\033[0m' + +# --------------------------------------------------------------------------- +# File scopes +# --------------------------------------------------------------------------- + +test_code_files() { + { + find cmd internal tests -type f -name '*_test.go' + find tests -type f -name '*.go' + } 2>/dev/null | sort -u +} + +all_go_files() { + find cmd internal tests -type f -name '*.go' 2>/dev/null | sort -u +} + +# --------------------------------------------------------------------------- +# Scanning +# --------------------------------------------------------------------------- + +TOTAL=0 +declare -a ROWS=() +declare -a DETAILS=() + +# scan