Skip to content

Commit 3c03eef

Browse files
committed
Run only the suites a change can reach, across every core
Running the tests before a push took 294s measured, and most of it was suites the change could not have broken. CI has scoped per part since .github/actions/changed-paths landed; locally there was nothing, so editing one pipeline script still stood up Postgres and ran 2,708 client tests. scripts/test.sh makes that same decision before the push. The scope lists are read out of each suite's own workflow YAML at run time rather than copied into the script - the parse test_ci_scope.py already does - so local and CI cannot disagree by being forgotten. Every uncertain case runs everything, for the reason the action gives: a stale main ref, an unreadable workflow, a detached head. Linters and formatters for everything selected go first, which turns a formatting slip from a CI round trip into six seconds. pytest-xdist for the three Python suites. Free for the pipeline and settings suites, which hold no state between tests. Not free for the backend, whose isolation model is "drop every table between tests" against one shared Postgres: four workers pointed at one database do not fail, they wedge on each other's locks - 1785s before it was killed, against 60s serially. So tests/conftest.py gives each worker its own database, created on demand, and the per-test drop is unchanged inside it. test_worker_database.py holds that in place, including a test that the rewrite really took effect rather than merely being correct. Coverage is off unless asked for. It is visibility-only in all four suites by deliberate decision, so leaving it out cannot change a green run into a red one, and it costs 148s against 100s on the client alone. --coverage puts it back and CI still measures it on every run. Four suites: 294s by hand, 174s through the script, 20 to 50 seconds for a change to one of the Python parts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxopV5HM7o9M4BywwnVBZX
1 parent bb66242 commit 3c03eef

14 files changed

Lines changed: 593 additions & 5 deletions

.github/tests/requirements-dev.in

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,8 @@
55
pytest
66
PyYAML
77
ruff
8+
9+
# Runs the suite across cores - 24s serial against 12s on four workers. These
10+
# tests parse workflow YAML and hold no state between them, so there is
11+
# nothing for the split to disturb.
12+
pytest-xdist

.github/tests/requirements-dev.txt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
# uv pip compile --universal --python-version 3.11 .github/tests/requirements-dev.in -o .github/tests/requirements-dev.txt
33
colorama==0.4.6 ; sys_platform == 'win32'
44
# via pytest
5+
execnet==2.1.2
6+
# via pytest-xdist
57
iniconfig==2.3.0
68
# via pytest
79
packaging==26.3
@@ -11,6 +13,10 @@ pluggy==1.6.0
1113
pygments==2.20.0
1214
# via pytest
1315
pytest==9.1.1
16+
# via
17+
# -r .github/tests/requirements-dev.in
18+
# pytest-xdist
19+
pytest-xdist==3.8.0
1420
# via -r .github/tests/requirements-dev.in
1521
pyyaml==6.0.3
1622
# via -r .github/tests/requirements-dev.in

CLAUDE.md

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,23 @@ that used to live here.
122122

123123
## Run what CI runs, before pushing
124124

125-
Every suite CI runs, not only the one you touched:
125+
```
126+
scripts/test.sh
127+
```
128+
129+
That is the whole command. It works out which suites your changes actually
130+
reach — reading each one's scope list out of its own workflow YAML, so it
131+
cannot disagree with CI by being forgotten — and runs those, linters and
132+
formatters first, each suite across every core. A change to one of the Python
133+
parts finishes in 20 to 50 seconds; `--all` is the full four suites in 174s,
134+
against 294s for running them by hand. `--list` says what it picked and which
135+
file decided it, `--all` overrides the scoping, and `--coverage` puts the
136+
coverage reports back.
137+
138+
Every uncertain case runs everything rather than guessing — a stale `main`
139+
ref, an unreadable workflow, a detached head — so a wrong answer costs a
140+
minute, never a missed regression. If you want the long-hand form anyway, or
141+
the script cannot run:
126142

127143
```
128144
cd client && npm run typecheck && npm run lint && npm run format:check && npm test && npm run build

CONTRIBUTING.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ Area labels: `client`, `backend`, `pipeline`, `data`, `ops`, `docs`.
5252

5353
Three independent parts, each with its own tests, plus a small fourth suite covering the repository's own CI configuration. CI runs the same commands, so a green local run means a green CI run.
5454

55+
**`scripts/test.sh` runs the ones your change actually reaches**, which is usually one of the four. It reads each suite's scope list out of that suite's own workflow file, so it makes the same decision CI does rather than a second copy of it that can go stale; it runs the linters and formatters for everything selected before it runs any tests, so a formatting slip costs six seconds instead of a CI round trip; and it runs each suite across every core. Measured, four cores: 294s for the full sequence below, 174s for `scripts/test.sh --all`, 20 to 50 seconds for a change to one of the Python parts. `--list` shows what it picked and which changed file decided it. Anything it cannot work out — a stale `main` ref, an unreadable workflow — it resolves by running everything.
56+
57+
The per-part commands below are what it runs, and remain the reference.
58+
5559
**Client** — React + TypeScript + Vite, MapLibre GL for the map.
5660

5761
```

TESTING.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@ Built. pytest, 167 tests (measured 2026-08-06), reusing the pipeline's approach
115115

116116
The isolation model is worth knowing before pointing `DATABASE_URL` anywhere: each test drops every table in whatever database that URL names and recreates the schema. That is what makes the suite recover from a run killed mid-test, and it is also why the URL must never name a database anyone cares about.
117117

118+
That model is also why this suite is the one place in the repository where parallelism had to be bought rather than switched on. "Drop every table between tests" is safe exactly as long as one process is doing it; four `pytest -n` workers sharing one database do not merely fail, they wedge, each blocking on locks held by tables another worker is partway through dropping — measured at 1785s before it was killed, against 60s for the same suite serially. So `tests/conftest.py` gives each worker its own database, created on demand, and the per-test drop is unchanged inside it. Every worker still runs its own tests serially against its own schema, which is the model that was already there; `gw0` simply cannot see `gw1`'s tables to drop them. `tests/test_worker_database.py` is what keeps that true, including one test asserting the rewrite actually took effect in the process running it — a rewrite that silently became a no-op would put every worker back on one database and buy the deadlock back.
119+
118120
Three invariants from the wireframe handoff live here specifically, since they're only meaningfully enforceable server-side:
119121

120122
- `severity: serious` on a `Report` is only ever set by a user with a moderator role; a self-set attempt is rejected server-side, not just hidden client-side.
@@ -208,6 +210,20 @@ The distinction is which half of the check you want. `settings-check.yml`'s `con
208210

209211
The action answers "run" for every case it is unsure about - a push, a PR too large for the files API to list, an API call that failed, an empty path list. Running a suite that did not need to run costs a minute; skipping one that did costs a merge, and does it quietly.
210212

213+
### The same decision, locally
214+
215+
`scripts/test.sh` makes that decision before the push instead of after it. CONTRIBUTING.md asks for every suite before every push and is right to - the round trip it prevents is real, and a quarter of this repository's CI failures were formatting alone. What it costs is the whole four-suite run for a change that could only have broken one part, which is most changes here.
216+
217+
Measured on a four-core machine: the full sequence CONTRIBUTING.md lists takes **294s**. `scripts/test.sh --all` is the same four suites in **174s**, and a change to one of the Python parts is **20 to 50 seconds** because the other three suites do not run at all. A client-only change is about **two minutes**, nearly all of it the client suite itself - that one is large enough that scoping is what helps every *other* change, rather than something that helps it. Three things get those numbers, and only the first is a judgement call:
218+
219+
- **Only the affected suites run.** The scope lists are *read out of the workflow YAML at run time*, not copied into the script - the same parse `test_ci_scope.py` already does, so local and CI cannot disagree by being forgotten. Adding a path to a workflow changes what runs locally in the same edit. Every uncertain case runs everything, for the reason the action gives: a stale `main` ref, no upstream, an unreadable workflow, a detached head.
220+
- **The suites run across cores.** `pytest-xdist` for the three Python suites; vitest already did. Pipeline 45s to 22s, settings 24s to 12s, backend 60s to 16s.
221+
- **Coverage is off unless asked for.** It is visibility-only in all four suites by deliberate decision, so leaving it out cannot turn a green run red or the reverse - and it is not free: 148s against 100s for the client. `--coverage` puts it back, and CI measures it on every run regardless, which is where the report is actually read.
222+
223+
The linters and formatters for every selected suite run **before any suite does**, which is the ordering CLAUDE.md asks for and the reason it asks. Ruff and prettier answer in about six seconds against three minutes of tests, and the CI job that catches formatting runs the formatter first - so a formatting-only failure there never ran the tests at all and the log said nothing about the change being made.
224+
225+
What this does *not* do is select individual tests, for the reason the section above gives. Per part is still the whole of the mapping.
226+
211227
## The long-term strategy
212228

213229
Where this is going, given what the audit measured. Five commitments, in the order they pay off; the concrete deltas live in issues, per CONTRIBUTING.md's one-home rule.

backend/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ bash scripts/local-postgres.sh # start the local database
1414

1515
Everything the suite touches lives in `ourhike_test`, which it drops tables from freely - that is why it is a separate database from `ourhike_dev` and why nothing you are working on lives there.
1616

17+
Run it with `-n auto` and each worker gets `ourhike_test_gw0`, `ourhike_test_gw1` and so on, created on demand by `tests/conftest.py` - 16s against 60s, and the `CREATEDB` this script already grants the role is what lets it happen without coming back here. They are the same kind of database as `ourhike_test` and just as disposable; `dropdb` them whenever, the next run makes them again. Sharing one database between workers is what does not work, and does not fail cleanly - see TESTING.md's backend section.
18+
1719
## Quick start
1820

1921
```

backend/requirements-dev.in

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,16 @@ pytest-cov
44
httpx
55
ruff
66

7+
# Runs the suite across cores - 60s serial against 16s on four workers, which
8+
# is the largest single saving available in this repository's local loop.
9+
#
10+
# Not free here the way it is for the other two suites: this one shares a
11+
# Postgres and drops every table between tests, so four workers pointed at one
12+
# database deadlock rather than merely fail. tests/conftest.py gives each
13+
# worker its own database, and tests/test_worker_database.py is why that stays
14+
# true.
15+
pytest-xdist
16+
717
# S3 doubled in-process rather than a live bucket, the project's established
818
# convention (pipeline/requirements-dev.in asks for the same thing for
919
# publish.py's tests).

backend/requirements-dev.txt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ cryptography==50.0.0
5656
# -c backend/requirements.txt
5757
# moto
5858
# pyjwt
59+
execnet==2.1.2
60+
# via pytest-xdist
5961
fastapi==0.141.1
6062
# via
6163
# -c backend/requirements.txt
@@ -144,8 +146,11 @@ pytest==9.1.1
144146
# via
145147
# -r backend/requirements-dev.in
146148
# pytest-cov
149+
# pytest-xdist
147150
pytest-cov==7.1.0
148151
# via -r backend/requirements-dev.in
152+
pytest-xdist==3.8.0
153+
# via -r backend/requirements-dev.in
149154
python-dateutil==2.9.0.post0
150155
# via
151156
# -c backend/requirements.txt

backend/tests/conftest.py

Lines changed: 70 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,11 @@
1111
runs against is the engine production runs on, so a passing run means
1212
something about Supabase's Postgres rather than about a local stand-in.
1313
14-
It is one shared database across the whole test run rather than a fresh one
15-
per test, so isolation comes from dropping every table the test created
16-
before the next test starts (see `_reset_schema` below).
14+
It is one shared database per *worker* rather than a fresh one per test, so
15+
isolation comes from dropping every table the test created before the next
16+
test starts (see `_reset_schema` below). Serially that is one database for
17+
the whole run; under `pytest -n` it is one each, for the reason
18+
`_worker_database_url` gives.
1719
"""
1820

1921
import os
@@ -39,9 +41,73 @@
3941

4042
import pytest # noqa: E402
4143
from fastapi.testclient import TestClient # noqa: E402
42-
from sqlalchemy import MetaData, create_engine # noqa: E402
44+
from sqlalchemy import MetaData, create_engine, text # noqa: E402
45+
from sqlalchemy.engine import make_url # noqa: E402
46+
from sqlalchemy.exc import ProgrammingError # noqa: E402
4347
from sqlalchemy.orm import Session, sessionmaker # noqa: E402
4448

49+
50+
def _worker_database_url(url: str, worker: str) -> str:
51+
"""`url` with the running xdist worker's name appended to the database.
52+
53+
The isolation model in this file's docstring is "drop every table between
54+
tests", and that is only safe while one process is doing it. Point four
55+
`pytest -n` workers at one database and they drop each other's tables
56+
mid-test: measured here, that is not a handful of failures but a
57+
deadlock - workers block on locks held by tables another worker is in the
58+
middle of dropping, and a run that takes 60s serially took 1785s before
59+
it was killed.
60+
61+
So parallelism is bought with a database per worker rather than by
62+
weakening the isolation. Each worker still runs its own tests serially
63+
against its own database, which is exactly the model that was there
64+
before - `gw0` simply cannot see `gw1`'s tables to drop them.
65+
"""
66+
parsed = make_url(url)
67+
return parsed.set(database=f"{parsed.database}_{worker}").render_as_string(hide_password=False)
68+
69+
70+
def _ensure_database(url: str) -> None:
71+
"""Create `url`'s database if it is not there yet.
72+
73+
`scripts/local-postgres.sh` creates `ourhike_test`; it cannot create the
74+
per-worker ones because how many there are is decided by the `-n` on the
75+
command line. Creating them here keeps that script's contract intact and
76+
means a parallel run needs no setup step of its own.
77+
78+
CREATE DATABASE has no IF NOT EXISTS, and two workers racing on the same
79+
name is the normal case rather than an edge one, so the duplicate is
80+
caught instead of tested for.
81+
"""
82+
parsed = make_url(url)
83+
admin = create_engine(parsed.set(database="postgres"), isolation_level="AUTOCOMMIT")
84+
try:
85+
with admin.connect() as connection:
86+
already_there = connection.execute(
87+
text("select 1 from pg_database where datname = :name"), {"name": parsed.database}
88+
).scalar()
89+
if not already_there:
90+
try:
91+
connection.execute(text(f'create database "{parsed.database}"'))
92+
except ProgrammingError:
93+
# Another worker got there between the check and the
94+
# create. Its database is as good as this one would be.
95+
pass
96+
finally:
97+
admin.dispose()
98+
99+
100+
# Before `from app.config import settings` below, because that reads
101+
# DATABASE_URL once at import and every other reader in the codebase - the
102+
# module-level engine in app/db/session.py, alembic/env.py, and the tests that
103+
# call `settings.database_url` directly - goes through that one object. Setting
104+
# the variable is therefore the whole change; nothing else has to learn about
105+
# workers.
106+
_XDIST_WORKER = os.environ.get("PYTEST_XDIST_WORKER")
107+
if _XDIST_WORKER:
108+
os.environ["DATABASE_URL"] = _worker_database_url(os.environ["DATABASE_URL"], _XDIST_WORKER)
109+
_ensure_database(os.environ["DATABASE_URL"])
110+
45111
from app.config import settings # noqa: E402
46112
from app.db.base import Base # noqa: E402
47113
from app.db.session import get_db # noqa: E402
Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
"""Each parallel worker gets its own database, and nothing else moves.
2+
3+
The gotcha this guards, per TESTING.md's core rule, is a measured one rather
4+
than a hypothetical. `conftest.py`'s isolation model is "drop every table
5+
between tests", which is safe exactly as long as one process is doing it.
6+
Pointed at one database, four `pytest -n` workers do not merely fail - they
7+
wedge, each blocking on locks held by tables another worker is partway
8+
through dropping. The run that found this took 1785s before it was killed,
9+
against 60s for the same suite serially.
10+
11+
The fix is a database per worker, and it is one line of URL rewriting, which
12+
is the kind of thing that looks obviously correct and silently stops
13+
happening. These tests are what make it stay true: the first three pin the
14+
rewrite, and the last one checks it actually took effect in the process
15+
running right now - because a rewrite that quietly became a no-op would put
16+
every worker back on one database and buy back the deadlock.
17+
"""
18+
19+
from __future__ import annotations
20+
21+
import os
22+
23+
from sqlalchemy.engine import make_url
24+
25+
from app.config import settings
26+
from tests.conftest import _worker_database_url
27+
28+
SERIAL_URL = "postgresql+psycopg://ourhike:ourhike@localhost:5432/ourhike_test"
29+
30+
31+
def test_the_worker_name_lands_on_the_database_and_nowhere_else():
32+
"""Only the database changes - the server it is on must not.
33+
34+
Rewriting the wrong component is the failure that would not look like
35+
one: a URL pointing at a different host fails loudly, but one that
36+
silently kept the shared database name would pass this file's siblings
37+
and deadlock the moment somebody ran with `-n`.
38+
"""
39+
rewritten = make_url(_worker_database_url(SERIAL_URL, "gw0"))
40+
original = make_url(SERIAL_URL)
41+
42+
assert rewritten.database == "ourhike_test_gw0"
43+
assert rewritten.host == original.host
44+
assert rewritten.port == original.port
45+
assert rewritten.username == original.username
46+
assert rewritten.password == original.password
47+
assert rewritten.drivername == original.drivername
48+
49+
50+
def test_two_workers_never_land_on_the_same_database():
51+
"""The invariant the whole change exists for, stated directly.
52+
53+
Everything else here is detail; this is the property that makes parallel
54+
running safe at all, so it is asserted on its own rather than left to be
55+
inferred from the naming test above.
56+
"""
57+
databases = {make_url(_worker_database_url(SERIAL_URL, f"gw{n}")).database for n in range(8)}
58+
59+
assert len(databases) == 8
60+
61+
62+
def test_a_url_carrying_no_database_is_still_rewritten_per_worker():
63+
"""CI passes DATABASE_URL in, so the input is not always the default.
64+
65+
A URL whose database is absent must not collapse every worker onto one
66+
name - that is the deadlock again, arriving through a code path nobody
67+
ran locally.
68+
"""
69+
without = "postgresql+psycopg://ourhike:ourhike@localhost:5432/"
70+
71+
first = make_url(_worker_database_url(without, "gw0")).database
72+
second = make_url(_worker_database_url(without, "gw1")).database
73+
74+
assert first != second
75+
76+
77+
def test_this_process_is_really_on_the_database_its_worker_was_given():
78+
"""Guards the guard: the rewrite above is wired up, not just correct.
79+
80+
The three tests above would all pass on a `conftest.py` that computed the
81+
per-worker URL and then never applied it, which is the shape this is here
82+
to catch. Serially there is no worker and the URL is the plain one, and
83+
asserting that is worth as much - it is what says the parallel path stays
84+
out of the way of an ordinary `pytest` run.
85+
"""
86+
worker = os.environ.get("PYTEST_XDIST_WORKER")
87+
database = make_url(settings.database_url).database or ""
88+
89+
if worker:
90+
assert database.endswith(f"_{worker}")
91+
else:
92+
assert not database.startswith("ourhike_test_gw")

0 commit comments

Comments
 (0)