From 1e289e736111d89a7ef0632977ea4402780b483f Mon Sep 17 00:00:00 2001 From: Karolis Strazdas Date: Thu, 29 Oct 2026 14:07:57 +0200 Subject: [PATCH 1/2] test: reuse database schema and isolate migration fixtures --- .github/workflows/ci.yml | 2 +- AGENTS.md | 4 +- pyproject.toml | 1 + tests/api/routes/test_auth_sandbox.py | 12 +-- tests/api/routes/test_health.py | 12 +-- tests/api/routes/test_powersync_sandbox.py | 28 +++--- tests/conftest.py | 95 +++++++++++++++++-- tests/services/test_auth.py | 33 +------ tests/services/test_sync.py | 17 ---- tests/test_account_deletion_migration.py | 2 + tests/test_acquisition_migration.py | 102 ++++++++++++--------- tests/test_bookmark_migration.py | 3 + tests/test_database.py | 48 ++++++++++ tests/test_library_migration.py | 3 + tests/test_sync_schema.py | 18 ++++ tests/test_tracking_migration.py | 3 + 16 files changed, 257 insertions(+), 126 deletions(-) create mode 100644 tests/test_sync_schema.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e877020..52abf93 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,4 +65,4 @@ jobs: run: uv run mypy . - name: Pytest - run: uv run pytest + run: uv run pytest --durations=20 diff --git a/AGENTS.md b/AGENTS.md index 6b2734f..33f2c50 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,6 +57,8 @@ maintainer's reviewed examples. The rules below also apply in standalone checkou - Add or update tests for every behavior change. - Prefer route tests for HTTP contract changes and service tests for business logic. - If database behavior changes, add the narrowest regression test that proves the query or transaction behavior. +- Mark schema-changing tests with `pytest.mark.migration`. Ordinary tests share the schema and truncate rows after each case; migration tests rebuild it before and after each case, including failures. +- Use `public_client` or `public_debug_client` for routes that do not access the database. These fixtures reject accidental database access. - Do not claim success without running the most relevant checks, or explicitly stating why they were not run. ## Migration Rules @@ -102,7 +104,7 @@ When checked out inside the Papyrus workspace, run from its root: - `tools/papyrus check server` runs Ruff lint, non-mutating format checks, and Mypy. - `tools/papyrus test server -- tests/services/test_sync.py` runs focused tests. -The shared fixtures drop and recreate test tables. Most route tests need PostgreSQL +The shared fixtures truncate test rows and rebuild tables for migration tests. Most route tests need PostgreSQL without being marked `integration`. Use a separate local test database, never the application database; do not run suites concurrently on the same database. The CLI checks the database target and holds a workspace test lock. External `auth_smoke` diff --git a/pyproject.toml b/pyproject.toml index 56d44e8..5a07995 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -53,6 +53,7 @@ addopts = "-v --tb=short" markers = [ "integration: tests that require a running database", "auth_smoke: provider-backed authentication smoke tests", + "migration: tests that alter the database schema and require a full rebuild", ] [tool.ruff] diff --git a/tests/api/routes/test_auth_sandbox.py b/tests/api/routes/test_auth_sandbox.py index 073d117..e295f0d 100644 --- a/tests/api/routes/test_auth_sandbox.py +++ b/tests/api/routes/test_auth_sandbox.py @@ -8,20 +8,20 @@ from papyrus.main import settings as app_settings -async def test_auth_sandbox_not_registered_in_production_mode(prod_client: AsyncClient): +async def test_auth_sandbox_not_registered_in_production_mode(public_client: AsyncClient): """Test the auth sandbox is absent when debug mode is disabled.""" - response = await prod_client.get("/__dev/auth-sandbox") + response = await public_client.get("/__dev/auth-sandbox") assert response.status_code == 404 async def test_auth_sandbox_registered_in_debug_vite_mode( - debug_client: AsyncClient, + public_debug_client: AsyncClient, monkeypatch, ): """Test the auth sandbox page points at the Vite dev server when enabled.""" monkeypatch.setattr(app_settings, "dev_pages_use_vite", True) monkeypatch.setattr(app_settings, "dev_pages_vite_url", "http://vite.test:5173") - response = await debug_client.get("/__dev/auth-sandbox") + response = await public_debug_client.get("/__dev/auth-sandbox") assert response.status_code == 200 assert "Authentication sandbox" in response.text assert "window.__PAPYRUS_DEV_PAGE_CONFIG__ =" in response.text @@ -38,7 +38,7 @@ async def test_auth_sandbox_registered_in_debug_vite_mode( async def test_auth_sandbox_renders_built_assets_when_manifest_exists( - debug_client: AsyncClient, + public_debug_client: AsyncClient, monkeypatch, tmp_path, ): @@ -62,7 +62,7 @@ async def test_auth_sandbox_renders_built_assets_when_manifest_exists( monkeypatch.setattr(app_settings, "dev_pages_use_vite", False) monkeypatch.setattr(app_settings, "dev_pages_manifest_path", str(manifest_path)) dev_pages._load_manifest.cache_clear() - response = await debug_client.get("/__dev/auth-sandbox") + response = await public_debug_client.get("/__dev/auth-sandbox") assert response.status_code == 200 assert 'href="/__dev/static/assets/auth-sandbox.css"' in response.text assert 'src="/__dev/static/assets/auth-sandbox.js"' in response.text diff --git a/tests/api/routes/test_health.py b/tests/api/routes/test_health.py index 9a018be..01204fa 100644 --- a/tests/api/routes/test_health.py +++ b/tests/api/routes/test_health.py @@ -3,9 +3,9 @@ from httpx import AsyncClient -async def test_index_lists_available_pages(prod_client: AsyncClient): +async def test_index_lists_available_pages(public_client: AsyncClient): """Test root index returns the available page endpoints.""" - response = await prod_client.get("/") + response = await public_client.get("/") assert response.status_code == 200 data = response.json() pages = {page["name"]: page["path"] for page in data["pages"]} @@ -16,9 +16,9 @@ async def test_index_lists_available_pages(prod_client: AsyncClient): assert "auth_sandbox" not in pages -async def test_index_lists_debug_pages(debug_client: AsyncClient): +async def test_index_lists_debug_pages(public_debug_client: AsyncClient): """Test root index includes the debug sandboxes in debug mode.""" - response = await debug_client.get("/") + response = await public_debug_client.get("/") assert response.status_code == 200 data = response.json() pages = {page["name"]: page["path"] for page in data["pages"]} @@ -26,9 +26,9 @@ async def test_index_lists_debug_pages(debug_client: AsyncClient): assert pages["powersync_sandbox"] == "http://localhost:8080/__dev/powersync-sandbox" -async def test_health_check(client: AsyncClient): +async def test_health_check(public_client: AsyncClient): """Test health check endpoint.""" - response = await client.get("/health") + response = await public_client.get("/health") assert response.status_code == 200 data = response.json() assert data["status"] == "healthy" diff --git a/tests/api/routes/test_powersync_sandbox.py b/tests/api/routes/test_powersync_sandbox.py index 69f1063..d7088a7 100644 --- a/tests/api/routes/test_powersync_sandbox.py +++ b/tests/api/routes/test_powersync_sandbox.py @@ -37,20 +37,20 @@ async def _seed_demo_item( return item -async def test_powersync_sandbox_not_registered_in_production_mode(prod_client: AsyncClient): +async def test_powersync_sandbox_not_registered_in_production_mode(public_client: AsyncClient): """The PowerSync sandbox page is absent when debug mode is disabled.""" - response = await prod_client.get("/__dev/powersync-sandbox") + response = await public_client.get("/__dev/powersync-sandbox") assert response.status_code == 404 async def test_powersync_sandbox_registered_in_debug_vite_mode( - debug_client: AsyncClient, + public_debug_client: AsyncClient, monkeypatch, ): """The PowerSync sandbox page uses Vite-served assets when configured.""" monkeypatch.setattr(app_settings, "dev_pages_use_vite", True) monkeypatch.setattr(app_settings, "dev_pages_vite_url", "http://vite.test:5173") - response = await debug_client.get("/__dev/powersync-sandbox") + response = await public_debug_client.get("/__dev/powersync-sandbox") assert response.status_code == 200 assert "PowerSync Sandbox" in response.text assert 'data-dev-page="powersync-sandbox"' in response.text @@ -66,7 +66,7 @@ async def test_powersync_sandbox_registered_in_debug_vite_mode( async def test_powersync_sandbox_renders_built_assets_when_manifest_exists( - debug_client: AsyncClient, + public_debug_client: AsyncClient, monkeypatch, tmp_path, ): @@ -90,7 +90,7 @@ async def test_powersync_sandbox_renders_built_assets_when_manifest_exists( monkeypatch.setattr(app_settings, "dev_pages_use_vite", False) monkeypatch.setattr(app_settings, "dev_pages_manifest_path", str(manifest_path)) dev_pages._load_manifest.cache_clear() - response = await debug_client.get("/__dev/powersync-sandbox") + response = await public_debug_client.get("/__dev/powersync-sandbox") assert response.status_code == 200 assert 'href="/__dev/static/assets/powersync-sandbox.css"' in response.text assert 'src="/__dev/static/assets/powersync-sandbox.js"' in response.text @@ -98,9 +98,9 @@ async def test_powersync_sandbox_renders_built_assets_when_manifest_exists( assert "@vite/client" not in response.text -async def test_powersync_sandbox_config_returns_expected_urls(debug_client: AsyncClient): +async def test_powersync_sandbox_config_returns_expected_urls(public_debug_client: AsyncClient): """The sandbox config endpoint exposes the browser and backend integration URLs.""" - response = await debug_client.get("/__dev/powersync-sandbox/config?client=Two Tabs") + response = await public_debug_client.get("/__dev/powersync-sandbox/config?client=Two Tabs") assert response.status_code == 200 body = response.json() assert body["register_url"] == "/v1/auth/register" @@ -115,7 +115,7 @@ async def test_powersync_sandbox_config_returns_expected_urls(debug_client: Asyn async def test_powersync_sandbox_worker_asset_is_served_from_backend_origin( - debug_client: AsyncClient, + public_debug_client: AsyncClient, monkeypatch, ): """Worker assets and their nested chunks are proxied through the backend origin.""" @@ -125,19 +125,21 @@ async def test_powersync_sandbox_worker_asset_is_served_from_backend_origin( lambda asset_path: f"// {asset_path}".encode(), ) - response = await debug_client.get("/__dev/powersync-sandbox/worker/WASQLiteDB.umd.js") + response = await public_debug_client.get("/__dev/powersync-sandbox/worker/WASQLiteDB.umd.js") assert response.status_code == 200 assert response.headers["content-type"].startswith("text/javascript") assert response.headers["x-papyrus-vendor-path"] == "WASQLiteDB.umd.js" assert response.text == "// worker/WASQLiteDB.umd.js" - nested_response = await debug_client.get("/__dev/powersync-sandbox/worker/node_modules_pnpm_example_chunk.umd.js") + nested_response = await public_debug_client.get( + "/__dev/powersync-sandbox/worker/node_modules_pnpm_example_chunk.umd.js" + ) assert nested_response.status_code == 200 assert nested_response.headers["x-papyrus-vendor-path"] == "node_modules_pnpm_example_chunk.umd.js" assert nested_response.text == "// worker/node_modules_pnpm_example_chunk.umd.js" async def test_powersync_sandbox_root_asset_is_served_from_backend_origin( - debug_client: AsyncClient, + public_debug_client: AsyncClient, monkeypatch, ): """Root dist assets such as WASM binaries are proxied through the backend origin.""" @@ -147,7 +149,7 @@ async def test_powersync_sandbox_root_asset_is_served_from_backend_origin( lambda asset_path: f"// {asset_path}".encode(), ) - response = await debug_client.get("/__dev/powersync-sandbox/ca59e199e1138b553fad.wasm") + response = await public_debug_client.get("/__dev/powersync-sandbox/ca59e199e1138b553fad.wasm") assert response.status_code == 200 assert response.headers["content-type"].startswith("application/wasm") assert response.headers["x-papyrus-vendor-path"] == "ca59e199e1138b553fad.wasm" diff --git a/tests/conftest.py b/tests/conftest.py index d6887dc..9bd40ce 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,6 @@ import os from collections.abc import AsyncGenerator -from contextlib import suppress +from contextlib import asynccontextmanager, suppress from datetime import UTC, datetime, timedelta from uuid import uuid4 @@ -8,8 +8,9 @@ import pytest import pytest_asyncio from httpx import ASGITransport, AsyncClient +from sqlalchemy import text from sqlalchemy.engine import URL, make_url -from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import AsyncEngine, AsyncSession, async_sessionmaker, create_async_engine from papyrus.config import get_settings from papyrus.core.database import get_db @@ -139,17 +140,19 @@ async def setup_test_db() -> None: await connection.close() -@pytest_asyncio.fixture -async def test_session_maker(setup_test_db: None) -> AsyncGenerator[async_sessionmaker[AsyncSession], None]: - engine = create_async_engine(TEST_DATABASE_URL) - session_maker = async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession) - +async def rebuild_schema(engine: AsyncEngine) -> None: async with engine.begin() as conn: await conn.run_sync(Base.metadata.drop_all) await conn.run_sync(Base.metadata.create_all) + +@pytest_asyncio.fixture(scope="session") +async def test_schema(setup_test_db: None) -> AsyncGenerator[None, None]: + engine = create_async_engine(TEST_DATABASE_URL) + try: - yield session_maker + await rebuild_schema(engine) + yield finally: async with engine.begin() as conn: await conn.run_sync(Base.metadata.drop_all) @@ -157,6 +160,74 @@ async def test_session_maker(setup_test_db: None) -> AsyncGenerator[async_sessio await engine.dispose() +@asynccontextmanager +async def isolated_sessions( + engine: AsyncEngine, + *, + migration: bool = False, +) -> AsyncGenerator[async_sessionmaker[AsyncSession], None]: + """Preserve real transactions while removing committed state even after failure.""" + if migration: + await rebuild_schema(engine) + + try: + yield async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession) + finally: + if migration: + await rebuild_schema(engine) + else: + tables = ", ".join(_quote_ident(table.name) for table in Base.metadata.sorted_tables) + + async with engine.begin() as conn: + await conn.execute(text(f"TRUNCATE {tables} RESTART IDENTITY CASCADE")) + + +@pytest_asyncio.fixture +async def test_session_maker( + test_schema: None, + request: pytest.FixtureRequest, +) -> AsyncGenerator[async_sessionmaker[AsyncSession], None]: + engine = create_async_engine(TEST_DATABASE_URL) + + try: + async with isolated_sessions( + engine, migration=request.node.get_closest_marker("migration") is not None + ) as maker: + yield maker + finally: + await engine.dispose() + + +@pytest_asyncio.fixture +async def public_client(monkeypatch: pytest.MonkeyPatch) -> AsyncGenerator[AsyncClient, None]: + """Exercise public routes without opening a database connection.""" + async with public_http_client(monkeypatch, debug=False) as client: + yield client + + +@pytest_asyncio.fixture +async def public_debug_client(monkeypatch: pytest.MonkeyPatch) -> AsyncGenerator[AsyncClient, None]: + async with public_http_client(monkeypatch, debug=True) as client: + yield client + + +@asynccontextmanager +async def public_http_client(monkeypatch: pytest.MonkeyPatch, *, debug: bool) -> AsyncGenerator[AsyncClient, None]: + monkeypatch.setattr(app_settings, "debug", debug) + monkeypatch.setattr(app_settings, "public_base_url", "http://localhost:8080") + monkeypatch.setattr(app_settings, "app_public_base_url", "http://papyrus.localhost:3000") + app = create_app() + + async def reject_database_access() -> AsyncGenerator[AsyncSession, None]: + raise AssertionError("Use a database-backed client for routes that access the database") + yield + + app.dependency_overrides[get_db] = reject_database_access + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + yield client + + @pytest_asyncio.fixture async def prod_client( monkeypatch: pytest.MonkeyPatch, @@ -219,6 +290,7 @@ async def db_session( @pytest_asyncio.fixture async def auth_user( test_session_maker: async_sessionmaker[AsyncSession], + fixture_password_hash: str, ) -> dict[str, str]: user_uuid = uuid4() refresh_token = generate_opaque_token() @@ -233,7 +305,7 @@ async def auth_user( ) session.add(user) - session.add(PasswordCredential(user_id=user_uuid, password_hash=hash_password("current_password"))) + session.add(PasswordCredential(user_id=user_uuid, password_hash=fixture_password_hash)) await session.flush() auth_session = AuthSession( @@ -263,6 +335,11 @@ async def auth_headers(auth_user: dict[str, str]) -> dict[str, str]: return {"Authorization": f"Bearer {auth_user['access_token']}"} +@pytest.fixture(scope="session") +def fixture_password_hash() -> str: + return hash_password("current_password") + + @pytest.fixture(autouse=True) def reset_rate_limiter() -> None: """Keep per-IP rate-limit state isolated between tests.""" diff --git a/tests/services/test_auth.py b/tests/services/test_auth.py index a3f8d7a..5aee6e6 100644 --- a/tests/services/test_auth.py +++ b/tests/services/test_auth.py @@ -15,18 +15,10 @@ from papyrus.core.exceptions import ConflictError, ForbiddenError, UnauthorizedError, ValidationError from papyrus.core.security import generate_opaque_token, hash_opaque_token, hash_password from papyrus.models import AuthExchangeCode, AuthSession, EmailActionToken, PasswordCredential, User, UserIdentity -from papyrus.schemas.auth import LoginRequest, OAuthExchangeRequest, RefreshTokenRequest, RegisterRequest +from papyrus.schemas.auth import LoginRequest, OAuthExchangeRequest, RegisterRequest from papyrus.services import auth as auth_service -def test_auth_package_facade_exports_public_surface() -> None: - """The auth package façade preserves the legacy public surface.""" - assert auth_service.google_oauth_client is not None - assert auth_service.GoogleIdentity.__name__ == "GoogleIdentity" - assert auth_service.GOOGLE_PROVIDER == "google" - assert callable(auth_service.register_user) - - def test_google_login_allows_configured_app_public_base_url_redirect( monkeypatch: pytest.MonkeyPatch, configured_google: None, @@ -157,29 +149,6 @@ async def test_login_user_rejects_disabled_user( ) -async def test_refresh_tokens_rotates_and_invalidates_previous_token( - test_session_maker: async_sessionmaker[AsyncSession], -): - """Test service-layer refresh token rotation invalidates the previous token.""" - async with test_session_maker() as session: - register_result = await auth_service.register_user( - session, - RegisterRequest( - email="refresh@example.com", - password="SecureP@ss123", - display_name="Refresh User", - ), - None, - ) - - first_refresh_token = register_result.refresh_token - rotated = await auth_service.refresh_tokens(session, RefreshTokenRequest(refresh_token=first_refresh_token)) - assert rotated.refresh_token != first_refresh_token - - with pytest.raises(UnauthorizedError): - await auth_service.refresh_tokens(session, RefreshTokenRequest(refresh_token=first_refresh_token)) - - async def test_logout_current_session_revokes_session( test_session_maker: async_sessionmaker[AsyncSession], ): diff --git a/tests/services/test_sync.py b/tests/services/test_sync.py index 99c82cd..8dae7a5 100644 --- a/tests/services/test_sync.py +++ b/tests/services/test_sync.py @@ -6,7 +6,6 @@ from uuid import uuid4 import pytest -from pydantic import ValidationError as PydanticValidationError from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from papyrus.core.exceptions import ForbiddenError @@ -98,19 +97,3 @@ async def test_apply_powersync_upload_batch_rejects_cross_user_book_update( ) ], ) - - -async def test_apply_powersync_upload_batch_rejects_unknown_book_fields( - test_session_maker: async_sessionmaker[AsyncSession], -): - """The backend rejects fields outside the books upload contract.""" - async with test_session_maker() as session: - await _create_user(session, "missing-book@example.com") - - with pytest.raises(PydanticValidationError): - PowerSyncCrudMutation( - type="books", - op="PUT", - id=str(uuid4()), - data={"title": "Book", "unexpected": "value"}, - ) diff --git a/tests/test_account_deletion_migration.py b/tests/test_account_deletion_migration.py index a05bcc5..eb78495 100644 --- a/tests/test_account_deletion_migration.py +++ b/tests/test_account_deletion_migration.py @@ -11,6 +11,8 @@ from papyrus.models import Base +pytestmark = pytest.mark.migration + REVISIONS = [ "e6f7a8b9c012_add_account_deletion.py", "f7a8b9c0d123_add_account_notifications.py", diff --git a/tests/test_acquisition_migration.py b/tests/test_acquisition_migration.py index 706316e..c858dba 100644 --- a/tests/test_acquisition_migration.py +++ b/tests/test_acquisition_migration.py @@ -1,44 +1,64 @@ -"""Managed acquisition migration contract tests.""" +"""Execute the managed acquisition revision against an existing submission.""" +import importlib.util from pathlib import Path +from uuid import UUID, uuid4 -MIGRATIONS = Path(__file__).resolve().parents[1] / "alembic" / "versions" - - -def test_managed_acquisition_has_a_follow_up_migration() -> None: - revisions = [ - path - for path in MIGRATIONS.glob("*.py") - if path.name != "d4e5f6a7b8c9_add_acquisition.py" - and 'down_revision: str | Sequence[str] | None = "d4e5f6a7b8c9"' in path.read_text(encoding="utf-8") - ] - - assert len(revisions) == 1 - migration = revisions[0].read_text(encoding="utf-8") - - for column in ( - "download_root", - "book_id", - "client_hash", - "client_state", - "progress_basis_points", - "downloaded_bytes", - "total_bytes", - "download_speed_bytes_per_second", - "eta_seconds", - "selected_file_path", - "retry_count", - "next_poll_at", - "lease_owner", - "lease_until", - "submitted_at", - "started_at", - "updated_at", - "completed_at", - "cancelled_at", - ): - assert f'"{column}"' in migration - - assert '"ix_acquisition_jobs_owner_status"' in migration - assert '"ix_acquisition_jobs_next_poll_at"' in migration - assert "def downgrade() -> None:" in migration +import pytest +from alembic.autogenerate import compare_metadata +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import text + +from papyrus.models import Base + +pytestmark = pytest.mark.migration + + +async def test_managed_acquisition_upgrade_preserves_submission(db_session, auth_user): + path = Path(__file__).parents[1] / "alembic/versions/a0c1456470b0_add_managed_acquisition_jobs.py" + spec = importlib.util.spec_from_file_location("managed_acquisition_revision", path) + revision = importlib.util.module_from_spec(spec) + spec.loader.exec_module(revision) + + def migrate(session, operation): + with Operations.context(MigrationContext.configure(session.connection())): + operation() + + await db_session.run_sync(lambda session: migrate(session, revision.downgrade)) + job_id = uuid4() + + await db_session.execute( + text( + "INSERT INTO acquisition_jobs (job_id, owner_user_id, title, download_url) " + "VALUES (:id, :owner, 'Existing submission', 'magnet:?xt=urn:btih:existing')" + ), + {"id": job_id, "owner": UUID(auth_user["user_id"])}, + ) + + await db_session.run_sync(lambda session: migrate(session, revision.upgrade)) + + row = ( + await db_session.execute( + text("SELECT title, download_url, retry_count, book_id FROM acquisition_jobs WHERE job_id = :id"), + {"id": job_id}, + ) + ).one() + + assert row == ("Existing submission", "magnet:?xt=urn:btih:existing", 0, None) + await db_session.execute(text("UPDATE acquisition_jobs SET download_url = NULL WHERE job_id = :id"), {"id": job_id}) + + differences = await db_session.run_sync( + lambda session: compare_metadata(MigrationContext.configure(session.connection()), Base.metadata) + ) + + assert differences == [] + await db_session.run_sync(lambda session: migrate(session, revision.downgrade)) + + restored_url = await db_session.scalar( + text("SELECT download_url FROM acquisition_jobs WHERE job_id = :id"), + {"id": job_id}, + ) + + assert restored_url == f"managed-job:{job_id}" + await db_session.commit() diff --git a/tests/test_bookmark_migration.py b/tests/test_bookmark_migration.py index 09aac40..afe3edc 100644 --- a/tests/test_bookmark_migration.py +++ b/tests/test_bookmark_migration.py @@ -3,6 +3,7 @@ import importlib.util from pathlib import Path +import pytest from alembic.autogenerate import compare_metadata from alembic.migration import MigrationContext from alembic.operations import Operations @@ -10,6 +11,8 @@ from papyrus.models import Base +pytestmark = pytest.mark.migration + async def test_bookmark_revision_upgrade_downgrade_and_metadata(db_session): path = Path(__file__).parents[1] / "alembic/versions/af0fea8d6317_add_owned_bookmark_sync.py" diff --git a/tests/test_database.py b/tests/test_database.py index 8d21b23..9afd784 100644 --- a/tests/test_database.py +++ b/tests/test_database.py @@ -17,3 +17,51 @@ async def test_setup_creates_database_idempotently(db_session: AsyncSession, tes """Verify we are connected to the correct test database.""" result = await db_session.execute(text("SELECT current_database()")) assert result.scalar() == test_database_name + + +async def test_session_cleanup_removes_committed_rows_after_failure(test_schema): + from sqlalchemy.ext.asyncio import create_async_engine + + from papyrus.models import User + from tests.conftest import TEST_DATABASE_URL, isolated_sessions + + engine = create_async_engine(TEST_DATABASE_URL) + + try: + with pytest.raises(RuntimeError, match="interrupted test"): + async with isolated_sessions(engine) as sessions: + async with sessions() as writer: + writer.add(User(display_name="Committed", primary_email="isolation@example.com")) + await writer.commit() + + async with sessions() as observer: + assert await observer.scalar(text("SELECT count(*) FROM users")) == 1 + + raise RuntimeError("interrupted test") + + async with isolated_sessions(engine) as sessions, sessions() as observer: + assert await observer.scalar(text("SELECT count(*) FROM users")) == 0 + finally: + await engine.dispose() + + +async def test_migration_cleanup_restores_schema_after_committed_failure(test_schema): + from sqlalchemy.ext.asyncio import create_async_engine + + from tests.conftest import TEST_DATABASE_URL, isolated_sessions + + engine = create_async_engine(TEST_DATABASE_URL) + + try: + with pytest.raises(RuntimeError, match="interrupted migration"): + async with isolated_sessions(engine, migration=True) as sessions: + async with sessions() as session: + await session.execute(text("ALTER TABLE users DROP COLUMN display_name")) + await session.commit() + + raise RuntimeError("interrupted migration") + + async with isolated_sessions(engine) as sessions, sessions() as observer: + assert (await observer.execute(text("SELECT display_name FROM users"))).all() == [] + finally: + await engine.dispose() diff --git a/tests/test_library_migration.py b/tests/test_library_migration.py index f319283..a508c6a 100644 --- a/tests/test_library_migration.py +++ b/tests/test_library_migration.py @@ -4,6 +4,7 @@ from pathlib import Path from uuid import uuid4 +import pytest from alembic.autogenerate import compare_metadata from alembic.migration import MigrationContext from alembic.operations import Operations @@ -11,6 +12,8 @@ from papyrus.models import Base +pytestmark = pytest.mark.migration + async def test_library_revision_backfill_and_metadata(db_session, auth_user): path = Path(__file__).parents[1] / "alembic/versions/dcd3b384e6a4_add_owned_library_sync_and_promoted_.py" diff --git a/tests/test_sync_schema.py b/tests/test_sync_schema.py new file mode 100644 index 0000000..941ed9c --- /dev/null +++ b/tests/test_sync_schema.py @@ -0,0 +1,18 @@ +"""Upload validation does not require a service or database session.""" + +from uuid import uuid4 + +import pytest +from pydantic import ValidationError + +from papyrus.schemas.sync import PowerSyncCrudMutation + + +def test_book_mutation_rejects_unknown_fields() -> None: + with pytest.raises(ValidationError): + PowerSyncCrudMutation( + type="books", + op="PUT", + id=str(uuid4()), + data={"title": "Book", "unexpected": "value"}, + ) diff --git a/tests/test_tracking_migration.py b/tests/test_tracking_migration.py index 9292562..f1b51d0 100644 --- a/tests/test_tracking_migration.py +++ b/tests/test_tracking_migration.py @@ -5,6 +5,7 @@ from pathlib import Path from uuid import uuid4 +import pytest from alembic.autogenerate import compare_metadata from alembic.migration import MigrationContext from alembic.operations import Operations @@ -12,6 +13,8 @@ from papyrus.models import Base, SyncBook, User +pytestmark = pytest.mark.migration + async def test_tracking_upgrade_preserves_library_and_matches_metadata(db_session): path = Path(__file__).parents[1] / "alembic/versions/b5c6d7e8f901_add_reading_activity_and_goals.py" From d2ac512792f4369db6934eece35fe12a9f90ac43 Mon Sep 17 00:00:00 2001 From: Karolis Strazdas Date: Thu, 29 Oct 2026 14:37:37 +0200 Subject: [PATCH 2/2] ci: cancel superseded checks and report pytest phase timings --- .github/workflows/ci.yml | 15 +++++++ .github/workflows/release-checks.yml | 4 ++ AGENTS.md | 1 + tests/conftest.py | 2 + tests/test_timing.py | 57 +++++++++++++++++++++++++++ tests/timing.py | 58 ++++++++++++++++++++++++++++ 6 files changed, 137 insertions(+) create mode 100644 tests/test_timing.py create mode 100644 tests/timing.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52abf93..17a98ac 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,10 @@ on: branches: [development, master] pull_request: +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: backend: runs-on: ubuntu-latest @@ -65,4 +69,15 @@ jobs: run: uv run mypy . - name: Pytest + env: + PAPYRUS_TEST_TIMING: build/test-results/timing.json run: uv run pytest --durations=20 + + - name: Save test timing report + if: ${{ !cancelled() && hashFiles('build/test-results/timing.json') != '' }} + uses: actions/upload-artifact@v4 + with: + name: server-test-timing-${{ github.run_attempt }} + path: build/test-results/timing.* + if-no-files-found: error + retention-days: 14 diff --git a/.github/workflows/release-checks.yml b/.github/workflows/release-checks.yml index 4e00e1c..464a888 100644 --- a/.github/workflows/release-checks.yml +++ b/.github/workflows/release-checks.yml @@ -10,6 +10,10 @@ on: permissions: contents: read +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: checks: runs-on: ubuntu-latest diff --git a/AGENTS.md b/AGENTS.md index 33f2c50..0dfce6a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,6 +59,7 @@ maintainer's reviewed examples. The rules below also apply in standalone checkou - If database behavior changes, add the narrowest regression test that proves the query or transaction behavior. - Mark schema-changing tests with `pytest.mark.migration`. Ordinary tests share the schema and truncate rows after each case; migration tests rebuild it before and after each case, including failures. - Use `public_client` or `public_debug_client` for routes that do not access the database. These fixtures reject accidental database access. +- Set `PAPYRUS_TEST_TIMING=/tmp/pytest-timing.json` to export setup, call, and teardown timings. CI publishes these as a job summary and artifact. - Do not claim success without running the most relevant checks, or explicitly stating why they were not run. ## Migration Rules diff --git a/tests/conftest.py b/tests/conftest.py index 9bd40ce..3d88338 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,8 @@ from papyrus.main import settings as app_settings from papyrus.models import AuthSession, Base, PasswordCredential, User +pytest_plugins = ["tests.timing"] + @pytest.fixture def user_id() -> str: diff --git a/tests/test_timing.py b/tests/test_timing.py new file mode 100644 index 0000000..e859811 --- /dev/null +++ b/tests/test_timing.py @@ -0,0 +1,57 @@ +import json +import os +import subprocess +import sys +from pathlib import Path + +from tests.timing import summary + + +def test_phase_totals_and_rankings_include_failures(): + records = [ + {"test": "one", "phase": "setup", "seconds": 2.0, "outcome": "passed"}, + {"test": "two", "phase": "setup", "seconds": 3.0, "outcome": "failed"}, + {"test": "one", "phase": "call", "seconds": 4.0, "outcome": "failed"}, + {"test": "one", "phase": "teardown", "seconds": 1.0, "outcome": "passed"}, + ] + report = summary(records) + assert "| setup | 5.00 |" in report + assert "| call | 4.00 |" in report + assert "| teardown | 1.00 |" in report + assert report.index("| 3.00 | failed | two |") < report.index("| 2.00 | passed | one |") + + +def test_plugin_exports_all_phases_on_failure_without_hiding_exit_status(tmp_path): + source = tmp_path / "test_example.py" + source.write_text( + "import pytest\n" + "@pytest.fixture\n" + "def broken():\n raise RuntimeError('setup failure')\n" + "def test_setup(broken):\n pass\n" + "def test_failure():\n assert False\n" + "@pytest.mark.skip(reason='optional')\n" + "def test_skip():\n pass\n" + ) + output = tmp_path / "reports/timing.json" + step_summary = tmp_path / "summary.md" + environment = { + **os.environ, + "PYTEST_DISABLE_PLUGIN_AUTOLOAD": "1", + "PYTHONPATH": str(Path(__file__).resolve().parents[1]), + "PAPYRUS_TEST_TIMING": str(output), + "GITHUB_STEP_SUMMARY": str(step_summary), + } + result = subprocess.run( + [sys.executable, "-m", "pytest", "-p", "tests.timing", str(source)], + cwd=tmp_path, + env=environment, + capture_output=True, + text=True, + ) + assert result.returncode == 1, result.stdout + result.stderr + records = json.loads(output.read_text()) + assert {record["phase"] for record in records} == {"setup", "call", "teardown"} + assert any(record["phase"] == "setup" and record["outcome"] == "failed" for record in records) + assert any(record["phase"] == "call" and record["outcome"] == "failed" for record in records) + assert any(record["outcome"] == "skipped" for record in records) + assert step_summary.read_text() == output.with_suffix(".md").read_text() diff --git a/tests/timing.py b/tests/timing.py new file mode 100644 index 0000000..c4b4e6f --- /dev/null +++ b/tests/timing.py @@ -0,0 +1,58 @@ +"""Export pytest phase durations when PAPYRUS_TEST_TIMING names an output file.""" + +import json +import os +from pathlib import Path + + +def summary(records): + lines = [ + "### Pytest phase durations", + "", + "Phase totals measure test execution, not workflow elapsed or aggregate runner time.", + "Shared fixture costs are attributed to the test that initializes or finalizes them.", + "", + "| Phase | Seconds |", + "| --- | ---: |", + ] + + for phase in ("setup", "call", "teardown"): + total = sum(record["seconds"] for record in records if record["phase"] == phase) + lines.append(f"| {phase} | {total:.2f} |") + + for phase in ("setup", "call", "teardown"): + lines.extend(["", f"#### Slowest {phase}", "", "| Seconds | Outcome | Test |", "| ---: | --- | --- |"]) + ranked = sorted( + (record for record in records if record["phase"] == phase), + key=lambda record: record["seconds"], + reverse=True, + ) + + for record in ranked[:10]: + name = record["test"].replace("|", "\\|").replace("\n", " ").replace("`", "'") + lines.append(f"| {record['seconds']:.2f} | {record['outcome']} | {name} |") + + return "\n".join(lines) + "\n" + + +def pytest_terminal_summary(terminalreporter): + destination = os.environ.get("PAPYRUS_TEST_TIMING") + + if not destination: + return + + records = [ + {"test": report.nodeid, "phase": report.when, "seconds": report.duration, "outcome": report.outcome} + for reports in terminalreporter.stats.values() + for report in reports + if getattr(report, "when", None) in ("setup", "call", "teardown") + ] + path = Path(destination) + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(records, indent=2) + "\n", encoding="utf-8") + markdown = summary(records) + path.with_suffix(".md").write_text(markdown, encoding="utf-8") + + if step_summary := os.environ.get("GITHUB_STEP_SUMMARY"): + with Path(step_summary).open("a", encoding="utf-8") as stream: + stream.write(markdown)