forgejo_admin/basketball-api
1
0
Fork 1
Code Issues 10 Pull requests 3 Projects Releases Packages Wiki Activity Actions

Rewrite test fixtures for Postgres CI #17

Merged
forgejo_admin merged 1 commit from 16-postgres-ci-testing-rewrite-test-fixture into main 2026-03-09 23:05:15 +00:00
Conversation 1 Commits 1 Files changed 5 +124 -192
forgejo_admin commented 2026-03-09 22:55:07 +00:00
Owner
Copy link

Summary

Replace per-file SQLite test databases with shared Postgres-backed fixtures in conftest.py. Add a postgres:16-alpine service container to Woodpecker CI so tests run against real Postgres in the pipeline.

Changes

  • .woodpecker.yaml — Added services: block with postgres:16-alpine container; set BASKETBALL_DATABASE_URL env var in test step
  • tests/conftest.py — Rewritten with shared fixtures: engine, setup_db (autouse, create/drop per test), db (session), tenant (seeded tenant), client (TestClient with get_db override)
  • tests/test_register.py — Removed module-level SQLite engine/session/setup/teardown; now uses conftest fixtures via pytest injection
  • tests/test_coach.py — Removed module-level SQLite engine/session; now uses conftest fixtures; kept coach fixture local to this module
  • tests/test_models.py — Removed module-level SQLite engine/session/setup/teardown; now uses conftest fixtures

Test Plan

  • CI pipeline runs pytest against the Postgres service container
  • ruff check . and ruff format --check . pass
  • All existing test assertions preserved — no behavioral changes

Review Checklist

  • No src/ application code modified — test infrastructure only
  • ruff check . passes
  • ruff format --check . passes
  • Mocked Stripe preserved in coach tests
  • Service container uses postgres:16-alpine

Related

## Summary Replace per-file SQLite test databases with shared Postgres-backed fixtures in conftest.py. Add a `postgres:16-alpine` service container to Woodpecker CI so tests run against real Postgres in the pipeline. ## Changes - `.woodpecker.yaml` — Added `services:` block with `postgres:16-alpine` container; set `BASKETBALL_DATABASE_URL` env var in test step - `tests/conftest.py` — Rewritten with shared fixtures: `engine`, `setup_db` (autouse, create/drop per test), `db` (session), `tenant` (seeded tenant), `client` (TestClient with `get_db` override) - `tests/test_register.py` — Removed module-level SQLite engine/session/setup/teardown; now uses conftest fixtures via pytest injection - `tests/test_coach.py` — Removed module-level SQLite engine/session; now uses conftest fixtures; kept `coach` fixture local to this module - `tests/test_models.py` — Removed module-level SQLite engine/session/setup/teardown; now uses conftest fixtures ## Test Plan - CI pipeline runs `pytest` against the Postgres service container - `ruff check .` and `ruff format --check .` pass - All existing test assertions preserved — no behavioral changes ## Review Checklist - [x] No `src/` application code modified — test infrastructure only - [x] `ruff check .` passes - [x] `ruff format --check .` passes - [x] Mocked Stripe preserved in coach tests - [x] Service container uses `postgres:16-alpine` ## Related - Closes #16
Rewrite test fixtures for Postgres CI, add Woodpecker service container
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
595f4c74b4 
Replace per-file SQLite test databases with shared Postgres-backed fixtures
in conftest.py. All test files now use the shared engine/session/client/tenant
fixtures instead of module-level setup/teardown. Add postgres:16-alpine service
container to .woodpecker.yaml with DATABASE_URL passed to the test step.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
forgejo_admin commented 2026-03-09 23:04:56 +00:00
Author
Owner
Copy link

PR #17 Review

BLOCKERS

None. The code is correct and well-structured.

