Skip to content

ci: run backend tests against Postgres, not only in-memory SQLite - #154

Open
lucastononro wants to merge 2 commits into
fix/116-coverage-gatefrom
fix/117-postgres-ci
Open

ci: run backend tests against Postgres, not only in-memory SQLite#154
lucastononro wants to merge 2 commits into
fix/116-coverage-gatefrom
fix/117-postgres-ci

Conversation

@lucastononro

@lucastononro lucastononro commented Jul 19, 2026

Copy link
Copy Markdown
Owner

Summary

  • backend/tests/conftest.py unconditionally forced DATABASE_URL=sqlite+aiosqlite://, while production runs postgresql+asyncpg. That hid real divergences: models.py uses Column(JSON) in ~9 places (SQLite vs Postgres JSON semantics differ), the FK ON DELETE CASCADE behavior, and the hand-rolled db._run_migrations were never exercised against the engine we actually ship.
  • backend-test now runs a real postgres:16-alpine service container and points the suite at it.

Changes

  • .github/workflows/ci.yml (backend-test): add a postgres:16-alpine services: container (health-checked via pg_isready) and a job-level TEST_DATABASE_URL pointing at it.
  • backend/tests/conftest.py: respect TEST_DATABASE_URL (highest priority — this is what CI sets), then DATABASE_URL if already set, else fall back to the existing in-memory SQLite default for local dev. No behavior change for anyone not setting either var.
  • backend/pytest.ini (new): pins asyncio_default_fixture_loop_scope / asyncio_default_test_loop_scope to session. This was a necessary correctness fix, not just plumbing — without it, pytest-asyncio 1.x's default per-test event loop silently works with aiosqlite but breaks the module-level asyncpg engine with RuntimeError: ... got Future <Future ...> attached to a different loop as soon as a second test needs the DB (asyncpg connections are strictly bound to the loop that created them; the engine is created once at import time). Session-scoping the loop keeps every test on the same loop as the engine, for both SQLite and Postgres.

Test plan

Existing

  • Full suite against the existing SQLite default (unchanged): 296 passed, 8 skipped.

New

  • Full suite against a real dockerized postgres:16-alpine via TEST_DATABASE_URL: 296 passed, 8 skipped — identical pass count to SQLite. The 8 skips are the pre-existing RUN_LLM_E2E-gated live-provider tests in test_providers_e2e.py, unrelated to the database backend; nothing had to be newly skipped or marked to keep Postgres green.
  • Confirmed the coverage gate from Add coverage measurement and a coverage gate to CI #116 still passes unchanged on top of this (65.81% ≥ 60% floor).

Caveats

  • No test needed skipping/marking for Postgres-only reasons — the suite was already engine-agnostic once the event-loop scoping bug was fixed.
  • CI now always runs against Postgres (matching prod); the SQLite fallback in conftest.py remains for fast local iteration when TEST_DATABASE_URL/DATABASE_URL aren't set.

Closes #117

🤖 Generated with Claude Code

Greptile Summary

This PR wires the backend-test CI job to a real postgres:16-alpine service container so the test suite exercises the same engine that ships in production, and fixes a previously hidden correctness bug where pytest-asyncio's per-function event loop silently worked with aiosqlite but would raise RuntimeError: got Future attached to a different loop the moment asyncpg reused its pooled connections across tests.

  • CI (ci.yml): adds a health-checked postgres:16-alpine service and sets TEST_DATABASE_URL so the job points at it automatically.
  • backend/pytest.ini (new): pins asyncio_mode = strict and both loop-scope options to session, which is the correct pytest-asyncio 1.x replacement for the old event_loop fixture override.
  • backend/tests/conftest.py: replaces the hard-coded SQLite assignment with an explicit TEST_DATABASE_URL-wins / SQLite-fallback guard, closing the previous concern about ambient DATABASE_URL env vars pointing at developer databases; adds a regression test to lock this behaviour.

Confidence Score: 5/5

Safe to merge — the only finding is a dead pre-existing fixture that pytest-asyncio 1.x silently ignores, causing no runtime harm.

The DATABASE_URL guard in conftest.py is correct and directly addresses the previous review concern; the pytest.ini loop-scope settings are the proper pytest-asyncio 1.x mechanism; the CI Postgres service is health-checked and the credentials are appropriate for an ephemeral test container. The one cleanup item (dead event_loop fixture) is harmless.

backend/tests/conftest.py — the pre-existing event_loop fixture at lines 71-75 is now dead code and can be removed.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Adds a postgres:16-alpine service container with health checks and sets TEST_DATABASE_URL for the backend-test job; setup is correct and well-structured.
backend/pytest.ini New file pinning asyncio_mode=strict and both loop-scope options to "session"; valid pytest-asyncio 1.x configuration that replaces the old event_loop fixture approach.
backend/tests/conftest.py DATABASE_URL logic now correctly honours only TEST_DATABASE_URL (falling back to SQLite), addressing the previous concern about ambient env vars; custom event_loop fixture (pre-existing, lines 71-75) is now dead code in pytest-asyncio 1.x.
backend/tests/test_conftest_db_url.py Adds a regression test verifying DATABASE_URL is always either TEST_DATABASE_URL or the SQLite default after conftest import.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pytest starts] --> B{TEST_DATABASE_URL set?}
    B -- Yes CI path --> C[os.environ DATABASE_URL = TEST_DATABASE_URL]
    B -- No local dev path --> D[os.environ DATABASE_URL = sqlite+aiosqlite://]
    C --> E[SQLAlchemy async engine against Postgres]
    D --> F[SQLAlchemy async engine against in-memory SQLite]
    E --> G[pytest.ini: asyncio_default_test_loop_scope = session]
    F --> G
    G --> H[setup_db fixture: create_all / drop_all per test]
    H --> I[Tests run on single session-scoped event loop]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[pytest starts] --> B{TEST_DATABASE_URL set?}
    B -- Yes CI path --> C[os.environ DATABASE_URL = TEST_DATABASE_URL]
    B -- No local dev path --> D[os.environ DATABASE_URL = sqlite+aiosqlite://]
    C --> E[SQLAlchemy async engine against Postgres]
    D --> F[SQLAlchemy async engine against in-memory SQLite]
    E --> G[pytest.ini: asyncio_default_test_loop_scope = session]
    F --> G
    G --> H[setup_db fixture: create_all / drop_all per test]
    H --> I[Tests run on single session-scoped event loop]
Loading

Reviews (2): Last reviewed commit: "fix(review): address Greptile findings o..." | Re-trigger Greptile

backend-test now spins up a postgres:16-alpine service container and
points TEST_DATABASE_URL at it; conftest.py respects TEST_DATABASE_URL
(falling back to DATABASE_URL, then the sqlite default for local dev)
instead of unconditionally forcing sqlite+aiosqlite. This exercises the
Column(JSON) fields, FK ON DELETE CASCADE behavior, and the hand-rolled
db._run_migrations against the engine we actually ship.

Also pins asyncio_default_fixture_loop_scope/asyncio_default_test_loop_scope
to "session" in a new backend/pytest.ini. Without it, pytest-asyncio 1.x's
per-test event loop (fine for aiosqlite) breaks the module-level asyncpg
engine with "Future attached to a different loop" once a second test needs
the DB. Pinning both to session fixes this for both SQLite and Postgres.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wp66uSq9saSpRq9Bbmjxj4
Comment thread backend/tests/conftest.py
P1 (conftest.py:34): the middle `elif` branch honored an ambient
DATABASE_URL from the developer's shell; combined with setup_db's
drop_all teardown that could destroy a real dev database on `pytest`.
Now only the explicit TEST_DATABASE_URL opt-in overrides; everything
else falls back to in-memory SQLite (as before this PR). CI is
unaffected — it sets TEST_DATABASE_URL.

Adds tests/test_conftest_db_url.py pinning that guarantee.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Wp66uSq9saSpRq9Bbmjxj4
@lucastononro

Copy link
Copy Markdown
Owner Author

Greptile review findings addressed (de197b8)

Fixed: 1 / 1

P1 — backend/tests/conftest.py:34 (ambient DATABASE_URL could be dropped by tests) — FIXED

Valid finding. The middle elif branch silently adopted whatever DATABASE_URL was already in the shell, and the setup_db fixture runs Base.metadata.drop_all after every test — a developer with DATABASE_URL pointing at a real dev database would have it wiped by pytest. Applied the suggested fix: only the explicit TEST_DATABASE_URL opt-in is honored; everything else falls back to in-memory SQLite (the pre-PR behavior). CI is unaffected since it sets TEST_DATABASE_URL.

Tests

  • Added backend/tests/test_conftest_db_url.py pinning the guarantee: after conftest import, DATABASE_URL is either TEST_DATABASE_URL or the SQLite default — never an ambient shell value. Verified it passes under all three scenarios (clean env, bogus ambient DATABASE_URL, explicit TEST_DATABASE_URL).
  • Behavioral check: ran suite subsets with DATABASE_URL=postgresql+asyncpg://fake-dev-host:5432/production_db in the environment — tests run on SQLite and pass instead of connecting anywhere.

Validation

  • Full backend suite (py3.12, SQLite default): 297 passed, 8 skipped
  • ruff check + ruff format --check: clean
  • .github/workflows/ci.yml: valid YAML

lucastononro added a commit that referenced this pull request Jul 29, 2026
Stack wave 5, PR 2/4. Greptile P1 (ambient DATABASE_URL drop risk)
already fixed on the PR branch by author fixup de197b8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant