diff --git a/.github/workflows/postgres-tests.yml b/.github/workflows/postgres-tests.yml index 9eb05c97c..9c36d23c5 100644 --- a/.github/workflows/postgres-tests.yml +++ b/.github/workflows/postgres-tests.yml @@ -1,9 +1,10 @@ name: Postgres tests # The full Python suite on Postgres (`make test-py-pg`), including the -# `@pytest.mark.postgres` tests that SQLite cannot run. Slow and serial (every -# test resets one database), so it is not part of the PR aggregate: it runs -# nightly, on demand, and on PRs that touch the database/tenancy layers (#508). +# `@pytest.mark.postgres` tests that SQLite cannot run. Parallel under xdist, +# one database per worker (every test resets its schema), but still slower than +# SQLite, so it is not part of the PR aggregate: it runs nightly, on demand, and +# on PRs that touch the database/tenancy layers (#508). on: schedule: diff --git a/CHANGELOG.md b/CHANGELOG.md index 2099738a5..b1d74d027 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -156,6 +156,26 @@ All notable changes to this project are documented in this file. The format is b it, which removes the `users` ↔ `audit_log` import cycle. ### Changed +- **Behaviour change:** the `simple_module_test` plugin no longer refuses `-n` + on Postgres: each pytest-xdist worker creates and uses its own database + (`_gw0`, `_gw1`, ...) and points `SM_TEST_DATABASE_URL` at it, so the + role needs `CREATEDB`. `make test-py-pg` now runs with + `-n $(PYTEST_WORKERS) --dist loadgroup` like `make test-py` + (`PYTEST_WORKERS=0` keeps the old serial run on the named database). +- `simple_module_test` on Postgres builds the schema once per process and, before + each later test, deletes every row and restarts the sequences instead of + dropping and re-creating `public` (~0.05 s instead of ~0.25 s per test). A + direct `reset_schema()` call or any change to the catalog (a test's own + tables, extensions, constraints) falls back to the full rebuild. +- `create_app` no longer resolves every route's dependencies at build time + (~0.5 s for the full module set, ~70% of a test's setup): module routers now + get their prefix from `include_router(prefix=...)`, which skips FastAPI's + resolving empty-path check (effective paths and the OpenAPI document are + unchanged), and the route prefix guard and the SPA overlap check read paths + from FastAPI's include contexts without resolving routes. Outside + `environment="testing"` the lifespan still resolves all routes at startup, + so the first requests after boot stay warm. The SQLite and Postgres suites + run ~45% faster. - tenants and api_tokens pages call their APIs through the typed client and take typed component props (#504). **Behaviour change:** the api_tokens pages no longer receive the unused `api_url` prop. diff --git a/CLAUDE.md b/CLAUDE.md index cbed2fee8..5991da05b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,7 +28,7 @@ Everything goes through `make`. Full list: `docs/reference/make-commands.md`. | `make install` | `uv sync --all-packages` + `npm install` | | `make dev` | Shared Docker stack up + `gen-pages` + API (:8000) and Vite (:5050) | | `make test` / `make test-py` / `make test-js` | Pytest (SQLite, e2e/perf excluded; parallel via `pytest-xdist`, `-n auto --dist loadgroup`, `PYTEST_WORKERS=0` for serial) then Vitest | -| `make test-py-pg` | Pytest on Postgres (`SM_TEST_PG_URL`, default db `sm_test`); serial, the plugin refuses `-n` there; not run in CI | +| `make test-py-pg` | Pytest on Postgres (`SM_TEST_PG_URL`, default db `sm_test`); parallel like `test-py`, each xdist worker creates its own `sm_test_gw` database (role needs `CREATEDB`); not in `pr-checks` | | `make test-e2e` | Playwright smoke tests against a running `make dev` | | `make lint` | Ruff format + Ruff + `ty` + Biome + per-workspace `tsc` + file-size cap + hardcoded-string check + untranslated-string check + package metadata/README checks | | `make doctor` | Module diagnostics (`SM0xx`); a dev boot runs the same plus `SM012`/`SM025` | diff --git a/Makefile b/Makefile index c406c6ab2..81716a5e2 100644 --- a/Makefile +++ b/Makefile @@ -72,11 +72,13 @@ test-py: # `@pytest.mark.anyio` would otherwise run on anyio's event loop while the # async fixtures ran on pytest-asyncio's, and an asyncpg connection cannot # cross loops (aiosqlite's thread hides this on SQLite). asyncio_mode=auto -# still runs those tests. Serial on purpose: every test drops and recreates -# `public` in the one database, so parallel workers would wipe each other's. +# still runs those tests. Parallel like test-py: every test resets the schema +# (rows emptied, or `public` rebuilt when it changed), so each xdist worker +# creates and uses its own `_gw` database (the role needs CREATEDB); +# PYTEST_WORKERS=0 uses the named database itself. SM_TEST_PG_URL ?= postgresql+asyncpg://postgres:postgres@localhost:5432/sm_test test-py-pg: - SM_TEST_DATABASE_URL=$(SM_TEST_PG_URL) uv run pytest -p no:anyio + SM_TEST_DATABASE_URL=$(SM_TEST_PG_URL) uv run pytest -p no:anyio -n $(PYTEST_WORKERS) --dist loadgroup test-js: npm test diff --git a/docs/database/extensions.md b/docs/database/extensions.md index c6854ba8b..f720dfb2e 100644 --- a/docs/database/extensions.md +++ b/docs/database/extensions.md @@ -67,5 +67,9 @@ pytestmark = pytest.mark.postgres They are skipped unless `SM_TEST_DATABASE_URL` is a Postgres URL (see `make test-py-pg`); the marker is registered by the `simple_module_test` -plugin. The fixtures drop and re-create `public` per test, and then re-create -every installed module's declared extensions. +plugin. The fixtures build the schema once per process and empty its tables +between tests; whenever they drop and re-create `public` (the first test, or a +test that changed the schema), they re-create every installed module's declared +extensions. Under pytest-xdist each worker +runs on its own database (`_gw0`, ...), which the plugin creates on first +use, so the role needs `CREATEDB`. diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index d50a4aaa7..934e01498 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -187,7 +187,8 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: app.state.deferred_startup.append(mod) # Here rather than in create_app: the host includes its own routers # after create_app returns, and those should be guarded too. - guard_included_routers(app) + settings = getattr(app.state.sm, "settings", None) + guard_included_routers(app, warm=getattr(settings, "environment", None) != "testing") yield for mod in reversed(modules): await mod.on_shutdown(app) diff --git a/framework/hosting/simple_module_hosting/_module_routes.py b/framework/hosting/simple_module_hosting/_module_routes.py index 123747207..cc77917d8 100644 --- a/framework/hosting/simple_module_hosting/_module_routes.py +++ b/framework/hosting/simple_module_hosting/_module_routes.py @@ -25,9 +25,14 @@ def wire_module_routes(app: FastAPI, module) -> bool: The single canonical implementation so ``create_app`` and the test harness in ``simple_module_test`` stay in lockstep if ``ModuleBase`` ever gains a new router type. + + Each prefix is given to ``include_router``, not to the ``APIRouter``: the + effective paths are the same, but FastAPI only checks an un-prefixed + include for empty paths, and that check resolves every nested route + (dependants, signatures, adapters; ~0.2 s per app build) that would + otherwise resolve lazily on first use. """ api_router = APIRouter( - prefix=module.meta.route_prefix, tags=[module.meta.name], generate_unique_id_function=make_unique_id_function(), ) @@ -36,7 +41,6 @@ def wire_module_routes(app: FastAPI, module) -> bool: # not (``token_guard``). pages = [Depends(page_route)] view_router = APIRouter( - prefix=module.meta.view_prefix, tags=[f"{module.meta.name} Views"], include_in_schema=False, dependencies=pages, @@ -44,8 +48,8 @@ def wire_module_routes(app: FastAPI, module) -> bool: module.register_routes(api_router, view_router) _clone_bare_prefix_route(view_router, module.meta.view_prefix) has_views = bool(view_router.routes) - app.include_router(api_router) - app.include_router(view_router, include_in_schema=False) + app.include_router(api_router, prefix=module.meta.route_prefix) + app.include_router(view_router, prefix=module.meta.view_prefix, include_in_schema=False) # Second view router for modules that serve both public and admin pages # and therefore cannot express both under a single view_prefix — see @@ -53,14 +57,13 @@ def wire_module_routes(app: FastAPI, module) -> bool: admin_prefix = getattr(module.meta, "admin_view_prefix", "") if admin_prefix: admin_router = APIRouter( - prefix=admin_prefix, tags=[f"{module.meta.name} Admin Views"], include_in_schema=False, dependencies=pages, ) module.register_admin_routes(admin_router) _clone_bare_prefix_route(admin_router, admin_prefix) - app.include_router(admin_router, include_in_schema=False) + app.include_router(admin_router, prefix=admin_prefix, include_in_schema=False) return has_views @@ -80,9 +83,9 @@ def _clone_bare_prefix_route(router: APIRouter, prefix: str) -> None: """ if not prefix: return - bare_target = f"{prefix}/" + # Paths are relative to the router: the prefix is added on include. for route in list(router.routes): - if isinstance(route, APIRoute) and route.path == bare_target: + if isinstance(route, APIRoute) and route.path == "/": router.add_api_route( "", route.endpoint, diff --git a/framework/hosting/simple_module_hosting/_route_guard.py b/framework/hosting/simple_module_hosting/_route_guard.py index 59ee63027..41f0e5bc8 100644 --- a/framework/hosting/simple_module_hosting/_route_guard.py +++ b/framework/hosting/simple_module_hosting/_route_guard.py @@ -36,6 +36,8 @@ from starlette.routing import Match from starlette.types import Scope +from simple_module_hosting._route_paths import included_route_paths + try: # private, and absent before FastAPI 0.140 from fastapi.routing import _IncludedRouter except ImportError: # pragma: no cover - older FastAPI flattens eagerly @@ -44,14 +46,6 @@ __all__ = ["guard_included_routers"] -def _effective_paths(included: Any) -> list[str]: - paths: list[str] = [] - for ctx in included.effective_route_contexts(): - route = ctx.starlette_route - paths.append((getattr(route, "path", "") if route is not None else ctx.path) or "") - return paths - - def _common_prefix(paths: list[str]) -> str: """The literal leading part every path shares — never past a ``{param}``.""" if not paths: @@ -73,7 +67,7 @@ def __init__(self, included: Any) -> None: def _current_prefix(self) -> str: version = self._included.original_router._get_routes_version() if version != self._version: - self._prefix = _common_prefix(_effective_paths(self._included)) + self._prefix = _common_prefix(included_route_paths(self._included)) self._version = version return self._prefix @@ -85,14 +79,16 @@ def __call__(self, scope: Scope) -> tuple[Match, Scope]: return self._matches(scope) -def guard_included_routers(app: FastAPI) -> None: +def guard_included_routers(app: FastAPI, *, warm: bool = True) -> None: """Install a prefix guard on each router included directly into ``app``. - Computing a guard's prefix resolves its router's effective routes, which is - also what FastAPI does lazily on the first request to reach them — building - every route's dependant, signature and pydantic adapters. Doing it here - moves that one-time cost (hundreds of ms for a full module set) from the - first request after boot into startup. + A guard's prefix comes from its routes' paths alone, so installing it + resolves nothing. ``warm`` then resolves every router's effective routes, as + FastAPI would lazily on the first request to reach each one (building every + route's dependant, signature and pydantic adapters), moving that one-time + cost (hundreds of ms for a full module set) from the first requests after + boot into startup. Test apps skip it: each is built for a handful of + requests, so resolving all ~400 routes was most of their setup time. """ if _IncludedRouter is None: return @@ -101,3 +97,6 @@ def guard_included_routers(app: FastAPI) -> None: guard = _PrefixGuard(route) guard._current_prefix() route.matches = guard # ty: ignore[invalid-assignment] + if warm: + for _ in route.effective_route_contexts(): + pass diff --git a/framework/hosting/simple_module_hosting/_route_paths.py b/framework/hosting/simple_module_hosting/_route_paths.py new file mode 100644 index 000000000..75609cd75 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_route_paths.py @@ -0,0 +1,72 @@ +"""Effective route paths of an app, without resolving its routes. + +FastAPI >= 0.140 includes routers lazily: an ``_IncludedRouter`` builds each +route's dependant, signature and pydantic adapters the first time a request +reaches it, which is ~0.5 s for the full module set. Asking it for its +``effective_route_contexts()`` just to read their paths forces all of that at +once, on every app build, for every router. + +The path itself is cheap: it is the include context's prefix plus the route's +own path (``include_context.path_for``), with nested includes folded in by +``include_context.combine``, the same two steps FastAPI's own resolution uses. +A route type with any other shape falls back to FastAPI building that one +route's context. ``tests/test_route_paths.py`` checks the result matches the +resolved paths for every router of a real app, so a FastAPI upgrade that +changes how paths compose fails there rather than misrouting. +""" + +from __future__ import annotations + +from typing import Any + +from fastapi import FastAPI +from fastapi.routing import APIRoute, APIWebSocketRoute +from starlette import routing + +# Private; see tests/test_route_guard_fastapi_contract.py before widening the +# fastapi dependency bound. +try: + from fastapi.routing import _IncludedRouter +except ImportError: # pragma: no cover - older FastAPI flattens eagerly + _IncludedRouter = None + +__all__ = ["app_route_paths", "included_route_paths"] + +# Types whose effective path is exactly ``include_context.path_for(route)``. +_PREFIXED = (APIRoute, routing.Route, APIWebSocketRoute) + + +def included_route_paths(included: Any) -> list[str]: + """Every effective path under one ``_IncludedRouter``, nested includes too.""" + context = included.include_context + paths: list[str] = [] + for route in included.original_router.routes: + if _IncludedRouter is not None and isinstance(route, _IncludedRouter): + child = _IncludedRouter( + original_router=route.original_router, + include_context=context.combine(route.include_context), + ) + paths.extend(included_route_paths(child)) + elif isinstance(route, _PREFIXED): + paths.append(context.path_for(route)) + else: + resolved = included._build_effective_context(route) + if resolved is not None: + paths.append(_resolved_path(resolved)) + return paths + + +def app_route_paths(app: FastAPI) -> list[str]: + """Every path registered on *app*, flattening its included routers.""" + paths: list[str] = [] + for route in app.routes: + if _IncludedRouter is not None and isinstance(route, _IncludedRouter): + paths.extend(included_route_paths(route)) + else: + paths.append(getattr(route, "path", "") or "") + return paths + + +def _resolved_path(context: Any) -> str: + route = context.starlette_route + return (getattr(route, "path", "") if route is not None else context.path) or "" diff --git a/framework/hosting/simple_module_hosting/_spa_mount.py b/framework/hosting/simple_module_hosting/_spa_mount.py index a0cfe36a6..2f029ee4a 100644 --- a/framework/hosting/simple_module_hosting/_spa_mount.py +++ b/framework/hosting/simple_module_hosting/_spa_mount.py @@ -25,6 +25,7 @@ from starlette.responses import Response from starlette.types import Scope +from simple_module_hosting._route_paths import app_route_paths from simple_module_hosting.static_files import IMMUTABLE_CACHE_CONTROL logger = logging.getLogger(__name__) @@ -100,24 +101,9 @@ def _is_page_request(path: str, scope: Scope) -> bool: return "text/html" in Headers(scope=scope).get("accept", "").lower() -def _registered_route_paths(app: FastAPI) -> list[str]: - """Every path registered on *app*, flattening FastAPI's lazy included routers.""" - paths: list[str] = [] - for route in app.routes: - contexts = getattr(route, "effective_route_contexts", None) - if contexts is None: - paths.append(getattr(route, "path", "") or "") - continue - for ctx in contexts(): - starlette_route = ctx.starlette_route - path = getattr(starlette_route, "path", "") if starlette_route is not None else ctx.path - paths.append(path or "") - return paths - - def mount_spa_mounts(app: FastAPI, modules: list) -> None: """Mount every module's SPAs, exempt the public ones from auth, publish prefixes.""" - problems = check_spa_mount_overlaps(modules, _registered_route_paths(app)) + problems = check_spa_mount_overlaps(modules, app_route_paths(app)) if problems: raise SpaMountError("\n".join(str(p) for p in problems)) diff --git a/framework/hosting/tests/test_route_guard_fastapi_contract.py b/framework/hosting/tests/test_route_guard_fastapi_contract.py index e73978e88..f5b686206 100644 --- a/framework/hosting/tests/test_route_guard_fastapi_contract.py +++ b/framework/hosting/tests/test_route_guard_fastapi_contract.py @@ -92,3 +92,37 @@ def replacement(scope): assert included.matches(scope) == (Match.NONE, {}), ( f"assigned matches(scope) must be callable; update {GUARD}" ) + + +PATHS = "framework/hosting/simple_module_hosting/_route_paths.py" + + +def test_private_include_context_contract(): + included_type = routing._IncludedRouter + leaf = APIRouter() + + @leaf.get("/item") + async def item(): + return {} + + parent = APIRouter() + parent.include_router(leaf, prefix="/leaf") + app = FastAPI() + app.include_router(parent, prefix="/api") + included = next(r for r in app.router.routes if isinstance(r, included_type)) + + context = getattr(included, "include_context", None) + assert context is not None, f"_IncludedRouter.include_context must exist; update {PATHS}" + nested = next(r for r in parent.routes if isinstance(r, included_type)) + combine = _accepts( + getattr(context, "combine", None), "include_context.combine(child)", nested.include_context + ) + child = combine(nested.include_context) + route = next(r for r in leaf.routes if getattr(r, "name", "") == "item") + path_for = _accepts(getattr(child, "path_for", None), "include_context.path_for(route)", route) + assert path_for(route) == "/api/leaf/item", ( + f"combine/path_for must compose include prefixes; update {PATHS}" + ) + included_type(original_router=leaf, include_context=child) # constructible by keyword + build = getattr(included, "_build_effective_context", None) + _accepts(build, "_IncludedRouter._build_effective_context(route)", route) diff --git a/framework/hosting/tests/test_route_paths.py b/framework/hosting/tests/test_route_paths.py new file mode 100644 index 000000000..d8e740e90 --- /dev/null +++ b/framework/hosting/tests/test_route_paths.py @@ -0,0 +1,89 @@ +"""``_route_paths`` gives the paths FastAPI's own resolution would, without resolving.""" + +from __future__ import annotations + +from fastapi import APIRouter, FastAPI, WebSocket +from fastapi.routing import _IncludedRouter +from simple_module_hosting._route_paths import app_route_paths, included_route_paths +from starlette.responses import PlainTextResponse + + +def _resolved(included) -> list[str]: + paths = [] + for ctx in included.effective_route_contexts(): + route = ctx.starlette_route + paths.append((getattr(route, "path", "") if route is not None else ctx.path) or "") + return paths + + +def _included(app: FastAPI) -> list: + return [route for route in app.router.routes if isinstance(route, _IncludedRouter)] + + +def _nested_app() -> FastAPI: + leaf = APIRouter(prefix="/leaf") + + @leaf.get("/item/{item_id}") + async def item(item_id: int): + return {"id": item_id} + + async def plain(request): + return PlainTextResponse("ok") + + leaf.add_route("/plain", plain) # a starlette Route: no router prefix applied + + @leaf.websocket("/ws") + async def ws(websocket: WebSocket): + await websocket.close() + + middle = APIRouter(prefix="/middle") + + @middle.get("") + async def bare(): + return {} + + middle.include_router(leaf, prefix="/extra") + middle.include_router(APIRouter(), prefix="/empty") + + app = FastAPI() + app.include_router(middle, prefix="/api") + app.include_router(leaf) + + @app.get("/root") + async def root(): + return {} + + return app + + +def test_nested_includes_compose_like_fastapi(): + app = _nested_app() + for included in _included(app): + mine = included_route_paths(included) + assert mine == _resolved(included) + assert "/api/middle/extra/leaf/item/{item_id}" in app_route_paths(app) + + +def test_computing_paths_resolves_no_route(): + app = _nested_app() + for included in _included(app): + included_route_paths(included) + assert included._effective_candidates_version is None + + +async def test_every_router_of_the_real_app_matches(app): + routers = _included(app) + assert routers + for included in routers: + assert included_route_paths(included) == _resolved(included), included.original_router + + +def test_the_guard_resolves_routes_only_when_asked_to_warm(): + from simple_module_hosting._route_guard import guard_included_routers + + cold, warm = _nested_app(), _nested_app() + guard_included_routers(cold, warm=False) + guard_included_routers(warm) + + assert all(r._effective_candidates_version is None for r in _included(cold)) + assert all(r._effective_candidates_version is not None for r in _included(warm)) diff --git a/framework/testing/simple_module_test/_schema.py b/framework/testing/simple_module_test/_schema.py index dcf206ec4..c4d7d6260 100644 --- a/framework/testing/simple_module_test/_schema.py +++ b/framework/testing/simple_module_test/_schema.py @@ -54,27 +54,34 @@ async def _create_all_tables(engine) -> None: """ from sqlalchemy import text - from simple_module_test.database import reset_schema_once + from simple_module_test.database import prepare_schema_once bases = _ensure_models_imported() heads = _alembic_heads() - await reset_schema_once(engine) # Postgres: start empty; SQLite memory already is - async with engine.begin() as conn: + async def _build(engine) -> None: + async with engine.begin() as conn: + + def _sync_create_all(sync_conn): + for base in bases: + base.metadata.create_all(sync_conn) - def _sync_create_all(sync_conn): - for base in bases: - base.metadata.create_all(sync_conn) + await conn.run_sync(_sync_create_all) + # Part of the build, so a reused Postgres schema's fingerprint covers it. + if heads: + await conn.execute( + text( + "CREATE TABLE IF NOT EXISTS alembic_version " + "(version_num VARCHAR(32) NOT NULL PRIMARY KEY)" + ) + ) - await conn.run_sync(_sync_create_all) + tables = [table for base in bases for table in base.metadata.tables.values()] + # Postgres reuses the schema built for an earlier test, emptied of rows. + await prepare_schema_once(engine, tables, _build) + async with engine.begin() as conn: if heads: - await conn.execute( - text( - "CREATE TABLE IF NOT EXISTS alembic_version " - "(version_num VARCHAR(32) NOT NULL PRIMARY KEY)" - ) - ) await conn.execute(text("DELETE FROM alembic_version")) for head in heads: await conn.execute( diff --git a/framework/testing/simple_module_test/database.py b/framework/testing/simple_module_test/database.py index 63e6a25be..49b96cc95 100644 --- a/framework/testing/simple_module_test/database.py +++ b/framework/testing/simple_module_test/database.py @@ -5,21 +5,27 @@ suites on Postgres — what SQLite cannot show: real concurrent transactions, row locks, stricter types, and SQL that SQLite happens to accept. -On Postgres every fixture starts from an empty ``public`` schema, so tests -stay independent; pooled connections are disabled because pytest-asyncio -gives each test its own event loop. +On Postgres every test starts with empty tables, so tests stay independent: +the schema is built once per process and emptied of rows between tests, and +dropped and rebuilt only when it changed (``prepare_schema_once``). Pooled +connections are disabled because pytest-asyncio +gives each test its own event loop. Under pytest-xdist each worker gets its +own database (``_gw0``, ...), so workers never drop each other's schema. """ from __future__ import annotations +import asyncio import os from typing import Any from sqlalchemy import text +from sqlalchemy.engine import make_url from sqlalchemy.pool import NullPool TEST_DATABASE_ENV = "SM_TEST_DATABASE_URL" SQLITE_MEMORY = "sqlite+aiosqlite:///:memory:" +XDIST_WORKER_ENV = "PYTEST_XDIST_WORKER" def database_url_for_tests() -> str: @@ -38,10 +44,45 @@ def init_db_kwargs(url: str) -> dict[str, Any]: return {} if is_sqlite(url) else {"poolclass": NullPool} +def isolate_worker_database() -> None: + """Point this xdist worker at its own Postgres database, creating it if needed. + + Rewrites ``SM_TEST_DATABASE_URL`` rather than wrapping it, so tests that read + the variable directly and subprocesses that inherit it land on the same + database. A no-op on SQLite and outside xdist workers. The role needs + ``CREATEDB`` the first time. + """ + worker = os.environ.get(XDIST_WORKER_ENV) + if not worker or not database_is_postgres(): + return + base = make_url(database_url_for_tests()) + url = base.set(database=f"{base.database}_{worker}") + asyncio.run(_create_database_if_missing(base, url.database or "")) + os.environ[TEST_DATABASE_ENV] = url.render_as_string(hide_password=False) + + +async def _create_database_if_missing(server_url: Any, name: str) -> None: + from sqlalchemy.ext.asyncio import create_async_engine + + engine = create_async_engine(server_url, poolclass=NullPool, isolation_level="AUTOCOMMIT") + try: + async with engine.connect() as conn: + exists = await conn.scalar( + text("SELECT 1 FROM pg_database WHERE datname = :n"), {"n": name} + ) + if not exists: + quoted = conn.dialect.identifier_preparer.quote_identifier(name) + await conn.execute(text(f"CREATE DATABASE {quoted}")) + finally: + await engine.dispose() + + async def reset_schema(engine: Any) -> None: """Empty the database before a test (Postgres); a no-op on SQLite.""" + global _built_schema if engine.dialect.name == "sqlite": return + _built_schema = None # whatever was built before is gone async with engine.begin() as conn: await conn.execute(text("DROP SCHEMA IF EXISTS public CASCADE")) await conn.execute(text("CREATE SCHEMA public")) @@ -79,6 +120,70 @@ async def reset_schema_once(engine: Any) -> None: _reset_for = _current_test +# The tables this process last built and the catalog fingerprint after the +# build (Postgres only); reused only while both still match. +_built_schema: tuple[frozenset[str], str] | None = None + +_FINGERPRINT = text( + "SELECT md5(coalesce(string_agg(item, ',' ORDER BY item), '')) FROM (" + " SELECT c.relname || ':' || c.relkind::text || ':' || coalesce(a.attname || ':'" + " || a.atttypid::text || ':' || a.attnotnull::text, '') AS item" + " FROM pg_class c LEFT JOIN pg_attribute a" + " ON a.attrelid = c.oid AND a.attnum > 0 AND NOT a.attisdropped" + " WHERE c.relnamespace = 'public'::regnamespace" + " UNION ALL SELECT 'con:' || conname FROM pg_constraint" + " WHERE connamespace = 'public'::regnamespace" + " UNION ALL SELECT 'type:' || typname FROM pg_type" + " WHERE typnamespace = 'public'::regnamespace" + " UNION ALL SELECT 'ext:' || extname FROM pg_extension) AS s" +) +_RESTART_SEQUENCES = text( + "SELECT setval(c.oid::regclass, 1, false) FROM pg_class c" + " WHERE c.relkind = 'S' AND c.relnamespace = 'public'::regnamespace" +) + + +async def prepare_schema_once(engine: Any, tables: list[Any], build: Any) -> None: + """Give this test empty ``tables``, at most once per test (see ``begin_test``). + + SQLite: ``build(engine)``, since every engine is a fresh in-memory database. + Postgres: when this process already built the schema and the catalog still + matches it, delete every row and restart the sequences (~0.03 s) instead of + dropping and rebuilding it (~0.25 s). Anything else (the first test, a + direct ``reset_schema`` call, a test's own DDL) resets and rebuilds. + """ + global _reset_for, _built_schema + if engine.dialect.name == "sqlite": + await build(engine) + return + if _current_test is not None and _reset_for is _current_test: + return + _reset_for = _current_test + names = frozenset(table.fullname for table in tables) + if _built_schema is not None and _built_schema == (names, await _fingerprint(engine)): + await _empty_tables(engine, tables) + return + await reset_schema(engine) + await build(engine) + _built_schema = (names, await _fingerprint(engine)) + + +async def _fingerprint(engine: Any) -> str: + async with engine.connect() as conn: + return await conn.scalar(_FINGERPRINT) + + +async def _empty_tables(engine: Any, tables: list[Any]) -> None: + from sqlalchemy.schema import sort_tables + + async with engine.begin() as conn: + quote = conn.dialect.identifier_preparer.format_table + # Children first, so no foreign key refuses a parent's delete. + for table in reversed(sort_tables(tables)): + await conn.execute(text(f"DELETE FROM {quote(table)}")) + await conn.execute(_RESTART_SEQUENCES) + + __all__ = [ "SQLITE_MEMORY", "TEST_DATABASE_ENV", @@ -87,6 +192,8 @@ async def reset_schema_once(engine: Any) -> None: "database_url_for_tests", "init_db_kwargs", "is_sqlite", + "isolate_worker_database", + "prepare_schema_once", "reset_schema", "reset_schema_once", ] diff --git a/framework/testing/simple_module_test/plugin.py b/framework/testing/simple_module_test/plugin.py index 6f424e4bc..2e8f52b0b 100644 --- a/framework/testing/simple_module_test/plugin.py +++ b/framework/testing/simple_module_test/plugin.py @@ -43,15 +43,19 @@ def pytest_configure(config: pytest.Config) -> None: "markers", "postgres: needs a PostgreSQL test database (SM_TEST_DATABASE_URL=postgresql+...)", ) - from simple_module_test.database import database_is_postgres - # Every Postgres test drops and recreates ``public`` in the one database, so - # xdist workers would wipe each other's schema mid-test. - if database_is_postgres() and getattr(config.option, "numprocesses", None): - raise pytest.UsageError( - "SM_TEST_DATABASE_URL is Postgres, whose schema every test drops: run " - "serially with `make test-py-pg` or `-n 0` (`PYTEST_WORKERS=0 make test-py`)" - ) + +@pytest.hookimpl(tryfirst=True) +def pytest_load_initial_conftests(early_config, parser, args) -> None: + """Give each xdist worker its own Postgres database. + + Every Postgres test drops and recreates ``public``. This runs before any + conftest is imported (``pytest_configure`` runs after), so a conftest or + test helper that reads the URL at import time already sees the worker's. + """ + from simple_module_test.database import isolate_worker_database + + isolate_worker_database() def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None: diff --git a/framework/testing/tests/test_schema_reuse_pg.py b/framework/testing/tests/test_schema_reuse_pg.py new file mode 100644 index 000000000..db0319ba0 --- /dev/null +++ b/framework/testing/tests/test_schema_reuse_pg.py @@ -0,0 +1,75 @@ +"""Postgres reuses the schema between tests, emptied of rows (``prepare_schema_once``).""" + +from __future__ import annotations + +import pytest +from simple_module_test import database +from sqlalchemy import Column, Integer, MetaData, String, Table, func, select, text +from sqlalchemy.ext.asyncio import create_async_engine + +pytestmark = pytest.mark.postgres + +_metadata = MetaData() +_item = Table( + "reuse_probe_item", + _metadata, + Column("id", Integer, primary_key=True), + Column("name", String(20)), +) + + +@pytest.fixture +async def engine(monkeypatch): + # The probe drives the per-test state itself; put it back afterwards. + for attr in ("_current_test", "_reset_for", "_built_schema"): + monkeypatch.setattr(database, attr, getattr(database, attr)) + url = database.database_url_for_tests() + engine = create_async_engine(url, **database.init_db_kwargs(url)) + try: + yield engine + finally: + await database.reset_schema(engine) # the next test rebuilds the real schema + await engine.dispose() + + +async def _next_test(engine, builds: list[int]) -> None: + async def build(engine) -> None: + builds.append(1) + async with engine.begin() as conn: + await conn.run_sync(_metadata.create_all) + + database.begin_test(object()) + await database.prepare_schema_once(engine, [_item], build) + + +async def _insert(engine) -> int: + async with engine.begin() as conn: + return ( + await conn.execute(_item.insert().values(name="x").returning(_item.c.id)) + ).scalar_one() + + +async def test_a_later_test_gets_the_same_schema_without_rows(engine): + builds: list[int] = [] + await _next_test(engine, builds) + assert [await _insert(engine), await _insert(engine)] == [1, 2] + + await _next_test(engine, builds) + + assert builds == [1] + async with engine.connect() as conn: + assert await conn.scalar(select(func.count()).select_from(_item)) == 0 + assert await _insert(engine) == 1 # sequences restart too + + +async def test_a_test_that_changes_the_schema_forces_a_rebuild(engine): + builds: list[int] = [] + await _next_test(engine, builds) + async with engine.begin() as conn: + await conn.execute(text("CREATE TABLE reuse_probe_extra (x int)")) + + await _next_test(engine, builds) + + assert builds == [1, 1] + async with engine.connect() as conn: + assert await conn.scalar(text("SELECT to_regclass('reuse_probe_extra')")) is None diff --git a/framework/testing/tests/test_worker_database.py b/framework/testing/tests/test_worker_database.py new file mode 100644 index 000000000..fddb64d90 --- /dev/null +++ b/framework/testing/tests/test_worker_database.py @@ -0,0 +1,51 @@ +"""Each xdist worker gets its own Postgres database (``isolate_worker_database``).""" + +from __future__ import annotations + +import pytest +from simple_module_test import database + +_PG = "postgresql+asyncpg://postgres:secret@db.local:5432/sm_test" + + +@pytest.fixture +def created(monkeypatch) -> list[tuple[str, str]]: + calls: list[tuple[str, str]] = [] + + async def fake_create(server_url, name): + calls.append((server_url.database, name)) + + monkeypatch.setattr(database, "_create_database_if_missing", fake_create) + return calls + + +def test_a_worker_is_pointed_at_its_own_database(monkeypatch, created): + monkeypatch.setenv(database.TEST_DATABASE_ENV, _PG) + monkeypatch.setenv(database.XDIST_WORKER_ENV, "gw3") + + database.isolate_worker_database() + + assert created == [("sm_test", "sm_test_gw3")] + assert database.database_url_for_tests() == ( + "postgresql+asyncpg://postgres:secret@db.local:5432/sm_test_gw3" + ) + + +def test_outside_xdist_the_named_database_is_used(monkeypatch, created): + monkeypatch.setenv(database.TEST_DATABASE_ENV, _PG) + monkeypatch.delenv(database.XDIST_WORKER_ENV, raising=False) + + database.isolate_worker_database() + + assert created == [] + assert database.database_url_for_tests() == _PG + + +def test_sqlite_workers_are_left_alone(monkeypatch, created): + monkeypatch.delenv(database.TEST_DATABASE_ENV, raising=False) + monkeypatch.setenv(database.XDIST_WORKER_ENV, "gw0") + + database.isolate_worker_database() + + assert created == [] + assert database.database_url_for_tests() == database.SQLITE_MEMORY