From 0a50e0ffb652de1008f4e5b1e549c6406ab99a7e Mon Sep 17 00:00:00 2001 From: NixReaper Date: Sat, 5 Sep 2026 20:17:02 -0500 Subject: [PATCH] Fix two more test bugs surfaced by the full integration run 1. test_ingestion_cache_invalidation.py: the job was created through the test's db_session fixture, whose commit() only ends a savepoint that gets rolled back at teardown (see tests/conftest.py) -- it's never a real commit. _run_ingestion looks the job up through its own session_scope() (a genuinely separate connection), which could never see it, so the very first _require_job call raised "job vanished mid-run" every time. Worse, test_failed_ingestion_does_not_ invalidate_the_cache still passed, coincidentally -- it expected a RuntimeError and got one, just the wrong one, masking the bug. Fixed by having _make_job create the job through session_scope() itself, matching test_ingestion_retry.py's existing pattern. 2. Same class of bug as the video-collision fix in the last commit: FAKE_MOVIE's fixed imdb_id ("tt0133093"), not just its video, also collides across two different fake movies sharing it unmodified -- uq_external_id_source is a real unique constraint on (provider_id, entity_type, external_id). Fixed by also overriding imdb_id on the second movie in both affected tests. All caught by running the actual integration suite against Postgres -- these paths were previously only collect-only'd locally (no local DB). Co-Authored-By: Claude Sonnet 5 --- .../integration/test_collection_ingestion.py | 8 ++-- .../test_ingestion_cache_invalidation.py | 46 +++++++++++-------- tests/integration/test_movie_ingestion.py | 11 +++-- 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/tests/integration/test_collection_ingestion.py b/tests/integration/test_collection_ingestion.py index 39d3191..beb9f25 100644 --- a/tests/integration/test_collection_ingestion.py +++ b/tests/integration/test_collection_ingestion.py @@ -57,9 +57,10 @@ async def test_ingest_collection_only_links_already_ingested_movies(db_session: already knows about, in release-date order, never fabricate a stub for the other two (section 15).""" - # 604's videos overridden to empty: FAKE_MOVIE's embedded video has a - # fixed provider_video_id, and two different movies both ingesting it - # would collide on the real uq_video_provider_source unique constraint. + # 604's videos and imdb_id overridden: FAKE_MOVIE's embedded video and + # imdb_id are both fixed identifiers, and two different movies both + # ingesting them unmodified would collide on the real + # uq_video_provider_source / uq_external_id_source unique constraints. provider = FakeTMDbCollectionProvider( movie_payloads={ "603": {**FAKE_MOVIE, "id": 603, "title": "The Matrix"}, @@ -68,6 +69,7 @@ async def test_ingest_collection_only_links_already_ingested_movies(db_session: "id": 604, "title": "The Matrix Reloaded", "videos": {"results": []}, + "imdb_id": "tt0234215", }, }, collection_payload=FAKE_COLLECTION, diff --git a/tests/integration/test_ingestion_cache_invalidation.py b/tests/integration/test_ingestion_cache_invalidation.py index dddd502..a938972 100644 --- a/tests/integration/test_ingestion_cache_invalidation.py +++ b/tests/integration/test_ingestion_cache_invalidation.py @@ -14,33 +14,43 @@ import uuid import pytest from sqlalchemy.ext.asyncio import AsyncSession +from app.core.database import session_scope from app.core.ids import aurora_id from app.repositories.provider_repository import create_ingestion_job, get_or_create_provider from app.services.cache import build_key from app.services.ingestion import runner -async def _make_job(session: AsyncSession, *, attempts_already_used: int = 0) -> uuid.UUID: - provider = await get_or_create_provider(session, "tmdb", "TMDB") - job = await create_ingestion_job( - session, - provider_id=provider.id, - job_type="movie_sync", - idempotency_key=f"test:{uuid.uuid4()}", - entity_type="movie", - external_id="603", - ) - job.attempts = attempts_already_used - await session.flush() - return job.id +async def _make_job(*, attempts_already_used: int = 0) -> uuid.UUID: + # _run_ingestion looks the job up through its own session_scope() (a + # separate connection from the test's db_session fixture, which only + # fakes commits inside a savepoint that's rolled back at teardown -- + # see tests/conftest.py). Creating the job through session_scope() too + # (matching test_ingestion_retry.py) makes it a real, visible commit + # instead of one only db_session's own connection can see. + async with session_scope() as session: + provider = await get_or_create_provider(session, "tmdb", "TMDB") + job = await create_ingestion_job( + session, + provider_id=provider.id, + job_type="movie_sync", + idempotency_key=f"test:{uuid.uuid4()}", + entity_type="movie", + external_id="603", + ) + job.attempts = attempts_already_used + await session.flush() + return job.id @pytest.mark.asyncio async def test_successful_ingestion_invalidates_the_entity_cache_key( db_session: AsyncSession, monkeypatch ): - job_id = await _make_job(db_session) - await db_session.commit() + # db_session is unused directly (see _make_job's docstring-comment) but + # kept as a fixture dependency so this test skips cleanly when Postgres + # isn't reachable, matching test_ingestion_retry.py's pattern. + job_id = await _make_job() result_id = uuid.uuid4() deleted_keys: list[str] = [] @@ -66,8 +76,7 @@ async def test_successful_ingestion_invalidates_the_entity_cache_key( async def test_ingestion_without_cache_entity_type_does_not_touch_the_cache( db_session: AsyncSession, monkeypatch ): - job_id = await _make_job(db_session) - await db_session.commit() + job_id = await _make_job() async def fake_work(session, provider): return uuid.uuid4() @@ -92,8 +101,7 @@ async def test_failed_ingestion_does_not_invalidate_the_cache( # attempts_already_used=4 (default max_attempts is 5) so this failure is # terminal and raises the original error rather than arq.Retry -- either # way the cache must not be touched, but this keeps the assertion simple. - job_id = await _make_job(db_session, attempts_already_used=4) - await db_session.commit() + job_id = await _make_job(attempts_already_used=4) async def failing_work(session, provider): raise RuntimeError("simulated failure") diff --git a/tests/integration/test_movie_ingestion.py b/tests/integration/test_movie_ingestion.py index 4a37aa1..9e3aa90 100644 --- a/tests/integration/test_movie_ingestion.py +++ b/tests/integration/test_movie_ingestion.py @@ -188,16 +188,17 @@ async def test_movie_recommendations_only_include_already_ingested_movies( external id (section 15: never fabricate a stub for an unseen entity), and GET .../recommendations must return them in TMDb's ranked order.""" - # videos overridden to empty: FAKE_MOVIE's embedded video has a fixed - # provider_video_id, and two different movies both ingesting it would - # collide on the real uq_video_provider_source unique constraint -- - # a video id is only ever attached to one entity, unlike genres/companies - # which are legitimately shared. + # videos and imdb_id overridden: FAKE_MOVIE's embedded video and imdb_id + # are both fixed identifiers, and two different movies both ingesting + # them unmodified would collide on the real uq_video_provider_source / + # uq_external_id_source unique constraints -- each belongs to exactly + # one entity, unlike genres/companies which are legitimately shared. sequel_payload = { **FAKE_MOVIE, "id": 604, "title": "The Matrix Reloaded", "videos": {"results": []}, + "imdb_id": "tt0234215", } await ingest_movie(db_session, FakeTMDbProvider(sequel_payload), "604") await db_session.flush()