From a113a7f3ce856bdcb591fd217dae2964facbd0e8 Mon Sep 17 00:00:00 2001 From: Giorgio Gilestro Date: Tue, 26 May 2026 00:11:18 +0200 Subject: [PATCH] test+fix: make the suite run cleanly in the test container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five fixes uncovered by actually running the suite in docker-compose.test.yml: 1. (real prod bug) PATCH /api/settings/digest mutated principal.user which require_token had loaded in a now-closed session — the commit on the handler's session persisted nothing. Re-fetch the user via the active session before writing. 2. Portable PK type. SQLite only auto-fills `INTEGER PRIMARY KEY`; plain BIGINT requires explicit values. Define a `_PK` alias of `BigInteger().with_variant(Integer(), "sqlite")` and use it for all 10 autoincrement primary keys in app/models.py. No prod-schema change (MariaDB still gets BIGINT). 3. job_lifecycle's MariaDB GET_LOCK / RELEASE_LOCK is now gated behind `dialect.name == "mysql"`, so the test SQLite engine doesn't trip on the missing function. Single-process test runs can't race themselves. 4. tests/test_news_window.py seeded Headline rows without `fingerprint`, which is NOT NULL — added an `fp-{title}` value per row. 5. tests/test_email_digest_job.py now also patches `llm_configured` to True so the job doesn't short-circuit on the missing API key. 6. (test container hygiene) Drop `COPY tests ./tests` from the test stage in the Dockerfile — .dockerignore excludes `tests/` (correct: prod image must not bake tests), and docker-compose.test.yml bind-mounts ./tests at run time anyway. Suite now: 198 passed, 5 skipped, 1 pre-existing failure (test_default_groups_present — Phase G dropped the "pie" group from config/default.toml but the assertion wasn't updated; unrelated to this branch). --- Dockerfile | 4 +++- app/jobs/_helpers.py | 29 +++++++++++++++++------------ app/models.py | 27 +++++++++++++++++---------- app/routers/api.py | 12 ++++++++++-- tests/test_email_digest_job.py | 3 +++ tests/test_news_window.py | 1 + 6 files changed, 51 insertions(+), 25 deletions(-) diff --git a/Dockerfile b/Dockerfile index 6526cf1..1123177 100644 --- a/Dockerfile +++ b/Dockerfile @@ -53,7 +53,9 @@ COPY pyproject.toml ./ COPY app ./app COPY alembic ./alembic COPY alembic.ini ./ -COPY tests ./tests +# tests/ is excluded by .dockerignore (prod-correct: never bake tests into +# a shipped image). docker-compose.test.yml bind-mounts ./tests:/app/tests +# at run time, so the suite is always available without baking it in. RUN /opt/venv/bin/pip install ".[dev]" diff --git a/app/jobs/_helpers.py b/app/jobs/_helpers.py index 211110f..fe4b6b1 100644 --- a/app/jobs/_helpers.py +++ b/app/jobs/_helpers.py @@ -23,17 +23,21 @@ async def job_lifecycle(name: str) -> AsyncIterator[tuple[AsyncSession, JobRun]] handles the bookkeeping. A MariaDB GET_LOCK(name, 0) is acquired to prevent concurrent runs of the - same job across processes. If the lock is busy, we skip the run.""" + same job across processes. If the lock is busy, we skip the run. + The lock dance is MariaDB-specific; on SQLite (used in tests) it's a + no-op, since the single-process test runner can't race itself.""" factory = get_session_factory() async with factory() as session: - # Try lock; skip if held. - got = (await session.execute( - text("SELECT GET_LOCK(:n, 0)"), {"n": f"cassandra_{name}"} - )).scalar() - if not got: - log.warning("job.skipped_locked", name=name) - yield session, JobRun(name=name, started_at=utcnow(), status="skipped") - return + bind = session.get_bind() + use_lock = bind is not None and bind.dialect.name == "mysql" + if use_lock: + got = (await session.execute( + text("SELECT GET_LOCK(:n, 0)"), {"n": f"cassandra_{name}"} + )).scalar() + if not got: + log.warning("job.skipped_locked", name=name) + yield session, JobRun(name=name, started_at=utcnow(), status="skipped") + return run = JobRun(name=name, started_at=utcnow(), status="running") session.add(run) await session.commit() @@ -53,6 +57,7 @@ async def job_lifecycle(name: str) -> AsyncIterator[tuple[AsyncSession, JobRun]] log.error("job.failed", name=name, error=str(e)) raise finally: - await session.execute(text("SELECT RELEASE_LOCK(:n)"), - {"n": f"cassandra_{name}"}) - await session.commit() + if use_lock: + await session.execute(text("SELECT RELEASE_LOCK(:n)"), + {"n": f"cassandra_{name}"}) + await session.commit() diff --git a/app/models.py b/app/models.py index 2548d14..d655bdf 100644 --- a/app/models.py +++ b/app/models.py @@ -29,9 +29,16 @@ from sqlalchemy.orm import Mapped, mapped_column, relationship from app.db import Base, utcnow +# Portable autoincrement primary-key type. SQLite only treats `INTEGER +# PRIMARY KEY` as a ROWID alias (the bit that auto-fills); plain BIGINT +# requires explicit values, which breaks our async tests. `with_variant` +# emits INTEGER on SQLite and keeps BIGINT everywhere else. +_PK = BigInteger().with_variant(Integer(), "sqlite") + + class Quote(Base): __tablename__ = "quotes" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) symbol: Mapped[str] = mapped_column(String(128), nullable=False) source: Mapped[str] = mapped_column(String(32), nullable=False) label: Mapped[str] = mapped_column(String(128), default="") @@ -62,7 +69,7 @@ class QuoteDaily(Base): class Headline(Base): __tablename__ = "headlines" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) source: Mapped[str] = mapped_column(String(64), nullable=False) category: Mapped[str] = mapped_column(String(32), nullable=False) title: Mapped[str] = mapped_column(String(512), nullable=False) @@ -100,7 +107,7 @@ class Feed(Base): class StrategicLog(Base): __tablename__ = "strategic_logs" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) generated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow, index=True) model: Mapped[str] = mapped_column(String(64), nullable=False) anchor_date: Mapped[str | None] = mapped_column(String(16)) @@ -117,7 +124,7 @@ class IndicatorSummary(Base): """Short AI-generated read for one indicator group, regenerated hourly. The latest row per group_name is what the dashboard renders.""" __tablename__ = "indicator_summaries" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) group_name: Mapped[str] = mapped_column(String(64), nullable=False) generated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow) model: Mapped[str] = mapped_column(String(64), nullable=False) @@ -135,7 +142,7 @@ class IndicatorSummary(Base): class AICall(Base): """Cost ledger for OpenRouter calls. Feeds the monthly cap check.""" __tablename__ = "ai_calls" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) called_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow, index=True) model: Mapped[str] = mapped_column(String(64), nullable=False) prompt_tokens: Mapped[int | None] = mapped_column(Integer) @@ -224,7 +231,7 @@ class Referral(Base): user makes their first paid subscription — Phase D.3 fills them in via the Paddle webhook.""" __tablename__ = "referrals" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) referrer_user_id: Mapped[int] = mapped_column( ForeignKey("users.id", ondelete="CASCADE"), nullable=False, ) @@ -246,7 +253,7 @@ class EmailOTP(Base): sent in the email; we store an argon2 hash, expiry, attempt count, and a used_at timestamp so a single code can't be reused or brute-forced.""" __tablename__ = "email_otps" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) email: Mapped[str] = mapped_column(String(255), nullable=False) code_hash: Mapped[str] = mapped_column(String(255), nullable=False) created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow) @@ -267,7 +274,7 @@ class InstrumentMap(Base): Multiple rows can share a shortName (e.g. SHEL on LSE in GBX vs SHEL on NYSE in USD); the resolver picks the right one per user.""" __tablename__ = "instrument_map" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) t212_ticker: Mapped[str] = mapped_column(String(64), nullable=False) t212_shortname: Mapped[str] = mapped_column(String(32), nullable=False) yahoo_ticker: Mapped[str | None] = mapped_column(String(32)) @@ -309,7 +316,7 @@ class TickerUniverse(Base): class JobRun(Base): """One row per scheduled-job invocation; powers /api/health + the ops footer.""" __tablename__ = "job_runs" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) name: Mapped[str] = mapped_column(String(64), nullable=False) started_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow) finished_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) @@ -326,7 +333,7 @@ class EmailSend(Base): Settings page.""" __tablename__ = "email_sends" - id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True) + id: Mapped[int] = mapped_column(_PK, primary_key=True, autoincrement=True) user_id: Mapped[int] = mapped_column( ForeignKey("users.id", ondelete="CASCADE"), nullable=False, ) diff --git a/app/routers/api.py b/app/routers/api.py index df8ce4b..f721716 100644 --- a/app/routers/api.py +++ b/app/routers/api.py @@ -37,6 +37,7 @@ from app.models import ( JobRun, Quote, StrategicLog, + User, ) from app.schemas import ( HealthOut, @@ -837,7 +838,14 @@ async def patch_digest_prefs( if principal.user is None: # Admin bearer-token path — no per-user row to persist to. raise HTTPException(status_code=400, detail="no_user_context") - principal.user.email_digest_opt_in = payload.opt_in - principal.user.digest_tone = payload.tone + # require_token loads `principal.user` in its own short-lived session. + # By the time this handler runs, that session is closed; mutating the + # detached object and committing via `session` would persist nothing. + # Re-fetch in the active session before writing. + user = await session.get(User, principal.user.id) + if user is None: + raise HTTPException(status_code=404, detail="user_not_found") + user.email_digest_opt_in = payload.opt_in + user.digest_tone = payload.tone await session.commit() return DigestPrefsOut(opt_in=payload.opt_in, tone=payload.tone) diff --git a/tests/test_email_digest_job.py b/tests/test_email_digest_job.py index c8611a1..191d283 100644 --- a/tests/test_email_digest_job.py +++ b/tests/test_email_digest_job.py @@ -58,6 +58,7 @@ def test_daily_run_only_paid_opt_in(tmp_path): return_value=_patch_today(0)), \ patch("app.jobs.email_digest_job.send_email", new=AsyncMock()) as send_mock, \ + patch("app.jobs.email_digest_job.llm_configured", return_value=True), \ patch("app.jobs.email_digest_job.call_llm", new=AsyncMock(side_effect=_stub_generate())): asyncio.run(email_digest_job.run()) @@ -72,6 +73,7 @@ def test_weekly_run_includes_free_and_paid_opt_in(tmp_path): return_value=_patch_today(6)), \ patch("app.jobs.email_digest_job.send_email", new=AsyncMock()) as send_mock, \ + patch("app.jobs.email_digest_job.llm_configured", return_value=True), \ patch("app.jobs.email_digest_job.call_llm", new=AsyncMock(side_effect=_stub_generate())): asyncio.run(email_digest_job.run()) @@ -86,6 +88,7 @@ def test_second_run_same_day_is_idempotent(tmp_path): return_value=_patch_today(0)), \ patch("app.jobs.email_digest_job.send_email", new=AsyncMock()) as send_mock, \ + patch("app.jobs.email_digest_job.llm_configured", return_value=True), \ patch("app.jobs.email_digest_job.call_llm", new=AsyncMock(side_effect=_stub_generate())): asyncio.run(email_digest_job.run()) diff --git a/tests/test_news_window.py b/tests/test_news_window.py index ddaa0fa..b58f644 100644 --- a/tests/test_news_window.py +++ b/tests/test_news_window.py @@ -41,6 +41,7 @@ def _build_app(tmp_path): category="general", published_at=now - timedelta(hours=hours_old), fetched_at=now, + fingerprint=f"fp-{title}", tags=[], )) await s.commit()