NITS

  1. test_register.py _seed_tenant fixture body is empty. The fixture depends on tenant (which seeds it), so the empty body is functionally correct. However, adding a one-line docstring body or a pass would make it clearer that the fixture exists solely to pull in the tenant dependency. Currently it has a docstring but no yield or pass -- this works because docstrings are valid function bodies in Python, but a yield would make the autouse lifecycle explicit.

  2. conftest.py module-level engine and TestSession are created at import time. This means the BASKETBALL_DATABASE_URL env var must be set before conftest.py is imported, which it is (via os.environ.setdefault at the top of the file before the settings import). This is correct but fragile -- any future conftest refactoring that reorders imports could break it. The # noqa: E402 comments acknowledge this. Not blocking, just worth noting for future maintainers.

  3. Woodpecker service container has no healthcheck or readiness wait. The postgres:16-alpine service container may not be ready when pytest starts. Postgres typically starts fast enough that this is not an issue in practice, but if CI flakes appear, adding pg_isready to the commands before pytest would fix it. Example: apt-get update && apt-get install -y postgresql-client && pg_isready -h postgres -U basketball -d basketball_test --timeout=30.

SOP COMPLIANCE

  • Branch named after issue (16-postgres-ci-testing-rewrite-test-fixture references #16)
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related references Closes #16
  • No secrets committed (test credentials are hardcoded test-only values, not real secrets)
  • No unnecessary file changes -- exactly 5 files, all test infrastructure, no src/ modifications
  • Stripe mocks preserved in coach tests (confirmed @patch decorators on all Stripe calls)
  • BASKETBALL_DATABASE_URL env var matches app config (env_prefix = "BASKETBALL_" + field database_url in /home/ldraney/basketball-api/src/basketball_api/config.py)
  • No session leaks -- db fixture uses try/finally/session.close(), setup_db drops all tables after each test
  • Woodpecker service syntax is valid (top-level services: block with name, image, environment)

VERDICT: APPROVED

## PR #17 Review ### BLOCKERS None. The code is correct and well-structured. ### NITS 1. **`test_register.py` `_seed_tenant` fixture body is empty.** The fixture depends on `tenant` (which seeds it), so the empty body is functionally correct. However, adding a one-line docstring body or a `pass` would make it clearer that the fixture exists solely to pull in the `tenant` dependency. Currently it has a docstring but no `yield` or `pass` -- this works because docstrings are valid function bodies in Python, but a `yield` would make the autouse lifecycle explicit. 2. **`conftest.py` module-level `engine` and `TestSession` are created at import time.** This means the `BASKETBALL_DATABASE_URL` env var must be set before `conftest.py` is imported, which it is (via `os.environ.setdefault` at the top of the file before the `settings` import). This is correct but fragile -- any future conftest refactoring that reorders imports could break it. The `# noqa: E402` comments acknowledge this. Not blocking, just worth noting for future maintainers. 3. **Woodpecker service container has no healthcheck or readiness wait.** The `postgres:16-alpine` service container may not be ready when `pytest` starts. Postgres typically starts fast enough that this is not an issue in practice, but if CI flakes appear, adding `pg_isready` to the commands before `pytest` would fix it. Example: `apt-get update && apt-get install -y postgresql-client && pg_isready -h postgres -U basketball -d basketball_test --timeout=30`. ### SOP COMPLIANCE - [x] Branch named after issue (`16-postgres-ci-testing-rewrite-test-fixture` references #16) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references `Closes #16` - [x] No secrets committed (test credentials are hardcoded test-only values, not real secrets) - [x] No unnecessary file changes -- exactly 5 files, all test infrastructure, no `src/` modifications - [x] Stripe mocks preserved in coach tests (confirmed `@patch` decorators on all Stripe calls) - [x] `BASKETBALL_DATABASE_URL` env var matches app config (`env_prefix = "BASKETBALL_"` + field `database_url` in `/home/ldraney/basketball-api/src/basketball_api/config.py`) - [x] No session leaks -- `db` fixture uses `try/finally/session.close()`, `setup_db` drops all tables after each test - [x] Woodpecker service syntax is valid (top-level `services:` block with `name`, `image`, `environment`) ### VERDICT: APPROVED
forgejo_admin deleted branch 16-postgres-ci-testing-rewrite-test-fixture 2026-03-09 23:05:15 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend

   
status:approved

QA passed, awaiting merge approval

  
status:in-progress

Dev agent is actively working

  
status:needs-fix

QA found issues, back to dev

  
status:qa

PR submitted, awaiting QA review

  
type:bug

Bug fix

  
type:devops

Infrastructure/CI/config work

  
type:feature

New functionality

No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/basketball-api!17
No description provided.