Integration test suite, test tooling, and a batch PATCH bug fix

[podmar]

feat/test — integration test suite, test tooling, and a batch PATCH bug fix

Decisions worth noting

• Integration tests only, no mocks. The bugs this app can have are almost all data isolation bugs — wrong household_id filter, missing scope on a query. Mocks would hide exactly those. Every test hits a real Postgres DB via ASGI transport.

• other_auth fixture for isolation tests. Rather than manually registering a second user in every test, a shared fixture registers Bob in a separate household. Every resource has at least one test that proves a cross-household request returns 404, not 403 — returning 403 would leak whether an ID exists.

• Barcode upsert returns 200, not 201. A duplicate barcode POST returns the existing item with 200 rather than 409. The test names this explicitly (test_duplicate_barcode_returns_existing_with_200) so reviewers understand this is intentional, not a missing uniqueness check. A companion test verifies that the uniqueness constraint is scoped to household_id, not global — a globally unique barcode constraint would silently break multi-tenant use.

• Batch collision check before setattr. The PATCH handler was checking for a (location, expiry) collision after applying updates to the ORM object. SQLAlchemy's autoflush would emit the UPDATE before the SELECT, hitting the DB unique constraint instead of letting the code handle the merge. The fix resolves the new (location_id, expiry_date) first, checks for a collision, then applies setattr. Tests cover both the location-change and expiry-change collision paths.

• DDL once per session, TRUNCATE per test. The previous approach dropped and recreated all tables for every test. Moving DDL to a session-scoped fixture and using TRUNCATE ... RESTART IDENTITY CASCADE per test is faster. A separate asyncio.run() call in the session fixture avoids event loop conflicts with NullPool.

• Safety guard: TEST_DATABASE_URL must differ from DATABASE_URL. If both point to the same DB, every test run truncates all tables. The check reads both URLs via the existing _Env settings class and raises at import time if they match.

• run-tests.sh uses tee. The script writes to docs/test-report.txt while streaming output live — you see results as they run rather than waiting for the full suite to finish before anything appears.

• /fix-test skill. Works through docs/test-report.txt one failure at a time, forces diagnosis into one of four named modes (source bug / test bug / fixture bug / structural error), and requires user confirmation before writing any fix.

What's next

Auth tests, 401 tests, and switching to a local Postgres for tests — the suite currently runs against Neon which dominates the runtime regardless of other optimisations.

[claude]

Strong test suite with excellent household isolation coverage. The batch update fix is a textbook example of understanding ORM autoflush behavior.

[claude]

Security: Test database isolation not enforced

The test infrastructure doesn't verify that TEST_DATABASE_URL points to a different database than DATABASE_URL. If they're the same (e.g., both point to production), every test run will truncate all production data (line 59).

Why this matters: The .env.example shows both URLs, but nothing prevents a developer from copying production credentials to both variables or forgetting to set TEST_DATABASE_URL.

Suggested fix: Add a startup check in conftest.py:

_prod_url = os.getenv("DATABASE_URL", "")
_test_url = _env.test_database_url.get_secret_value()

if _prod_url and _prod_url == _test_url:
    raise RuntimeError(
        "TEST_DATABASE_URL must point to a separate database, not production. "
        "Tests TRUNCATE all tables on every run."
    )

This is the kind of defensive check that prevents catastrophic mistakes in a learning project — and shows you're thinking about operational safety, which is great for interviews.

[podmar]

great catch, thank you!

[claude]

Logic issue: Unclear test intent

This test creates two batches with no explicit location_id or expiry_date, expecting them to merge because they resolve to the same defaults. But the test doesn't verify what those defaults are — it just assumes they'll be identical.

Why this is fragile: If the default location logic changes (e.g., from "first seeded" to "last used"), this test might start failing not because merge logic broke, but because the two POSTs now resolve to different locations.

Suggested improvement: Explicitly set the fields to make the intent clear:

async def test_create_batch_merges_on_collision(client: AsyncClient, auth, item_id):
    pantry_id = (await client.get("/locations", headers=auth)).json()[0]["id"]
    expiry = "2026-12-31"
    
    first = await client.post(
        f"/items/{item_id}/batches", 
        json={"quantity": 3, "location_id": pantry_id, "expiry_date": expiry}, 
        headers=auth
    )
    second = await client.post(
        f"/items/{item_id}/batches", 
        json={"quantity": 2, "location_id": pantry_id, "expiry_date": expiry}, 
        headers=auth
    )
    assert second.json()["quantity"] == 5

