test+fix: make the suite run cleanly in the test container
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).
This commit is contained in:
parent
80e2ec53ac
commit
a113a7f3ce
6 changed files with 51 additions and 25 deletions
|
|
@ -53,7 +53,9 @@ COPY pyproject.toml ./
|
||||||
COPY app ./app
|
COPY app ./app
|
||||||
COPY alembic ./alembic
|
COPY alembic ./alembic
|
||||||
COPY alembic.ini ./
|
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]"
|
RUN /opt/venv/bin/pip install ".[dev]"
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -23,17 +23,21 @@ async def job_lifecycle(name: str) -> AsyncIterator[tuple[AsyncSession, JobRun]]
|
||||||
handles the bookkeeping.
|
handles the bookkeeping.
|
||||||
|
|
||||||
A MariaDB GET_LOCK(name, 0) is acquired to prevent concurrent runs of the
|
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()
|
factory = get_session_factory()
|
||||||
async with factory() as session:
|
async with factory() as session:
|
||||||
# Try lock; skip if held.
|
bind = session.get_bind()
|
||||||
got = (await session.execute(
|
use_lock = bind is not None and bind.dialect.name == "mysql"
|
||||||
text("SELECT GET_LOCK(:n, 0)"), {"n": f"cassandra_{name}"}
|
if use_lock:
|
||||||
)).scalar()
|
got = (await session.execute(
|
||||||
if not got:
|
text("SELECT GET_LOCK(:n, 0)"), {"n": f"cassandra_{name}"}
|
||||||
log.warning("job.skipped_locked", name=name)
|
)).scalar()
|
||||||
yield session, JobRun(name=name, started_at=utcnow(), status="skipped")
|
if not got:
|
||||||
return
|
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")
|
run = JobRun(name=name, started_at=utcnow(), status="running")
|
||||||
session.add(run)
|
session.add(run)
|
||||||
await session.commit()
|
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))
|
log.error("job.failed", name=name, error=str(e))
|
||||||
raise
|
raise
|
||||||
finally:
|
finally:
|
||||||
await session.execute(text("SELECT RELEASE_LOCK(:n)"),
|
if use_lock:
|
||||||
{"n": f"cassandra_{name}"})
|
await session.execute(text("SELECT RELEASE_LOCK(:n)"),
|
||||||
await session.commit()
|
{"n": f"cassandra_{name}"})
|
||||||
|
await session.commit()
|
||||||
|
|
|
||||||
|
|
@ -29,9 +29,16 @@ from sqlalchemy.orm import Mapped, mapped_column, relationship
|
||||||
from app.db import Base, utcnow
|
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):
|
class Quote(Base):
|
||||||
__tablename__ = "quotes"
|
__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)
|
symbol: Mapped[str] = mapped_column(String(128), nullable=False)
|
||||||
source: Mapped[str] = mapped_column(String(32), nullable=False)
|
source: Mapped[str] = mapped_column(String(32), nullable=False)
|
||||||
label: Mapped[str] = mapped_column(String(128), default="")
|
label: Mapped[str] = mapped_column(String(128), default="")
|
||||||
|
|
@ -62,7 +69,7 @@ class QuoteDaily(Base):
|
||||||
|
|
||||||
class Headline(Base):
|
class Headline(Base):
|
||||||
__tablename__ = "headlines"
|
__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)
|
source: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
category: Mapped[str] = mapped_column(String(32), nullable=False)
|
category: Mapped[str] = mapped_column(String(32), nullable=False)
|
||||||
title: Mapped[str] = mapped_column(String(512), nullable=False)
|
title: Mapped[str] = mapped_column(String(512), nullable=False)
|
||||||
|
|
@ -100,7 +107,7 @@ class Feed(Base):
|
||||||
|
|
||||||
class StrategicLog(Base):
|
class StrategicLog(Base):
|
||||||
__tablename__ = "strategic_logs"
|
__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)
|
generated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow, index=True)
|
||||||
model: Mapped[str] = mapped_column(String(64), nullable=False)
|
model: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
anchor_date: Mapped[str | None] = mapped_column(String(16))
|
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.
|
"""Short AI-generated read for one indicator group, regenerated hourly.
|
||||||
The latest row per group_name is what the dashboard renders."""
|
The latest row per group_name is what the dashboard renders."""
|
||||||
__tablename__ = "indicator_summaries"
|
__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)
|
group_name: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
generated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow)
|
generated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow)
|
||||||
model: Mapped[str] = mapped_column(String(64), nullable=False)
|
model: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
|
|
@ -135,7 +142,7 @@ class IndicatorSummary(Base):
|
||||||
class AICall(Base):
|
class AICall(Base):
|
||||||
"""Cost ledger for OpenRouter calls. Feeds the monthly cap check."""
|
"""Cost ledger for OpenRouter calls. Feeds the monthly cap check."""
|
||||||
__tablename__ = "ai_calls"
|
__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)
|
called_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow, index=True)
|
||||||
model: Mapped[str] = mapped_column(String(64), nullable=False)
|
model: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
prompt_tokens: Mapped[int | None] = mapped_column(Integer)
|
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
|
user makes their first paid subscription — Phase D.3 fills them in
|
||||||
via the Paddle webhook."""
|
via the Paddle webhook."""
|
||||||
__tablename__ = "referrals"
|
__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(
|
referrer_user_id: Mapped[int] = mapped_column(
|
||||||
ForeignKey("users.id", ondelete="CASCADE"), nullable=False,
|
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
|
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."""
|
a used_at timestamp so a single code can't be reused or brute-forced."""
|
||||||
__tablename__ = "email_otps"
|
__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)
|
email: Mapped[str] = mapped_column(String(255), nullable=False)
|
||||||
code_hash: 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)
|
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
|
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."""
|
SHEL on NYSE in USD); the resolver picks the right one per user."""
|
||||||
__tablename__ = "instrument_map"
|
__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_ticker: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
t212_shortname: Mapped[str] = mapped_column(String(32), nullable=False)
|
t212_shortname: Mapped[str] = mapped_column(String(32), nullable=False)
|
||||||
yahoo_ticker: Mapped[str | None] = mapped_column(String(32))
|
yahoo_ticker: Mapped[str | None] = mapped_column(String(32))
|
||||||
|
|
@ -309,7 +316,7 @@ class TickerUniverse(Base):
|
||||||
class JobRun(Base):
|
class JobRun(Base):
|
||||||
"""One row per scheduled-job invocation; powers /api/health + the ops footer."""
|
"""One row per scheduled-job invocation; powers /api/health + the ops footer."""
|
||||||
__tablename__ = "job_runs"
|
__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)
|
name: Mapped[str] = mapped_column(String(64), nullable=False)
|
||||||
started_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow)
|
started_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=utcnow)
|
||||||
finished_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True))
|
finished_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True))
|
||||||
|
|
@ -326,7 +333,7 @@ class EmailSend(Base):
|
||||||
Settings page."""
|
Settings page."""
|
||||||
__tablename__ = "email_sends"
|
__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(
|
user_id: Mapped[int] = mapped_column(
|
||||||
ForeignKey("users.id", ondelete="CASCADE"), nullable=False,
|
ForeignKey("users.id", ondelete="CASCADE"), nullable=False,
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -37,6 +37,7 @@ from app.models import (
|
||||||
JobRun,
|
JobRun,
|
||||||
Quote,
|
Quote,
|
||||||
StrategicLog,
|
StrategicLog,
|
||||||
|
User,
|
||||||
)
|
)
|
||||||
from app.schemas import (
|
from app.schemas import (
|
||||||
HealthOut,
|
HealthOut,
|
||||||
|
|
@ -837,7 +838,14 @@ async def patch_digest_prefs(
|
||||||
if principal.user is None:
|
if principal.user is None:
|
||||||
# Admin bearer-token path — no per-user row to persist to.
|
# Admin bearer-token path — no per-user row to persist to.
|
||||||
raise HTTPException(status_code=400, detail="no_user_context")
|
raise HTTPException(status_code=400, detail="no_user_context")
|
||||||
principal.user.email_digest_opt_in = payload.opt_in
|
# require_token loads `principal.user` in its own short-lived session.
|
||||||
principal.user.digest_tone = payload.tone
|
# 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()
|
await session.commit()
|
||||||
return DigestPrefsOut(opt_in=payload.opt_in, tone=payload.tone)
|
return DigestPrefsOut(opt_in=payload.opt_in, tone=payload.tone)
|
||||||
|
|
|
||||||
|
|
@ -58,6 +58,7 @@ def test_daily_run_only_paid_opt_in(tmp_path):
|
||||||
return_value=_patch_today(0)), \
|
return_value=_patch_today(0)), \
|
||||||
patch("app.jobs.email_digest_job.send_email",
|
patch("app.jobs.email_digest_job.send_email",
|
||||||
new=AsyncMock()) as send_mock, \
|
new=AsyncMock()) as send_mock, \
|
||||||
|
patch("app.jobs.email_digest_job.llm_configured", return_value=True), \
|
||||||
patch("app.jobs.email_digest_job.call_llm",
|
patch("app.jobs.email_digest_job.call_llm",
|
||||||
new=AsyncMock(side_effect=_stub_generate())):
|
new=AsyncMock(side_effect=_stub_generate())):
|
||||||
asyncio.run(email_digest_job.run())
|
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)), \
|
return_value=_patch_today(6)), \
|
||||||
patch("app.jobs.email_digest_job.send_email",
|
patch("app.jobs.email_digest_job.send_email",
|
||||||
new=AsyncMock()) as send_mock, \
|
new=AsyncMock()) as send_mock, \
|
||||||
|
patch("app.jobs.email_digest_job.llm_configured", return_value=True), \
|
||||||
patch("app.jobs.email_digest_job.call_llm",
|
patch("app.jobs.email_digest_job.call_llm",
|
||||||
new=AsyncMock(side_effect=_stub_generate())):
|
new=AsyncMock(side_effect=_stub_generate())):
|
||||||
asyncio.run(email_digest_job.run())
|
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)), \
|
return_value=_patch_today(0)), \
|
||||||
patch("app.jobs.email_digest_job.send_email",
|
patch("app.jobs.email_digest_job.send_email",
|
||||||
new=AsyncMock()) as send_mock, \
|
new=AsyncMock()) as send_mock, \
|
||||||
|
patch("app.jobs.email_digest_job.llm_configured", return_value=True), \
|
||||||
patch("app.jobs.email_digest_job.call_llm",
|
patch("app.jobs.email_digest_job.call_llm",
|
||||||
new=AsyncMock(side_effect=_stub_generate())):
|
new=AsyncMock(side_effect=_stub_generate())):
|
||||||
asyncio.run(email_digest_job.run())
|
asyncio.run(email_digest_job.run())
|
||||||
|
|
|
||||||
|
|
@ -41,6 +41,7 @@ def _build_app(tmp_path):
|
||||||
category="general",
|
category="general",
|
||||||
published_at=now - timedelta(hours=hours_old),
|
published_at=now - timedelta(hours=hours_old),
|
||||||
fetched_at=now,
|
fetched_at=now,
|
||||||
|
fingerprint=f"fp-{title}",
|
||||||
tags=[],
|
tags=[],
|
||||||
))
|
))
|
||||||
await s.commit()
|
await s.commit()
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue