From 4701d385843a571a92ea7e3ba10541ab5de52abb Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 15:45:47 +0000 Subject: [PATCH 1/2] test: run the Python suite in parallel and cut per-app setup (#524) `make test-py` drops from ~24 min serial to ~5.5 min on 4 cores. - Add pytest-xdist; `make test-py` passes `-n $(PYTEST_WORKERS)` (default auto). Kept out of addopts so a single-test run stays in-process. - Fix the two things that broke under xdist: - test_negative_authz built parametrize ids from `uuid4()`, so workers collected differently-named tests and xdist refused to run. - test_bg_invalidation_redis used a fixed pub/sub channel on a Redis that CI shares between workers; the channel is now per process. - The test plugin lowers PasswordHelper's Argon2/bcrypt work factor: hashing the seeded admin was ~90ms of every `app` fixture. - The plugin refuses `-n` on Postgres, where every test drops the shared `public` schema; `make test-py-pg` stays serial. - CI writes the pytest totals line (count + wall time) to the job summary. Claude-Session: https://claude.ai/code/session_01LWvhqY9o3KhSo2iAg19cCi --- .github/workflows/pr.yml | 15 +++++- CLAUDE.md | 8 ++-- Makefile | 10 +++- .../testing/simple_module_test/plugin.py | 47 +++++++++++++++++++ .../tests/test_bg_invalidation_redis.py | 7 ++- modules/users/tests/test_negative_authz.py | 5 +- pyproject.toml | 1 + 7 files changed, 85 insertions(+), 8 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 252b5d2ce..184371bbf 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -84,7 +84,20 @@ jobs: enable-cache: true cache-dependency-glob: ${{ env.UV_CACHE_GLOB }} - run: make install-py - - run: make test-py + # Tee'd so the next step can put the totals line (count + wall time) on + # the run's summary page: a suite creeping back toward the serial ~24 min + # of #524 should be visible without opening the log. + - name: make test-py + shell: bash + run: | + set -o pipefail + make test-py 2>&1 | tee pytest.log + - name: Record suite time + if: always() + shell: bash + run: | + echo "### Python tests" >> "$GITHUB_STEP_SUMMARY" + grep -E '^=+ .*(passed|failed|error).* in [0-9.]+s' pytest.log | tail -n 1 >> "$GITHUB_STEP_SUMMARY" || true js-tests: name: JS tests (Vitest) diff --git a/CLAUDE.md b/CLAUDE.md index 6be2dbd2c..af9219814 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,8 +34,8 @@ All day-to-day tasks go through `make`: | `make dev` | Docker up + regen module pages + API (8000) and Vite (5050) in parallel | | `make kill` | Free ports 8000/5050/5173 | | `make test` | Run `test-py` then `test-js` (e2e excluded by default) | -| `make test-py` / `make test-js` | Run a single suite | -| `make test-py-pg` | Python suite on Postgres (`SM_TEST_PG_URL`, default db `sm_test`, schema is dropped per test) | +| `make test-py` / `make test-js` | Run a single suite (`test-py` runs in parallel via `pytest-xdist`, `-n auto`; `PYTEST_WORKERS=0` for serial) | +| `make test-py-pg` | Python suite on Postgres (`SM_TEST_PG_URL`, default db `sm_test`, schema is dropped per test, so it runs serially; the plugin refuses `-n` on Postgres) | | `make test-e2e` | Playwright smoke tests (requires `make dev` running + `uv run playwright install chromium`) | | `make lint` | Ruff format-check + Ruff + `ty` + Biome + per-workspace `tsc` + 300-line file cap | | `make doctor` | Module diagnostics (orphan pages, coupling violations, migration drift, locale checks) — same checks run at prod boot | @@ -44,7 +44,7 @@ All day-to-day tasks go through `make`: | `make new-module name=` | Scaffold a new module package end-to-end | | `make gen-pages` | Regenerate `host/client_app/modules.{manifest.json,generated.ts,generated.css}` from installed modules | -Single test: `uv run pytest path/to/test_file.py::test_name` (root `pyproject.toml` sets `asyncio_mode=auto` and `-m 'not e2e and not perf'`). A single JS test: `npx vitest run `. +Single test: `uv run pytest path/to/test_file.py::test_name` (in-process — `-n` is passed by `make test-py`, not `addopts`) (root `pyproject.toml` sets `asyncio_mode=auto` and `-m 'not e2e and not perf'`). A single JS test: `npx vitest run `. Entry point: `host/main.py` (`uv run --project host uvicorn host.main:app --reload`). Alembic runs from the repo root (`host/alembic.ini`) so it shares the `.env` / `SM_DATABASE_URL` with the API. @@ -124,6 +124,8 @@ The `simple_module_test` plugin provides app-level fixtures available to every t - `db_state`, `engine`, `db_session` — fresh in-memory `DatabaseState` per test; `db_session` also creates all module tables and stamps `alembic_version` at head so the boot-time migration check passes. - `app` — `create_app(settings)` with lifespan started/stopped. - `client` / `authenticated_client` — `httpx.AsyncClient`; `authenticated_client` seeds an admin via `users.bootstrap.create_admin` and carries a forged session cookie. +- Importing the plugin drops `PasswordHelper()`'s Argon2/bcrypt work factor to the minimum (hashes stay real `$argon2`/`$2b`); at production cost hashing the seeded admin was ~90 ms of every `app` fixture. Pass `password_hash=` explicitly to test real parameters. +- Tests must collect identically in every xdist worker: no `uuid4()`/random values in `parametrize` ids, and anything shared across processes (a Redis channel, a file) needs a per-worker name. E2E tests live in `tests/e2e/` behind the `e2e` pytest marker and run against a live server — see [docs/e2e-testing.md](docs/e2e-testing.md). diff --git a/Makefile b/Makefile index b8d568c57..22089a7a0 100644 --- a/Makefile +++ b/Makefile @@ -49,15 +49,21 @@ build: # Testing test: test-py test-js +# Parallel via pytest-xdist (#524): every fixture builds its own in-memory +# SQLite app, so tests share nothing across worker processes. Not in addopts, +# so a single `uv run pytest path::test` still runs in-process without paying +# for worker start-up. PYTEST_WORKERS=0 runs serially (e.g. under a debugger). +PYTEST_WORKERS ?= auto test-py: - uv run pytest + uv run pytest -n $(PYTEST_WORKERS) # The Python suite on Postgres (#343). Needs an empty database the tests may # drop and recreate `public` in. `-p no:anyio`: tests marked # `@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. +# 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. 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 diff --git a/framework/testing/simple_module_test/plugin.py b/framework/testing/simple_module_test/plugin.py index e630a65fc..37b1e776c 100644 --- a/framework/testing/simple_module_test/plugin.py +++ b/framework/testing/simple_module_test/plugin.py @@ -42,6 +42,14 @@ 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: run serially (-n 0), the schema is shared" + ) def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None: @@ -86,6 +94,45 @@ def _bootstrap_eager_celery() -> None: _bootstrap_eager_celery() +def _cheap_password_hashing() -> None: + """Make ``PasswordHelper()`` hash with minimal Argon2/bcrypt cost. + + Every ``app`` fixture seeds an admin, and production-cost Argon2 is ~90ms + of CPU per hash — over a tenth of each app's setup, paid thousands of times + a run. The hashes stay real ``$argon2``/``$2b`` strings that verify and + upgrade through the same code paths; only their work factor drops, which is + a property of deployment rather than anything a test asserts. + + Patched at import, not in a fixture: test modules build ``PasswordHelper()`` + at module scope during collection, before any fixture runs. An explicit + ``password_hash=`` still wins. Skipped when ``fastapi_users`` is absent. + """ + try: + from fastapi_users.password import PasswordHelper + from pwdlib import PasswordHash + from pwdlib.hashers.argon2 import Argon2Hasher + from pwdlib.hashers.bcrypt import BcryptHasher + except ImportError: + return + + original_init = PasswordHelper.__init__ + + def _init(self, password_hash: PasswordHash | None = None) -> None: + if password_hash is None: + password_hash = PasswordHash( + ( + Argon2Hasher(time_cost=1, memory_cost=8, parallelism=1), + BcryptHasher(rounds=4), + ) + ) + original_init(self, password_hash) + + PasswordHelper.__init__ = _init # type: ignore[method-assign] + + +_cheap_password_hashing() + + @pytest.fixture def fake_event_bus() -> FakeEventBus: """Fresh recording EventBus for each test.""" diff --git a/modules/background_tasks/tests/test_bg_invalidation_redis.py b/modules/background_tasks/tests/test_bg_invalidation_redis.py index 57d763726..db0d6286d 100644 --- a/modules/background_tasks/tests/test_bg_invalidation_redis.py +++ b/modules/background_tasks/tests/test_bg_invalidation_redis.py @@ -17,12 +17,17 @@ from __future__ import annotations import asyncio +import os import pytest from background_tasks.invalidation import RedisInvalidationTransport from simple_module_core.invalidation import Invalidation, InvalidationBus -_CHANNEL = "qa.invalidation" +# Per process: under pytest-xdist every worker shares one Redis (CI points them +# all at a single service container), and pub/sub has no isolation but the +# channel name. On a fixed name one worker's publish reached another worker's +# subscriber, and the assertions that count deliveries or subscribers broke. +_CHANNEL = f"qa.invalidation.{os.getpid()}" _DELIVERY_TIMEOUT_SECONDS = 5.0 diff --git a/modules/users/tests/test_negative_authz.py b/modules/users/tests/test_negative_authz.py index 71197b747..12896a5a0 100644 --- a/modules/users/tests/test_negative_authz.py +++ b/modules/users/tests/test_negative_authz.py @@ -17,7 +17,10 @@ import pytest -_FAKE_ID = uuid.uuid4() +# Fixed, not ``uuid4()``: the id lands in the parametrize ids, and pytest-xdist +# refuses to run when its workers collect differently-named tests. The request +# must be refused before any lookup, so which id it names does not matter. +_FAKE_ID = uuid.UUID("00000000-0000-4000-8000-000000000000") _PROTECTED_ENDPOINTS: tuple[tuple[str, str, dict | None], ...] = ( # users — admin sub-router ("GET", "/api/users/admin", None), diff --git a/pyproject.toml b/pyproject.toml index 9e27a8e8b..8fed39acc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,6 +17,7 @@ dev = [ "pytest>=8.0", "pytest-asyncio>=0.24", "pytest-benchmark>=4.0", + "pytest-xdist>=3.6", "httpx>=0.27", "ruff>=0.8", "playwright>=1.58.0", From 1458223ff57ff0358afe669a545a9331a1ed6930 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 16:16:02 +0000 Subject: [PATCH 2/2] test: keep repo-mutating and stack-sharing tests on one xdist worker Review follow-ups on the xdist change (#524): - test_module_css_build: both tests rewrite the shared tree (generated manifests, host/static/dist, an audit_log probe page), so they raced when two workers ran them with node_modules present. Grouped with xdist_group("repo-build"); `make test-py` now uses --dist loadgroup. - test_invalidation_cross_process: grouped so the module-scoped subprocess stack is built once, not once per worker that draws one of its tests. - SM_TEST_REAL_PASSWORD_HASHING opts a consumer suite out of the plugin's cheap-hashing patch. - The Postgres + -n UsageError names `make test-py-pg` / PYTEST_WORKERS=0. Claude-Session: https://claude.ai/code/session_01LWvhqY9o3KhSo2iAg19cCi --- CLAUDE.md | 2 +- Makefile | 4 +++- framework/cli/tests/test_module_css_build.py | 3 +++ framework/testing/simple_module_test/plugin.py | 9 +++++++-- tests/integration/test_invalidation_cross_process.py | 5 +++++ 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index af9219814..e04af115e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,7 +124,7 @@ The `simple_module_test` plugin provides app-level fixtures available to every t - `db_state`, `engine`, `db_session` — fresh in-memory `DatabaseState` per test; `db_session` also creates all module tables and stamps `alembic_version` at head so the boot-time migration check passes. - `app` — `create_app(settings)` with lifespan started/stopped. - `client` / `authenticated_client` — `httpx.AsyncClient`; `authenticated_client` seeds an admin via `users.bootstrap.create_admin` and carries a forged session cookie. -- Importing the plugin drops `PasswordHelper()`'s Argon2/bcrypt work factor to the minimum (hashes stay real `$argon2`/`$2b`); at production cost hashing the seeded admin was ~90 ms of every `app` fixture. Pass `password_hash=` explicitly to test real parameters. +- Importing the plugin drops `PasswordHelper()`'s Argon2/bcrypt work factor to the minimum (hashes stay real `$argon2`/`$2b`); at production cost hashing the seeded admin was ~90 ms of every `app` fixture. Pass `password_hash=` explicitly to test real parameters, or set `SM_TEST_REAL_PASSWORD_HASHING=1` to opt a whole run out. - Tests must collect identically in every xdist worker: no `uuid4()`/random values in `parametrize` ids, and anything shared across processes (a Redis channel, a file) needs a per-worker name. E2E tests live in `tests/e2e/` behind the `e2e` pytest marker and run against a live server — see [docs/e2e-testing.md](docs/e2e-testing.md). diff --git a/Makefile b/Makefile index 22089a7a0..97184b723 100644 --- a/Makefile +++ b/Makefile @@ -53,9 +53,11 @@ test: test-py test-js # SQLite app, so tests share nothing across worker processes. Not in addopts, # so a single `uv run pytest path::test` still runs in-process without paying # for worker start-up. PYTEST_WORKERS=0 runs serially (e.g. under a debugger). +# loadgroup keeps tests marked `xdist_group(...)` on one worker (shared repo +# tree, module-scoped subprocess stacks); everything else is load-balanced. PYTEST_WORKERS ?= auto test-py: - uv run pytest -n $(PYTEST_WORKERS) + uv run pytest -n $(PYTEST_WORKERS) --dist loadgroup # The Python suite on Postgres (#343). Needs an empty database the tests may # drop and recreate `public` in. `-p no:anyio`: tests marked diff --git a/framework/cli/tests/test_module_css_build.py b/framework/cli/tests/test_module_css_build.py index 919a700fa..df8d2a539 100644 --- a/framework/cli/tests/test_module_css_build.py +++ b/framework/cli/tests/test_module_css_build.py @@ -27,6 +27,9 @@ reason="node_modules absent — run `npm install` to exercise the real build", ), pytest.mark.skipif(shutil.which("npm") is None, reason="npm not on PATH"), + # Both tests rewrite the shared tree (generated manifests, host/static/dist, a + # probe page under audit_log), so under xdist they must share one worker. + pytest.mark.xdist_group("repo-build"), ] diff --git a/framework/testing/simple_module_test/plugin.py b/framework/testing/simple_module_test/plugin.py index 37b1e776c..820ea24a3 100644 --- a/framework/testing/simple_module_test/plugin.py +++ b/framework/testing/simple_module_test/plugin.py @@ -48,7 +48,8 @@ def pytest_configure(config: pytest.Config) -> None: # 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: run serially (-n 0), the schema is shared" + "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`)" ) @@ -105,8 +106,12 @@ def _cheap_password_hashing() -> None: Patched at import, not in a fixture: test modules build ``PasswordHelper()`` at module scope during collection, before any fixture runs. An explicit - ``password_hash=`` still wins. Skipped when ``fastapi_users`` is absent. + ``password_hash=`` still wins. Skipped when ``fastapi_users`` is absent, or + when ``SM_TEST_REAL_PASSWORD_HASHING`` is set — the opt-out for a consumer + suite whose tests depend on production hash parameters. """ + if os.environ.get("SM_TEST_REAL_PASSWORD_HASHING"): + return try: from fastapi_users.password import PasswordHelper from pwdlib import PasswordHash diff --git a/tests/integration/test_invalidation_cross_process.py b/tests/integration/test_invalidation_cross_process.py index 97331abfb..04f6279f3 100644 --- a/tests/integration/test_invalidation_cross_process.py +++ b/tests/integration/test_invalidation_cross_process.py @@ -39,6 +39,11 @@ import httpx import pytest +# One worker for the module: the ``stack`` fixture (import-from-env plus uvicorn +# boots) is module-scoped, so splitting the two arms across xdist workers would +# build it twice. +pytestmark = pytest.mark.xdist_group("cross-process-stack") + _ROOT = str(Path(__file__).resolve().parent.parent.parent) _ADMIN = "admin@example.com" _PASSWORD = "AdminPass1!"