diff --git a/.forgejo/workflows/ci.yml b/.forgejo/workflows/ci.yml index 575b1f0..1899bc3 100644 --- a/.forgejo/workflows/ci.yml +++ b/.forgejo/workflows/ci.yml @@ -184,7 +184,80 @@ jobs: run: uv pip install --python /opt/venv/bin/python -e ".[dev]" - name: Run tests - run: /opt/venv/bin/python -m pytest tests/ -q + # DB-free by design. Anything needing a real Postgres is marked `integration` + # and runs in the job below. + run: /opt/venv/bin/python -m pytest tests/ -q -m "not integration" + + # Real-Postgres lane (family rule 6). Until this existed, `alembic upgrade head` ran + # for the first time when the operator's container started — 26 revisions, none of + # them ever executed by CI — and the schema the migrations build had never been + # checked against the models that read it. + # + # Runs for visibility and does NOT gate the build, matching the `test` lane and + # FabledScribe's equivalent job. + # + # Job key stays separator-free ("integration") with no `name:` — rule 80. act_runner + # derives the service-container name from the truncated job display name, and the + # discovery step below filters `docker ps` by it. Service hostnames are not routable + # on this runner (rule 79), so the step resolves the container's bridge IP. + integration: + if: github.ref == 'refs/heads/dev' || github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v') + runs-on: python-ci + container: + image: git.fabledsword.com/bvandeusen/ci-python:3.14 + services: + postgres: + # Same image the production compose runs, so the schema is proven against the + # Postgres it will actually meet. + image: postgres:16-alpine + env: + POSTGRES_USER: thoughtsync + POSTGRES_PASSWORD: ci_integration + POSTGRES_DB: thoughtsync_test + options: >- + --health-cmd "pg_isready -U thoughtsync" + --health-interval 10s + --health-timeout 5s + --health-retries 10 + steps: + - uses: actions/checkout@v6 + + - name: Create virtual environment + run: uv venv /opt/venv + + # Same install as the unit lane — the two must agree on versions, or + # "unit green, integration red" stops being a signal about the code. + - name: Install package with dev deps + run: uv pip install --python /opt/venv/bin/python -e ".[dev]" + + - name: Integration suite (resolve service IP, migrate, test) + run: | + set -eux + echo "=== container landscape (diagnostic for the name filter) ===" + docker ps -a --format '{{.ID}} {{.Image}} -> {{.Names}}' + PG=$(docker ps --filter "name=integration" --filter "ancestor=postgres:16-alpine" -q | head -n1) + test -n "$PG" + PG_IP=$(docker inspect -f '{{range .NetworkSettings.Networks}}{{.IPAddress}}{{end}}' "$PG") + test -n "$PG_IP" + export THOUGHTSYNC_DATABASE_URL="postgresql+asyncpg://thoughtsync:ci_integration@${PG_IP}:5432/thoughtsync_test" + # Wait for Postgres to accept connections. `run:` is busybox sh (rule 81) — + # no bash /dev/tcp — so use the Python that is always present here. + /opt/venv/bin/python - "$PG_IP" <<'PY' + import socket, sys, time + for _ in range(30): + try: + socket.create_connection((sys.argv[1], 5432), timeout=2).close() + break + except OSError: + time.sleep(1) + else: + sys.exit("postgres did not become reachable") + PY + # Real migrations build the schema, never metadata.create_all (rule 82) — + # testing a schema no deployment has ever seen would prove nothing. This + # step IS the migration test: a broken revision fails the job here. + /opt/venv/bin/alembic upgrade head + /opt/venv/bin/python -m pytest tests/ -v -m integration build: name: Build & push image diff --git a/ci-requirements.md b/ci-requirements.md index e9e9568..c458076 100644 --- a/ci-requirements.md +++ b/ci-requirements.md @@ -73,6 +73,43 @@ entirely on `ci-python:3.14`. `…/actions/artifacts/{id}/zip`. Note the workstation has no `unzip` — use `python3 -m zipfile -e`. +## The integration lane + +Added 2026-08-23. Before it, `alembic upgrade head` ran for the first time when the +operator's container started — 26 revisions, none of them ever executed by CI — and +the schema the migrations build had never been checked against the models that read +it. M13 dropped three columns and rebuilt a STORED GENERATED column with nothing +watching. + +Copied from FabledScribe's `integration` job, which had already solved the awkward +parts. Three of them are family rules for a reason: + +- **Job key `integration`, no `name:`** (rule 80). act_runner derives the service + container's name from the truncated job DISPLAY name, and the discovery step filters + `docker ps` by it. A spaced or underscored name breaks the filter. +- **Service hostnames are not routable** on this runner (rule 79), so the step resolves + the Postgres container's bridge IP with `docker ps --filter` + `docker inspect` and + builds `THOUGHTSYNC_DATABASE_URL` from it. `postgres:5432` will not connect. +- **`run:` is busybox sh** (rule 81) — no `/dev/tcp` — so the readiness wait is a small + Python heredoc. Its terminator must dedent to column 0 after YAML strips the block + indent; check with `yaml.safe_load` and print the `run` string if you edit it. + +`postgres:16-alpine`, matching the production compose, so the schema is proven against +the Postgres it will actually meet. The schema comes from **real migrations, never +`metadata.create_all`** (rule 82): testing a schema no deployment has ever seen proves +nothing, and that `alembic upgrade head` step IS the migration test — a broken revision +fails the job there, before it can fail a container start. + +Tests are marked `integration` (registered in `pyproject.toml`); the unit lane runs +`-m "not integration"` and stays DB-free. Data resets with `TRUNCATE ... CASCADE` +BEFORE each test rather than after, so a failure leaves its rows behind to look at. + +Like `test`, it runs for visibility and does **not** gate the build. + +There is no local way to run it — that would mean standing up Postgres on the +workstation, which rule 12 reserves for an explicit request. This lane is verified in +CI. + ## Desktop (Tauri) lane — separate workflow The Tauri desktop client (`desktop/`) builds in its own workflow, diff --git a/pyproject.toml b/pyproject.toml index 8f61eac..87046c7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,6 +29,12 @@ where = ["src"] [tool.pytest.ini_options] asyncio_mode = "auto" testpaths = ["tests"] +# The unit lane runs `-m "not integration"`; the integration lane runs `-m integration` +# against a real Postgres. Registered here so an unmarked typo fails loudly instead of +# quietly landing a test in neither lane. +markers = [ + "integration: needs a live Postgres — CI's integration job, not the unit lane", +] [tool.ruff] line-length = 120 diff --git a/tests/test_integration.py b/tests/test_integration.py new file mode 100644 index 0000000..cc02f4e --- /dev/null +++ b/tests/test_integration.py @@ -0,0 +1,203 @@ +"""The real-Postgres lane (family rule 6). + +Everything else in this suite is deliberately DB-free, which means the schema the +migrations build has never been checked against the models that read it. That gap is +what this file closes, and it is not theoretical: M13 dropped three columns and +rebuilt a generated column, and until now `alembic upgrade head` ran for the first +time when the operator's container started. + +Marked `integration` and excluded from the unit lane by `-m "not integration"`, so a +workstation without Postgres runs the rest of the suite unchanged. + +The schema comes from real migrations, never `metadata.create_all` (rule 82) — the +point is to test what actually ships, and `create_all` would build a schema no +deployment has ever seen. +""" +from __future__ import annotations + +import uuid + +import pytest +import pytest_asyncio +from sqlalchemy import select, text + +from thoughtsync.db import dispose_engine, session_scope +from thoughtsync.models.note import Note +from thoughtsync.models.note_item import NoteItem +from thoughtsync.models.user import User +from thoughtsync.notes.helpers import derive_display_title +from thoughtsync.sync import _apply_note_items + +pytestmark = pytest.mark.integration + +# Every table the tests touch, child-first so FKs never block the truncate. +# RESTART IDENTITY + CASCADE keeps this honest if a table gains children later. +_TABLES = "notes, note_items, note_revisions, note_labels, labels, users" + + +@pytest_asyncio.fixture +async def db(): + """A session against the migrated database, wiped before each test. + + Wiped BEFORE rather than after so a failed test leaves its rows behind to look at. + """ + async with session_scope() as session: + await session.execute(text(f"TRUNCATE {_TABLES} RESTART IDENTITY CASCADE")) + await session.commit() + yield session + await dispose_engine() + + +@pytest_asyncio.fixture +async def owner(db): + """A user to hang notes off — `notes.owner_id` is a real foreign key.""" + user = User(email=f"{uuid.uuid4().hex}@example.test", display_name="Integration") + db.add(user) + await db.commit() + await db.refresh(user) + return user + + +async def test_the_migrated_schema_matches_the_models(db, owner): + """The check that has never run: insert through the ORM, read it back. + + A column the models expect and the migrations never created — or the reverse — + fails right here, instead of when a container starts. + """ + note = Note(owner_id=owner.id, body="a thought", display_title="a thought") + db.add(note) + await db.commit() + await db.refresh(note) + + found = await db.scalar(select(Note).where(Note.id == note.id)) + assert found is not None + assert found.body == "a thought" + assert found.display_title == "a thought" + + +async def test_the_dropped_columns_are_actually_gone(db): + """M13 dropped three. If a migration silently no-opped, this is where it shows.""" + cols = set( + ( + await db.execute( + text("SELECT column_name FROM information_schema.columns WHERE table_name = 'notes'") + ) + ) + .scalars() + .all() + ) + assert "title" not in cols, "notes.title should have gone in 0026" + assert "kind" not in cols, "notes.kind should have gone in 0025" + assert "display_title" in cols and "body" in cols + + rev_cols = set( + ( + await db.execute( + text("SELECT column_name FROM information_schema.columns WHERE table_name = 'note_revisions'") + ) + ) + .scalars() + .all() + ) + assert "title" not in rev_cols, "note_revisions.title should have gone in 0026" + + tables = set( + (await db.execute(text("SELECT table_name FROM information_schema.tables WHERE table_schema = 'public'"))) + .scalars() + .all() + ) + assert "note_links" not in tables, "note_links should have gone in 0024" + + +async def test_the_search_vector_was_rebuilt_over_the_name(db, owner): + """0026 had to drop and recreate a STORED GENERATED column. + + Postgres refuses to drop a column another generated column depends on, so getting + this wrong doesn't produce a subtly wrong ranking — it produces a migration that + won't run at all. Worth proving the replacement actually indexes something. + """ + note = Note(owner_id=owner.id, body="ferry tickets\nbook before friday", display_title="ferry tickets") + db.add(note) + await db.commit() + + hit = await db.scalar( + text( + "SELECT count(*) FROM notes " + "WHERE search_vector @@ websearch_to_tsquery('english', :q)" + ).bindparams(q="ferry") + ) + assert hit == 1 + + # The NAME is weight A and the body weight B, which is what makes a name match + # rank above a body-only one. Both must be in the vector at all. + body_only = await db.scalar( + text( + "SELECT count(*) FROM notes " + "WHERE search_vector @@ websearch_to_tsquery('english', :q)" + ).bindparams(q="friday") + ) + assert body_only == 1 + + +async def test_a_note_keeps_both_its_body_and_its_items(db, owner): + """The shape M13 step 2 made normal: a note HAS a checklist, it isn't one.""" + note = Note(owner_id=owner.id, body="weekend shop", display_title="weekend shop") + db.add(note) + await db.flush() + db.add_all( + [ + NoteItem(note_id=note.id, text="milk", position=0), + NoteItem(note_id=note.id, text="eggs", position=1), + ] + ) + await db.commit() + + items = ( + await db.scalars(select(NoteItem).where(NoteItem.note_id == note.id).order_by(NoteItem.position)) + ).all() + assert [i.text for i in items] == ["milk", "eggs"] + assert (await db.scalar(select(Note.body).where(Note.id == note.id))) == "weekend shop" + + +async def test_sync_no_longer_deletes_items_from_a_note_with_a_body(db, owner): + """The data-loss path step 2 removed, pinned against a real database. + + `_apply_note_items` used to delete every item when the note wasn't `kind = "list"`. + Nothing can produce that state any more, but this is the regression that would + have silently eaten a checklist, and it deserves a test that would catch its + return. + """ + note = Note(owner_id=owner.id, body="packing", display_title="packing") + db.add(note) + await db.flush() + db.add(NoteItem(note_id=note.id, text="socks", position=0)) + await db.commit() + + # A change that says nothing about items must LEAVE them alone — absent means + # "not telling us", not "empty". + await _apply_note_items(db, note, {"body": "packing"}) + await db.commit() + assert (await db.scalar(select(NoteItem.text).where(NoteItem.note_id == note.id))) == "socks" + + # An explicit list replaces them. + await _apply_note_items(db, note, {"items": [{"text": "charger", "checked": True}]}) + await db.commit() + rows = (await db.scalars(select(NoteItem).where(NoteItem.note_id == note.id))).all() + assert [(r.text, r.checked) for r in rows] == [("charger", True)] + + +async def test_a_note_with_only_items_still_has_a_name(db, owner): + """The hole that made removing the title unsafe until step 2 closed it.""" + note = Note(owner_id=owner.id, body="", display_title="") + db.add(note) + await db.flush() + db.add(NoteItem(note_id=note.id, text="milk", position=0)) + await db.commit() + + first = await db.scalar( + select(NoteItem.text).where(NoteItem.note_id == note.id).order_by(NoteItem.position).limit(1) + ) + note.display_title = derive_display_title(note.body, first) + await db.commit() + + assert (await db.scalar(select(Note.display_title).where(Note.id == note.id))) == "milk"