perf(places): index the reverse lookup, and isolate the registry in tests
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m11s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m17s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 1m47s
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m11s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m17s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 1m47s
Two review findings, both confirmed before fixing.
The client fixture in test_school_details.py patched load_school_data
but not _place_registry, which is a module-level cache. A probe settled
it rather than an argument: poisoning the global with a registry built
from data the fixture never saw, then issuing the fixture's own
request, returned that other dataset's places. So the new
`places == []` assertion was satisfied by a stale registry exactly as
well as by the fixture's own data, and proved nothing. Every other test
that touches place data already reset it; the fixture predates places
existing and was never updated. It resets it now.
places_for_urn walked every place in the registry and did a tuple
membership test against each, on /api/schools/{urn}, the site's
highest-traffic endpoint. It now reads a dict built once per registry.
Measured against a synthetic corpus of 27k schools in 1,650 places:
0.118ms per request becomes 0.0001ms, with the index built once in
21ms. Production carries ~5,000 places, so the scan there is larger
again. The absolute saving per request is small; the point is that it
is repeated on every school page view and costs nothing to remove.
The index is cached against the registry by identity rather than
behind a second flag. Anything that drops _place_registry — every test
that touches place data does — gets a fresh registry object, which no
longer matches what the index was built from, so the index rebuilds
with it. A separate _place_index = None would be one more thing to
forget, and a stale reverse index is precisely the first finding's bug
wearing a different hat.
That invalidation has its own test, and the test was checked by
breaking the identity check: five tests fail without it, so three
existing ones were already relying on it too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DXnXQKnPpZBBP61fBQiFkq
This commit is contained in:
1 parent
d65eb58883
commit
b0d5334e06
4 files changed
+124
-18
No files matched your search
@@ -56,6 +56,11 @@ def client(monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
app_module, "get_supplementary_data", lambda db, urn: {}
|
||||
)
|
||||
# The place registry is a module-level cache, so without this the endpoint
|
||||
# answers from whatever registry an earlier test happened to leave behind
|
||||
# — and a `places == []` assertion is satisfied by a stale registry just
|
||||
# as well as by this fixture's own data, which makes it prove nothing.
|
||||
monkeypatch.setattr(app_module, "_place_registry", None)
|
||||
return TestClient(app_module.app, raise_server_exceptions=False)
|
||||
|
||||
|
||||
@@ -212,3 +217,31 @@ def test_a_school_absent_from_the_phase_page_is_not_linked_to_it(monkeypatch):
|
||||
# The town publishes a primary page, but this secondary school is not on
|
||||
# it, and there are too few secondaries for a secondary page.
|
||||
assert town["phases"] == []
|
||||
|
||||
|
||||
def test_the_place_index_rebuilds_when_the_registry_is_replaced(monkeypatch):
|
||||
"""The reverse index is cached; a stale one would put another dataset's
|
||||
places on a school page. Invalidation is an identity check against the
|
||||
registry rather than a second flag, so this asserts the check works."""
|
||||
from backend import app as app_module
|
||||
|
||||
monkeypatch.setattr(app_module, "_place_registry", None)
|
||||
monkeypatch.setattr(app_module, "_place_index", None)
|
||||
monkeypatch.setattr(app_module, "_place_index_source", None)
|
||||
monkeypatch.setattr(app_module, "load_school_data", _brentwood_df("Primary"))
|
||||
|
||||
first = app_module.get_place_index()
|
||||
assert 100000 in first
|
||||
|
||||
# Same registry object, so the index is reused rather than rebuilt.
|
||||
assert app_module.get_place_index() is first
|
||||
|
||||
# Drop the registry the way every test that touches place data does. The
|
||||
# index must follow it, not survive it.
|
||||
app_module._place_registry = None
|
||||
monkeypatch.setattr(app_module, "load_school_data",
|
||||
_brentwood_df("Primary", n=0))
|
||||
|
||||
rebuilt = app_module.get_place_index()
|
||||
assert rebuilt is not first
|
||||
assert 100000 not in rebuilt, "the index outlived the registry it came from"
|
||||
Reference in new issue
Block a user