-
Notifications
You must be signed in to change notification settings - Fork 0
Integration test suite, test tooling, and a batch PATCH bug fix #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1c6759c
77a4a50
43a2f1b
9cee665
ac4ee67
a7b0b7f
15735d9
26fc123
917d9fd
1adec6c
8a759e9
68bc28f
7ed3f19
f548c1e
0b350da
7708823
f15c1fb
4c3e1fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| Read `docs/test-report.txt`. If the file doesn't exist, tell the user to run `./scripts/run-tests.sh` first and stop. If all tests passed, say so and stop. | ||
|
|
||
| Count the failures and tell the user how many there are before starting. Work through them one at a time in the order they appear in the report. | ||
|
|
||
| For each failure: | ||
|
|
||
| Show the test name and the full error message and traceback — do not truncate it. | ||
|
|
||
| Read the failing test function from its test file. Also read the router or source function it exercises — find it by tracing the HTTP path the test calls (e.g. `POST /items` → `app/routers/items.py`). | ||
|
|
||
| Diagnose which of these four failure modes applies: | ||
| - **Source bug**: the source code does not implement what the test expects. Fix the source. | ||
| - **Test bug**: the test asserts something wrong (wrong status code, wrong field name, wrong logic). Fix the test. | ||
| - **Fixture bug**: a shared fixture in `conftest.py` produces unexpected state. Fix the fixture. | ||
| - **Structural error**: import failure, missing dependency, schema mismatch. Name the root cause explicitly before proposing anything. | ||
|
|
||
| If the diagnosis is ambiguous between two modes, say which two and ask the user before touching anything. | ||
|
|
||
| For source bugs and test bugs: propose the exact change and state why you're confident in the diagnosis. Wait for the user to confirm before writing. | ||
|
|
||
| For fixture bugs: do the same, but name every test that uses the fixture so the user knows the blast radius. | ||
|
|
||
| For structural errors: do not propose a fix until the root cause is certain. Describe what you'd need to verify and ask whether to proceed. | ||
|
|
||
| After each fix, ask the user to re-run `./scripts/run-tests.sh` and confirm before continuing. | ||
|
|
||
| Do not commit. The user handles all commits. | ||
|
|
||
| When all failures are done, list every file changed and what was changed in each. List any failure you skipped and why. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,19 +168,19 @@ async def update_batch( | |
| if location is None or location.household_id != user.household_id: | ||
| raise HTTPException(status_code=404, detail="Location not found") | ||
|
|
||
| for field, value in updates.items(): | ||
| setattr(batch, field, value) | ||
| # Resolve what the new (location, expiry) would be after the update, then check | ||
| # for a collision BEFORE modifying the batch. Modifying first would mark the ORM | ||
| # object dirty; SQLAlchemy's autoflush would then emit the UPDATE before our SELECT, | ||
| # hitting the unique constraint instead of letting us handle the merge gracefully. | ||
| new_location_id = updates.get("location_id", batch.location_id) | ||
| new_expiry_date = updates.get("expiry_date", batch.expiry_date) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent fix! This correctly solves the autoflush issue. By calculating the hypothetical future values before modifying the batch object, you avoid marking it dirty, which would trigger an autoflush during the collision-check query and hit the unique constraint. The comment explaining the "why" is perfect for a portfolio project — a reviewer will immediately understand the subtlety of ORM behavior here. Well done. |
||
|
|
||
| # Check whether the updated (item, location, expiry) would collide with an existing batch. | ||
| # If so, merge quantities into the surviving batch and delete this one — same behaviour | ||
| # as the location-delete move flow. Returns the surviving batch so the client sees the | ||
| # correct quantity at the destination without needing a separate fetch. | ||
| existing = ( | ||
| await session.exec( | ||
| select(Batch).where( | ||
| Batch.item_id == batch.item_id, | ||
| Batch.location_id == batch.location_id, | ||
| Batch.expiry_date == batch.expiry_date, | ||
| Batch.location_id == new_location_id, | ||
| Batch.expiry_date == new_expiry_date, | ||
| Batch.household_id == user.household_id, | ||
| Batch.id != batch_id, | ||
| ) | ||
|
|
@@ -195,6 +195,9 @@ async def update_batch( | |
| await session.refresh(existing) | ||
| return BatchRead.model_validate(existing) | ||
|
|
||
| for field, value in updates.items(): | ||
| setattr(batch, field, value) | ||
|
|
||
| session.add(batch) | ||
| await session.commit() | ||
| await session.refresh(batch) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import asyncio | ||
| from collections.abc import AsyncGenerator | ||
|
|
||
| import pytest | ||
| from httpx import ASGITransport, AsyncClient | ||
| from pydantic import SecretStr | ||
| from pydantic_settings import BaseSettings, SettingsConfigDict | ||
| from sqlalchemy import text | ||
| from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine | ||
| from sqlalchemy.pool import NullPool | ||
| from sqlmodel import SQLModel | ||
| from sqlmodel.ext.asyncio.session import AsyncSession | ||
|
|
||
| import app.database as _db | ||
|
|
||
|
|
||
| class _Env(BaseSettings): | ||
| model_config = SettingsConfigDict(env_file=".env", extra="ignore") | ||
| database_url: SecretStr | ||
| test_database_url: SecretStr | ||
|
|
||
|
|
||
| _env = _Env() # type: ignore[call-arg] | ||
|
|
||
| if _env.database_url.get_secret_value() == _env.test_database_url.get_secret_value(): | ||
| raise RuntimeError( | ||
| "TEST_DATABASE_URL must differ from DATABASE_URL — tests TRUNCATE all tables." | ||
| ) | ||
|
|
||
| # Replace the app's engine and session factory with test-DB equivalents before | ||
| # app.main is imported. All subsequent imports see the patched module globals, | ||
| # so every request in tests hits the test DB — not production. | ||
| # NullPool creates a fresh connection per operation and never retains one between | ||
| # uses — required for async SQLAlchemy tests where each test runs in its own | ||
| # event loop. A retained connection would be bound to the loop that created it | ||
| # and fail with "Future attached to a different loop" in subsequent tests, | ||
| # because pytest-asyncio creates a new event loop per test function by default. | ||
| _test_engine = create_async_engine( | ||
| _env.test_database_url.get_secret_value(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good choice using NullPool. The comment on line 27-29 explains it well, but you could make it even clearer: # NullPool creates a fresh connection per operation and never retains one between
# uses. This prevents "Future attached to a different loop" errors when running
# async tests where each test gets its own event loop (required with pytest-asyncio).This level of detail helps someone reading this as a learning resource understand exactly when they'd hit this error (it's not obvious until you hit it!). |
||
| poolclass=NullPool, | ||
| ) | ||
| _db.engine = _test_engine | ||
| _db._session_factory = async_sessionmaker( | ||
| _test_engine, class_=AsyncSession, expire_on_commit=False | ||
| ) | ||
|
|
||
| from app.main import app # noqa: E402 — must follow the patch above | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session", autouse=True) | ||
| def create_schema() -> None: | ||
| # Run DDL once per session rather than per test. asyncio.run() opens a fresh | ||
| # event loop here so the session-scoped fixture doesn't conflict with the | ||
| # function-scoped loops used by individual tests (required with NullPool). | ||
| async def _setup() -> None: | ||
| async with _test_engine.begin() as conn: | ||
| await conn.run_sync(SQLModel.metadata.drop_all) | ||
| await conn.run_sync(SQLModel.metadata.create_all) | ||
|
|
||
| asyncio.run(_setup()) | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| async def truncate_tables() -> None: | ||
| # TRUNCATE is DML, not DDL — much faster than drop/create per test. | ||
| # CASCADE handles FK ordering; RESTART IDENTITY resets sequences between tests. | ||
| table_names = ", ".join(f'"{t.name}"' for t in SQLModel.metadata.sorted_tables) | ||
| async with _test_engine.begin() as conn: | ||
| await conn.execute(text(f"TRUNCATE {table_names} RESTART IDENTITY CASCADE")) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| async def client() -> AsyncGenerator[AsyncClient]: | ||
| async with AsyncClient( | ||
| transport=ASGITransport(app=app), base_url="http://test" | ||
| ) as ac: | ||
| yield ac | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| async def auth(client: AsyncClient) -> dict[str, str]: | ||
| await client.post( | ||
| "/auth/register", json={"email": "alice@test.com", "password": "testpass123"} | ||
| ) | ||
| resp = await client.post( | ||
| "/auth/jwt/login", | ||
| data={"username": "alice@test.com", "password": "testpass123"}, | ||
| ) | ||
| return {"Authorization": f"Bearer {resp.json()['access_token']}"} | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| async def other_auth(client: AsyncClient) -> dict[str, str]: | ||
| """Second user in a separate household — for isolation tests.""" | ||
| await client.post( | ||
| "/auth/register", json={"email": "bob@test.com", "password": "testpass123"} | ||
| ) | ||
| resp = await client.post( | ||
| "/auth/jwt/login", data={"username": "bob@test.com", "password": "testpass123"} | ||
| ) | ||
| return {"Authorization": f"Bearer {resp.json()['access_token']}"} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix! This is exactly right — checking the collision before modifying the batch prevents SQLAlchemy's autoflush from firing the UPDATE prematurely.
Interview tip: This is a great example to walk through in an interview when discussing ORM behavior. The comment explains the "why" clearly, which is exactly what CLAUDE.md asks for. Well done.