From 64b7cac3bb91ecc003d9c14797d2d1f21c82bf43 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 19:32:57 +0000 Subject: [PATCH] =?UTF-8?q?Sprint=204=20PR=204.9=20=E2=80=94=20make=20the?= =?UTF-8?q?=20Makefile=20reset=20the=20DB=20the=20tests=20actually=20use?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: `make test-all` — the gate CLAUDE.md tells contributors to run before pushing — was red on main (9 failed / 333 passed at a09e422) on any machine that had run the suite before. Root cause is a path mismatch: - tests/conftest.py sets TESTING_FORCE_MEMORY=true, which makes api/database.py bind to /tmp/signupflow_test.db regardless of DATABASE_URL. - The Makefile defined its test DB as ./test_roster.db and `rm`'d that file between runs — a file the suite never reads. `setup_test_data` seeded it too, via the Makefile's DATABASE_URL. So every "rebuild fresh test database" step was a no-op. Combined with the Makefile's SKIP_TEST_DB_FIXTURES=true (which skips the per-test truncation fixture), /tmp/signupflow_test.db accumulated rows across every run — 518 orgs and 794 people by the time this was diagnosed — until fixed-ID create-tests collided and returned 409 CONFLICT. Failures grew run over run. CI was unaffected because it runs bare `pytest` on a fresh runner and never invokes make, which is why this stayed hidden. Fix: point TEST_DB_PATH at /tmp/signupflow_test.db so the reset and the seed both target the database the suite binds to, and derive every `rm` from that variable instead of hardcoding the stale filename. Changed files: - Makefile (TEST_DB_PATH + test-all/clean recipes) - tests/unit/test_make_test_db_path.py (new, 4 tests) The new tests pin both halves of the contract — that api/database.py redirects under TESTING_FORCE_MEMORY, that conftest sets it, that TEST_DB_PATH equals that path, and that no recipe references test_roster.db — so the two cannot drift apart again. Validation: - `make test-all` from a dirty DB -> unit 350, api 332, cli 16, integration 324; zero failures (was 9 failed / 333 passed). - Ran `make test-all` twice back to back: byte-identical counts, so the accumulation is gone and the target is now idempotent. - pytest tests/unit tests/api tests/cli tests/contract tests/web -> 916 passed, 21 skipped. - black + ruff clean; mypy strict clean (61 files). Follow-ups: - Those create-tests still rely on fixed IDs and are only safe because the DB is reset; switching them to unique per-test IDs would make them robust regardless of DB state. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013v7qGC4EZftRXXxdFcasAN --- Makefile | 12 +++-- tests/unit/test_make_test_db_path.py | 75 ++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_make_test_db_path.py diff --git a/Makefile b/Makefile index 99cc9445..d0cf6ff3 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,13 @@ TEST_SERVER_PORT ?= 8000 TEST_APP_URL ?= http://localhost:$(TEST_SERVER_PORT) TEST_API_BASE ?= $(TEST_APP_URL)/api -TEST_DB_PATH := $(abspath test_roster.db) +# tests/conftest.py sets TESTING_FORCE_MEMORY=true, which makes +# api/database.py bind to /tmp/signupflow_test.db no matter what +# DATABASE_URL says. Point the Makefile at that same file so `rm` and +# `setup_test_data` actually reset the database the suite reads. Using +# ./test_roster.db here meant every reset was a no-op and the real DB +# accumulated rows across runs until fixed-ID tests collided with 409. +TEST_DB_PATH := /tmp/signupflow_test.db TEST_DB_PATH_STRIPPED := $(patsubst /%,%,$(TEST_DB_PATH)) TEST_DB_URL := sqlite:////$(TEST_DB_PATH_STRIPPED) export DATABASE_URL ?= $(TEST_DB_URL) @@ -186,7 +192,7 @@ test-integration: check-poetry test-all: ensure-test-env @echo "🚀 Running complete test suite..." - @rm -f test_roster.db test_roster.db-shm test_roster.db-wal + @rm -f $(TEST_DB_PATH) $(TEST_DB_PATH)-shm $(TEST_DB_PATH)-wal @echo "🔄 Rebuilding fresh SQLite test database..." @poetry run python -m tests.setup_test_data >/dev/null @echo "" @@ -231,7 +237,7 @@ clean: @rm -rf coverage @rm -rf htmlcov @rm -rf .pytest_cache - @rm -rf test_roster.db + @rm -f $(TEST_DB_PATH) $(TEST_DB_PATH)-shm $(TEST_DB_PATH)-wal @rm -rf __pycache__ @find . -type d -name "__pycache__" ! -path "*/.venv/*" -exec rm -rf {} + 2>/dev/null || true @find . -type f -name "*.pyc" ! -path "*/.venv/*" -delete 2>/dev/null || true diff --git a/tests/unit/test_make_test_db_path.py b/tests/unit/test_make_test_db_path.py new file mode 100644 index 00000000..644e5f2e --- /dev/null +++ b/tests/unit/test_make_test_db_path.py @@ -0,0 +1,75 @@ +"""Guard: the Makefile must reset the database the tests actually use. + +`tests/conftest.py` forces `TESTING_FORCE_MEMORY=true`, which makes +`api/database.py` bind to `/tmp/signupflow_test.db` regardless of +`DATABASE_URL`. The Makefile used to define its test DB as +`./test_roster.db` and `rm` that file between runs — a file the suite +never reads. + +Combined with the Makefile's `SKIP_TEST_DB_FIXTURES=true` (which skips the +per-test truncation fixture), nothing ever reset the real database, so +`/tmp/signupflow_test.db` accumulated rows across every run and +fixed-ID create-tests eventually collided with 409 CONFLICT. `make +test-all` was red on any machine that had run the suite before, while CI +— which runs bare `pytest` on a fresh runner — stayed green. + +These tests pin the two halves of that contract so the paths cannot drift +apart again. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +pytestmark = pytest.mark.unit + +REPO_ROOT = Path(__file__).resolve().parents[2] +MAKEFILE = REPO_ROOT / "Makefile" +CONFTEST = REPO_ROOT / "tests" / "conftest.py" + +# The path api/database.py binds to when TESTING_FORCE_MEMORY is set. +FORCED_TEST_DB = "/tmp/signupflow_test.db" + + +def _makefile_var(name: str) -> str: + """Return the literal right-hand side of a `NAME := value` assignment.""" + match = re.search(rf"^{re.escape(name)}\s*:?=\s*(.+)$", MAKEFILE.read_text(), re.MULTILINE) + assert match is not None, f"{name} not found in Makefile" + return match.group(1).strip() + + +def test_database_module_forces_the_tmp_path_under_testing() -> None: + """`api/database.py` redirects to the /tmp DB when TESTING_FORCE_MEMORY is set.""" + source = (REPO_ROOT / "api" / "database.py").read_text() + + assert 'os.getenv("TESTING_FORCE_MEMORY") == "true"' in source + assert FORCED_TEST_DB in source + + +def test_conftest_sets_testing_force_memory() -> None: + """conftest turns that redirect on for every test run.""" + source = CONFTEST.read_text() + + assert 'os.environ["TESTING_FORCE_MEMORY"] = "true"' in source + + +def test_makefile_test_db_path_matches_the_db_tests_use() -> None: + """The Makefile's TEST_DB_PATH must be the DB the suite actually binds to.""" + assert _makefile_var("TEST_DB_PATH") == FORCED_TEST_DB + + +def test_makefile_never_targets_the_unused_roster_db() -> None: + """No Makefile recipe should still be resetting the stale test_roster.db.""" + offenders = [ + line.strip() + for line in MAKEFILE.read_text().splitlines() + if "test_roster.db" in line and not line.lstrip().startswith("#") + ] + + assert offenders == [], ( + "Makefile still references test_roster.db, which the test suite never " + f"reads (it binds to {FORCED_TEST_DB}):\n" + "\n".join(offenders) + )