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
248 lines
10 KiB
Python
248 lines
10 KiB
Python
"""Regression tests for GET /api/schools/{urn}.
|
|
|
|
Schools with no performance rows (special post-16 institutions, sixth-form
|
|
centres, PRUs, brand-new schools) come back from the marts LEFT JOIN with
|
|
NaN in every numeric column. The endpoint must still serialize them — a NaN
|
|
that reaches Starlette's JSONResponse raises ValueError (allow_nan=False)
|
|
and the route 500s, which the frontend then renders as a 404.
|
|
"""
|
|
|
|
import numpy as np
|
|
import pandas as pd
|
|
import pytest
|
|
from fastapi.testclient import TestClient
|
|
|
|
|
|
def _no_results_school_df() -> pd.DataFrame:
|
|
"""One school row as produced by the marts query for a school with no
|
|
performance data: GIAS/location fields partly populated, every
|
|
results-linked column NaN (including year)."""
|
|
return pd.DataFrame(
|
|
[
|
|
{
|
|
"urn": 150275,
|
|
"school_name": "West London Performing Arts Academy",
|
|
"phase": "Secondary",
|
|
"school_type": "Special post 16 institution",
|
|
"trust_name": None,
|
|
"religious_denomination": "Does not apply",
|
|
"gender": None,
|
|
"age_range": "16-25",
|
|
"admissions_policy": None,
|
|
"capacity": np.nan,
|
|
"gias_total_pupils": np.nan,
|
|
"headteacher_name": None,
|
|
"website": None,
|
|
"ofsted_grade": np.nan,
|
|
"local_authority": "Ealing",
|
|
"address": "268 Northfield Avenue, London, W5 4UB",
|
|
"postcode": "W5 4UB",
|
|
"latitude": 51.4986,
|
|
"longitude": -0.3148,
|
|
"year": np.nan,
|
|
"total_pupils": np.nan,
|
|
"eligible_pupils": np.nan,
|
|
"rwm_expected_pct": np.nan,
|
|
}
|
|
]
|
|
)
|
|
|
|
|
|
@pytest.fixture()
|
|
def client(monkeypatch):
|
|
from backend import app as app_module
|
|
|
|
monkeypatch.setattr(app_module, "load_school_data", _no_results_school_df)
|
|
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)
|
|
|
|
|
|
def test_school_without_performance_rows_returns_200(client):
|
|
resp = client.get("/api/schools/150275")
|
|
assert resp.status_code == 200, resp.text
|
|
|
|
|
|
def test_nan_gias_fields_serialize_as_null(client):
|
|
info = client.get("/api/schools/150275").json()["school_info"]
|
|
assert info["capacity"] is None
|
|
assert info["total_pupils"] is None
|
|
assert info["school_name"] == "West London Performing Arts Academy"
|
|
|
|
|
|
# ── Links out to the location layer ─────────────────────────────────────────
|
|
#
|
|
# School pages carried no link into the site at all: the only anchor on the
|
|
# template pointed at the school's own website, so ~27k pages received
|
|
# whatever authority the site had and sent it off-site. `places` is what the
|
|
# link module and the breadcrumb are built from.
|
|
|
|
def test_places_is_present_even_when_the_school_belongs_to_none(client):
|
|
# This fixture's single school cannot clear any publish threshold, so the
|
|
# honest answer is an empty list. The key must still be there: a missing
|
|
# key and "no places" are different things to the page rendering it.
|
|
body = client.get("/api/schools/150275").json()
|
|
assert body["places"] == []
|
|
|
|
|
|
def test_places_names_only_pages_that_exist(monkeypatch):
|
|
from backend import app as app_module
|
|
from backend.places import MIN_SCHOOLS
|
|
|
|
def _df():
|
|
return pd.DataFrame([
|
|
{
|
|
"urn": 100000 + i,
|
|
"school_name": f"Brentwood School {i}",
|
|
"town": "Brentwood",
|
|
"local_authority": "Essex",
|
|
"postcode": "CM15 8AA",
|
|
"phase": "Primary",
|
|
"year": 202425,
|
|
"rwm_expected_pct": 60.0,
|
|
"attainment_8_score": np.nan,
|
|
"ofsted_grade": 2.0,
|
|
"ofsted_date": None,
|
|
}
|
|
for i in range(MIN_SCHOOLS)
|
|
])
|
|
|
|
monkeypatch.setattr(app_module, "load_school_data", _df)
|
|
monkeypatch.setattr(app_module, "get_supplementary_data", lambda db, urn: {})
|
|
monkeypatch.setattr(app_module, "_place_registry", None)
|
|
client = TestClient(app_module.app, raise_server_exceptions=False)
|
|
|
|
places = client.get("/api/schools/100000").json()["places"]
|
|
assert places, "a school in a published town must offer links"
|
|
|
|
by_kind = {p["kind"]: p for p in places}
|
|
assert by_kind["town"]["url"] == "/schools/brentwood"
|
|
assert by_kind["authority"]["url"] == "/schools/authority/essex"
|
|
|
|
# Every entry carries what the link text needs, and a count, so the anchor
|
|
# can say what it leads to rather than "click here".
|
|
for place in places:
|
|
assert place["name"]
|
|
assert place["count"] >= 1
|
|
assert place["url"].startswith("/schools/")
|
|
|
|
|
|
def _brentwood_df(phase: str = "Primary", n: int = None):
|
|
from backend.places import MIN_SCHOOLS
|
|
n = n if n is not None else MIN_SCHOOLS
|
|
return lambda: pd.DataFrame([
|
|
{
|
|
"urn": 100000 + i,
|
|
"school_name": f"Brentwood School {i}",
|
|
"town": "Brentwood", "local_authority": "Essex",
|
|
"postcode": "CM15 8AA", "phase": phase, "year": 202425,
|
|
"rwm_expected_pct": 60.0, "attainment_8_score": 50.0,
|
|
"ofsted_grade": 2.0, "ofsted_date": None,
|
|
}
|
|
for i in range(n)
|
|
])
|
|
|
|
|
|
def _places_for(monkeypatch, df_factory, urn: int):
|
|
from backend import app as app_module
|
|
monkeypatch.setattr(app_module, "load_school_data", df_factory)
|
|
monkeypatch.setattr(app_module, "get_supplementary_data", lambda db, urn: {})
|
|
monkeypatch.setattr(app_module, "_place_registry", None)
|
|
client = TestClient(app_module.app, raise_server_exceptions=False)
|
|
return client.get(f"/api/schools/{urn}").json()["places"]
|
|
|
|
|
|
def test_a_place_offers_the_phase_page_this_school_appears_on(monkeypatch):
|
|
# "primary schools in brentwood" is the query the phase pages exist for,
|
|
# and ~950 of them were once reachable by nothing at all.
|
|
places = _places_for(monkeypatch, _brentwood_df("Primary"), 100000)
|
|
town = next(p for p in places if p["kind"] == "town")
|
|
|
|
assert town["phases"], "a primary school in a published primary town has a link"
|
|
assert town["phases"][0]["url"] == "/schools/brentwood/primary"
|
|
assert town["phases"][0]["count"] >= 1
|
|
|
|
|
|
def test_an_all_through_school_offers_both_phase_pages(monkeypatch):
|
|
# It genuinely appears on both, so there is no tie to break.
|
|
places = _places_for(monkeypatch, _brentwood_df("All-through"), 100000)
|
|
town = next(p for p in places if p["kind"] == "town")
|
|
|
|
assert {p["phase"] for p in town["phases"]} == {"primary", "secondary"}
|
|
|
|
|
|
def test_outcodes_never_offer_a_phase_page(monkeypatch):
|
|
# The registry gives outcodes no phase route — nobody searches "primary
|
|
# schools in SW11" — and computing them anyway once put a link to a
|
|
# nonexistent route on all 1,720 outcode pages.
|
|
places = _places_for(monkeypatch, _brentwood_df("Primary"), 100000)
|
|
outcode = next((p for p in places if p["kind"] == "outcode"), None)
|
|
|
|
if outcode is not None:
|
|
assert outcode["phases"] == []
|
|
|
|
|
|
def test_a_school_absent_from_the_phase_page_is_not_linked_to_it(monkeypatch):
|
|
# The check is URN membership in the registry's own phase list, not a
|
|
# re-derivation of the phase mapping. A secondary school must not be sent
|
|
# to a primary phase page that does not list it.
|
|
from backend.places import MIN_SCHOOLS
|
|
|
|
def df():
|
|
rows = [
|
|
{"urn": 100000 + i, "school_name": f"P{i}", "town": "Brentwood",
|
|
"local_authority": "Essex", "postcode": "CM15 8AA",
|
|
"phase": "Primary", "year": 202425, "rwm_expected_pct": 60.0,
|
|
"attainment_8_score": np.nan, "ofsted_grade": 2.0,
|
|
"ofsted_date": None}
|
|
for i in range(MIN_SCHOOLS)
|
|
]
|
|
rows.append({
|
|
"urn": 900000, "school_name": "Lone Secondary", "town": "Brentwood",
|
|
"local_authority": "Essex", "postcode": "CM15 8AA",
|
|
"phase": "Secondary", "year": 202425, "rwm_expected_pct": np.nan,
|
|
"attainment_8_score": 50.0, "ofsted_grade": 2.0, "ofsted_date": None,
|
|
})
|
|
return pd.DataFrame(rows)
|
|
|
|
places = _places_for(monkeypatch, df, 900000)
|
|
town = next(p for p in places if p["kind"] == "town")
|
|
|
|
# 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"
|