Repository navigation
Sprint 4 PR 4.9 — make the Makefile reset the DB the tests actually use - #250
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013v7qGC4EZftRXXxdFcasAN
tomqwu
marked this pull request as ready for review
August 4, 2026 19:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
make test-all— the gateCLAUDE.mdtells contributors to run before pushing a PR — has been red onmainon any machine that had run the suite before. Measured ata09e422: 9 failed / 333 passed.The cause is a path mismatch, not a flaky test.
The two halves that disagreed
tests/conftest.py:15setsTESTING_FORCE_MEMORY=trueunconditionally, which makesapi/database.py:18-19bind to/tmp/signupflow_test.dbregardless ofDATABASE_URL.TEST_DB_PATH := $(abspath test_roster.db)andrm'd that file between runs — a file the suite never reads.setup_test_dataseeded it too, via the Makefile's exportedDATABASE_URL.So
@echo "🔄 Rebuilding fresh SQLite test database..."was a no-op. Every run.Why that turned into failures
The Makefile also exports
SKIP_TEST_DB_FIXTURES=true, which makesconftestskip the per-test truncation fixture. With truncation skipped and the reset hitting the wrong file, nothing ever cleaned/tmp/signupflow_test.db. It accumulated across every run the repo had ever done — 518 organizations and 794 people by the time I diagnosed it. Tests that create fixed IDs (person_001,avail_test_org1, …) eventually hit rows left by earlier runs and got409 CONFLICT. Failures grew run over run: a partially-dirty DB gave 2 failures, a dirtier one gave 9.CI never noticed because it runs bare
pyteston a fresh runner and never invokesmake.Fix
Point
TEST_DB_PATHat/tmp/signupflow_test.dbso both the reset and the seed target the DB the suite actually binds to, and derive everyrmfrom that variable rather than hardcoding the stale filename.Tests
New
tests/unit/test_make_test_db_path.py(4 tests) pins both halves of the contract so they cannot drift apart again:api/database.pystill redirects to the/tmppath underTESTING_FORCE_MEMORYconfteststill sets that variableTEST_DB_PATHequals that pathtest_roster.dbValidation
make test-all(from a dirty DB)make test-allrun twice back to backpytest tests/unit tests/api tests/cli tests/contract tests/webblack/ruffmypy api/utils api/core api/schemasThe two-consecutive-runs check is the one that matters: before this change the second run was strictly worse than the first, which is the signature of the accumulation.
Follow-ups
Generated by Claude Code