This way the test documents what "collision" means and won't break if default logic evolves.

[claude]

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.

[claude]

Edge case: Expiry date collision not tested for PATCH

The test verifies that moving batch B to the same location as batch A causes a merge, but it doesn't test the full collision condition: (item_id, location_id, expiry_date).

Missing scenario: What if both batches are already in the Pantry with different expiry dates, and you PATCH one batch's expiry_date to match the other? That should also trigger a merge.

Why it matters: The updated code at batches.py:176 checks new_expiry_date, but this test only exercises new_location_id. A test that changes expiry would verify the full collision detection logic.

Suggested test:

async def test_update_batch_expiry_merges_on_collision(client: AsyncClient, auth, item_id):
    pantry_id = (await client.get("/locations", headers=auth)).json()[0]["id"]
    
    batch_a = (await client.post(
        f"/items/{item_id}/batches", 
        json={"quantity": 3, "location_id": pantry_id, "expiry_date": "2026-06-01"}, 
        headers=auth
    )).json()
    
    batch_b = (await client.post(
        f"/items/{item_id}/batches", 
        json={"quantity": 2, "location_id": pantry_id, "expiry_date": "2026-12-01"}, 
        headers=auth
    )).json()
    
    # Change batch B's expiry to match batch A → should merge.
    resp = await client.patch(
        f"/batches/{batch_b['id']}", 
        json={"expiry_date": "2026-06-01"}, 
        headers=auth
    )
    assert resp.status_code == 200
    assert resp.json()["id"] == batch_a["id"]
    assert resp.json()["quantity"] == 5

[claude]

Minor: Inconsistent whitespace in test fixtures

The item_id fixture has no blank line before the function body, but other fixtures in this file (and across the test suite) do. Keeping it consistent makes the code easier to scan.

Suggested change:

@pytest.fixture
async def item_id(client, auth):
    resp = await client.post(
        "/items", json={"name": "Milk", "unit": "litre"}, headers=auth
    )
    return resp.json()["id"]

(No blank line after the decorator is the pattern used elsewhere in the tests.)

[podmar]

skipping, sounds like a false positive

[claude]

Minor: Consider adding a household isolation test for PATCH /items/{id} to complete the coverage. Pattern:

async def test_isolation_cannot_update_other_household_item(
    client: AsyncClient, auth, other_auth, item_id
):
    resp = await client.patch(
        f"/items/{item_id}", 
        json={"name": "Stolen"}, 
        headers=other_auth
    )
    assert resp.status_code == 404

This would catch regressions if the household filter is accidentally removed from the UPDATE endpoint.

[claude]

Minor: Consider adding isolation tests for:

• GET /items/{item_id}/batches (list batches for another household's item)
• PATCH /batches/{id} (update another household's batch via the regular PATCH, not adjust)
• DELETE /batches/{id} (delete another household's batch)

You have test_isolation_cannot_adjust_other_household_batch which tests the adjust endpoint, but the regular PATCH and DELETE endpoints aren't covered. These follow the same pattern as your existing isolation tests and would complete the coverage.

[claude]

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.

[claude]

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!).

[claude]

Nice test for the merge behavior! This directly validates the fix in batches.py:168-176. The test confirms that:

1. You can create two batches in different locations
2. Moving one to collide with the other triggers the merge
3. The quantities combine correctly

This is exactly the scenario that would have failed before your fix (the autoflush would have hit the unique constraint before the merge logic ran).

[claude]

Review Summary

Critical: None — no blocking issues found.

Minor suggestions:

• Consider adding household isolation tests for PATCH /items/{id}, PATCH /batches/{id}, DELETE /batches/{id}, and GET /items/{item_id}/batches to complete the security coverage
• The NullPool comment in conftest.py could be slightly more explicit about when the "Future attached to a different loop" error occurs

Highlights:

• The batch update fix (lines 168-176 in batches.py) correctly solves the autoflush collision issue by calculating target values before marking the object dirty — excellent understanding of ORM behavior
• Comprehensive test coverage across all major endpoints with good isolation testing
• Clean test structure with reusable fixtures

Cost: $0.21