PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m2s
PR Checks / Backend Smoke (pull_request) Successful in 8s
PR Checks / Build Backend (no push) (pull_request) Successful in 17s
PR Checks / Build Frontend (no push) (pull_request) Successful in 44s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 10s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 2m34s
Two findings from review on #121, plus a third the review prompted. The cap at three authorities silently dropped the fourth in exactly the case where the information matters most — a genuinely fragmented place — and contradicted the stated goal of naming every authority a place sits in. It is gone. The share rule was always the real limit and already bounds the list at ten. Measured against the live corpus, one town would have been truncated today: LONDON, split evenly between Hackney, Lambeth, Westminster and Lewisham. parent_authority used mode() while authorities used value_counts(), and on an exact tie pandas does not guarantee the two pick the same name, so the 301 could have pointed somewhere other than the authority named first on the page. The parent is now derived from authorities[0]: one computation, one answer. It also inherits the sentinel filter, so a place can no longer redirect to /schools/authority/does-not-apply. Chasing the truncation case surfaced a worse bug. Places were grouped by raw town value, but the registry is keyed by slug, and GIAS spells the same place several ways. Five town slugs come from more than one spelling: "London" (1,819 schools) and "LONDON" (12) both slugify to `london`, so the later group simply overwrote the earlier one — /schools/london could have shown twelve schools, silently, depending on row order. Weston-super-Mare was split 14/19 across two spellings and Newcastle-under-Lyme across three. Grouping is now by slug, and the display name is the most common spelling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015mWQnpye9F299NVRCCSRvj
346 lines
14 KiB
Python
346 lines
14 KiB
Python
"""Tests for the place registry (spec 2026-08-21).
|
|
|
|
The registry is built from the in-memory school DataFrame, so these build a
|
|
small frame directly rather than touching a database.
|
|
"""
|
|
|
|
import numpy as np
|
|
import pandas as pd
|
|
import pytest
|
|
|
|
from backend.places import MIN_SCHOOLS, build_place_registry
|
|
|
|
|
|
def _df(rows: list[dict]) -> pd.DataFrame:
|
|
base = {
|
|
"year": 202425, "ofsted_grade": 2.0, "ofsted_date": None,
|
|
"rwm_expected_pct": 60.0, "attainment_8_score": np.nan,
|
|
"phase": "Primary", "postcode": "AA1 1AA",
|
|
}
|
|
return pd.DataFrame([{**base, **r} for r in rows])
|
|
|
|
|
|
def _town(n: int, town: str, la: str, start: int = 100000, **kw) -> list[dict]:
|
|
"""`start` offsets the URNs so two calls can describe different schools —
|
|
the Bedford case needs two authorities' worth of distinct URNs in one
|
|
town."""
|
|
return [
|
|
{"urn": start + i, "school_name": f"{town} School {i}",
|
|
"town": town, "local_authority": la, **kw}
|
|
for i in range(n)
|
|
]
|
|
|
|
|
|
def test_town_clearing_the_threshold_is_published():
|
|
reg = build_place_registry(_df(_town(MIN_SCHOOLS, "Brentwood", "Essex")))
|
|
assert "town:brentwood" in reg
|
|
assert reg["town:brentwood"].name == "Brentwood"
|
|
assert len(reg["town:brentwood"].urns) == MIN_SCHOOLS
|
|
|
|
|
|
def test_town_below_the_threshold_is_not_published():
|
|
reg = build_place_registry(_df(_town(MIN_SCHOOLS - 1, "Crosby", "Sefton")))
|
|
assert "town:crosby" not in reg
|
|
|
|
|
|
def test_a_town_below_threshold_still_names_its_authority():
|
|
# The route layer needs somewhere to 301 to.
|
|
reg = build_place_registry(_df(
|
|
_town(MIN_SCHOOLS - 1, "Crosby", "Sefton") + _town(MIN_SCHOOLS, "Bootle", "Sefton")))
|
|
assert "authority:sefton" in reg
|
|
|
|
|
|
def test_town_and_authority_of_the_same_name_are_separate_places():
|
|
# 67 real collisions. Neither set contains the other: Bedford the town has
|
|
# 104 schools, Bedford the authority 86, because postal towns cross
|
|
# authority boundaries.
|
|
rows = (_town(MIN_SCHOOLS, "Bedford", "Bedford")
|
|
+ _town(MIN_SCHOOLS, "Bedford", "Central Bedfordshire", start=200000))
|
|
reg = build_place_registry(_df(rows))
|
|
town, authority = reg["town:bedford"], reg["authority:bedford"]
|
|
assert set(town.urns) != set(authority.urns)
|
|
assert len(town.urns) == MIN_SCHOOLS * 2 # both authorities' schools
|
|
assert len(authority.urns) == MIN_SCHOOLS # only this authority's
|
|
|
|
|
|
def test_schools_without_publishable_data_do_not_count_toward_the_threshold():
|
|
rows = _town(MIN_SCHOOLS, "Ghosttown", "Nowhere")
|
|
for r in rows:
|
|
r["rwm_expected_pct"] = np.nan
|
|
r["ofsted_grade"] = np.nan
|
|
reg = build_place_registry(_df(rows))
|
|
assert "town:ghosttown" not in reg
|
|
|
|
|
|
def test_blank_town_is_ignored():
|
|
rows = _town(MIN_SCHOOLS, "", "Essex")
|
|
reg = build_place_registry(_df(rows))
|
|
assert not any(k.startswith("town:") for k in reg)
|
|
|
|
|
|
def test_a_school_is_counted_once_even_with_several_years_of_rows():
|
|
rows = []
|
|
for year in (202324, 202425):
|
|
rows += [{**r, "year": year} for r in _town(MIN_SCHOOLS, "Beccles", "Suffolk")]
|
|
reg = build_place_registry(_df(rows))
|
|
assert len(reg["town:beccles"].urns) == MIN_SCHOOLS
|
|
|
|
|
|
def test_locality_groups_schools_by_outcode(monkeypatch):
|
|
# The GIAS town field collapses 1,819 London schools into "London", so a
|
|
# locality is defined by its postcode districts instead.
|
|
from backend import localities
|
|
monkeypatch.setattr(localities, "LOCALITY_OUTCODES",
|
|
{"battersea": ("Battersea", ("SW11",))})
|
|
rows = _town(MIN_SCHOOLS, "London", "Wandsworth")
|
|
for r in rows:
|
|
r["postcode"] = "SW11 2AA"
|
|
reg = build_place_registry(_df(rows))
|
|
assert reg["locality:battersea"].name == "Battersea"
|
|
assert len(reg["locality:battersea"].urns) == MIN_SCHOOLS
|
|
|
|
|
|
def test_locality_below_the_threshold_is_not_published(monkeypatch):
|
|
from backend import localities
|
|
monkeypatch.setattr(localities, "LOCALITY_OUTCODES",
|
|
{"nowhere": ("Nowhere", ("ZZ99",))})
|
|
reg = build_place_registry(_df(_town(MIN_SCHOOLS, "London", "Wandsworth")))
|
|
assert "locality:nowhere" not in reg
|
|
|
|
|
|
def test_a_locality_may_not_shadow_a_viable_town(monkeypatch, caplog):
|
|
"""A colliding locality is skipped loudly, and the town survives.
|
|
|
|
This used to raise, which took down sitemap generation for all 25,000
|
|
school pages the first time a curated slug met a real GIAS town. Curated
|
|
data must not be able to break the site — and GIAS town names change with
|
|
no code change at all, so the raise could fire spontaneously.
|
|
"""
|
|
import logging
|
|
|
|
from backend import localities
|
|
monkeypatch.setattr(localities, "LOCALITY_OUTCODES",
|
|
{"brentwood": ("Brentwood", ("CM13",))})
|
|
rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
|
|
for r in rows:
|
|
r["postcode"] = "CM13 1AA"
|
|
|
|
with caplog.at_level(logging.ERROR):
|
|
reg = build_place_registry(_df(rows))
|
|
|
|
assert "locality:brentwood" not in reg # skipped
|
|
assert "town:brentwood" in reg # the town is untouched
|
|
assert "brentwood" in caplog.text # and it was loud about it
|
|
|
|
|
|
def test_a_locality_collision_does_not_break_the_rest_of_the_registry(monkeypatch):
|
|
# The whole point of skipping rather than raising.
|
|
from backend import localities
|
|
monkeypatch.setattr(localities, "LOCALITY_OUTCODES",
|
|
{"brentwood": ("Brentwood", ("CM13",))})
|
|
rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
|
|
for r in rows:
|
|
r["postcode"] = "CM13 1AA"
|
|
reg = build_place_registry(_df(rows))
|
|
assert "authority:essex" in reg
|
|
assert "outcode:cm13" in reg
|
|
|
|
|
|
def test_outcode_places_are_built_from_postcodes():
|
|
rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
|
|
for r in rows:
|
|
r["postcode"] = "CM13 1AA"
|
|
reg = build_place_registry(_df(rows))
|
|
assert reg["outcode:cm13"].name == "CM13"
|
|
assert len(reg["outcode:cm13"].urns) == MIN_SCHOOLS
|
|
|
|
|
|
def test_malformed_postcodes_do_not_create_places():
|
|
rows = _town(MIN_SCHOOLS, "Brentwood", "Essex")
|
|
for r in rows:
|
|
r["postcode"] = "not a postcode"
|
|
reg = build_place_registry(_df(rows))
|
|
assert not any(k.startswith("outcode:") for k in reg)
|
|
|
|
|
|
def test_every_curated_locality_is_structurally_valid():
|
|
# Guards the hand-maintained file: real slug, real name, real outcodes.
|
|
import re
|
|
from backend.localities import LOCALITY_OUTCODES
|
|
|
|
assert LOCALITY_OUTCODES, "the curated locality list must not be empty"
|
|
for slug, (name, outcodes) in LOCALITY_OUTCODES.items():
|
|
assert re.fullmatch(r"[a-z0-9-]+", slug), slug
|
|
assert name.strip() == name and name, slug
|
|
assert outcodes, f"{slug} has no outcodes"
|
|
for oc in outcodes:
|
|
assert re.fullmatch(r"[A-Z]{1,2}\d{1,2}[A-Z]?", oc), (slug, oc)
|
|
|
|
|
|
def test_the_pipeline_seed_mirrors_the_canonical_module():
|
|
"""Two copies with no drift guard is worse than one copy.
|
|
|
|
backend/localities.py is canonical because the backend image does not
|
|
contain pipeline/. The seed exists so the warehouse can join on the same
|
|
definitions, and this is what stops the two diverging — the same
|
|
arrangement assert_gias_code_names_match_seed.sql gives gias_codes.
|
|
"""
|
|
import csv
|
|
from pathlib import Path
|
|
|
|
from backend.localities import LOCALITY_OUTCODES
|
|
|
|
seed_path = (Path(__file__).resolve().parents[2]
|
|
/ "pipeline/transform/seeds/locality_outcodes.csv")
|
|
assert seed_path.exists(), f"missing seed mirror at {seed_path}"
|
|
|
|
seed = {
|
|
row["locality_slug"]: (row["locality_name"],
|
|
tuple(row["outcodes"].split("|")))
|
|
for row in csv.DictReader(seed_path.open())
|
|
}
|
|
assert seed == LOCALITY_OUTCODES
|
|
|
|
|
|
def test_no_curated_locality_names_a_london_borough():
|
|
"""Boroughs are authorities and already have a page.
|
|
|
|
A locality defined by two or three outcodes inside a borough would be a
|
|
partial, near-duplicate subset of that authority page — the exact
|
|
thin-content failure the two-namespace design exists to avoid. Hackney,
|
|
Islington, Greenwich and Ealing were all in the first draft.
|
|
|
|
Hardcoded rather than read from the corpus because this must fail in CI,
|
|
where there is no database.
|
|
"""
|
|
from backend.localities import LOCALITY_OUTCODES
|
|
|
|
boroughs = {
|
|
"barking-and-dagenham", "barnet", "bexley", "brent", "bromley",
|
|
"camden", "croydon", "ealing", "enfield", "greenwich", "hackney",
|
|
"hammersmith-and-fulham", "haringey", "harrow", "havering",
|
|
"hillingdon", "hounslow", "islington", "kensington-and-chelsea",
|
|
"kingston-upon-thames", "lambeth", "lewisham", "merton", "newham",
|
|
"redbridge", "richmond-upon-thames", "southwark", "sutton",
|
|
"tower-hamlets", "waltham-forest", "wandsworth", "westminster",
|
|
}
|
|
named = boroughs & set(LOCALITY_OUTCODES)
|
|
assert not named, (
|
|
f"these are boroughs, not districts: {sorted(named)} - they already "
|
|
"have an authority page covering every school"
|
|
)
|
|
|
|
|
|
def test_a_place_names_every_authority_it_straddles():
|
|
"""SW19 is mostly Merton but partly Wandsworth.
|
|
|
|
A quarter of viable outcodes and a third of viable towns cross an
|
|
authority boundary, so naming only the largest asserts something false.
|
|
"""
|
|
rows = (_town(26, "London", "Merton", start=300000)
|
|
+ _town(7, "London", "Wandsworth", start=400000))
|
|
for r in rows:
|
|
r["postcode"] = "SW19 1AA"
|
|
reg = build_place_registry(_df(rows))
|
|
|
|
names = [n for n, _ in reg["outcode:sw19"].authorities]
|
|
assert names == ["Merton", "Wandsworth"] # largest first
|
|
assert dict(reg["outcode:sw19"].authorities)["Wandsworth"] == 7
|
|
|
|
|
|
def test_the_redirect_target_stays_a_single_authority():
|
|
# parent_authority and authorities do different jobs: a 301 needs one
|
|
# target, the page needs the truth.
|
|
rows = (_town(26, "London", "Merton", start=300000)
|
|
+ _town(7, "London", "Wandsworth", start=400000))
|
|
for r in rows:
|
|
r["postcode"] = "SW19 1AA"
|
|
reg = build_place_registry(_df(rows))
|
|
assert reg["outcode:sw19"].parent_authority == "Merton"
|
|
|
|
|
|
def test_a_stray_authority_below_the_share_threshold_is_not_named():
|
|
# GIAS carries postcode errors — EN6 lists two Shropshire schools among
|
|
# fourteen in Hertfordshire. Printing those as though real would be worse
|
|
# than omitting them.
|
|
rows = (_town(30, "Barnet", "Hertfordshire", start=300000)
|
|
+ _town(1, "Barnet", "Shropshire", start=400000))
|
|
for r in rows:
|
|
r["postcode"] = "EN6 1AA"
|
|
reg = build_place_registry(_df(rows))
|
|
assert [n for n, _ in reg["outcode:en6"].authorities] == ["Hertfordshire"]
|
|
|
|
|
|
def test_a_sentinel_authority_is_never_named():
|
|
rows = (_town(20, "London", "Merton", start=300000)
|
|
+ _town(6, "London", "Does not apply", start=400000))
|
|
for r in rows:
|
|
r["postcode"] = "SW19 1AA"
|
|
reg = build_place_registry(_df(rows))
|
|
assert [n for n, _ in reg["outcode:sw19"].authorities] == ["Merton"]
|
|
|
|
|
|
def test_a_place_always_names_at_least_one_authority():
|
|
# Even when every authority is below the share threshold, the page has to
|
|
# say where the place is.
|
|
rows = []
|
|
for i, la in enumerate(["A", "B", "C", "D", "E", "F", "G"]):
|
|
rows += _town(1, "Fragmented", la, start=300000 + i * 100)
|
|
reg = build_place_registry(_df(rows))
|
|
place = reg.get("town:fragmented")
|
|
assert place is not None
|
|
assert len(place.authorities) == 1
|
|
|
|
|
|
def test_every_qualifying_authority_is_named_with_no_cap():
|
|
"""An earlier cut stopped at three, dropping the fourth silently.
|
|
|
|
That truncation bit exactly where the information matters most — a
|
|
genuinely fragmented place — and nothing recorded it.
|
|
"""
|
|
rows = []
|
|
for i, la in enumerate(["Hackney", "Lambeth", "Westminster", "Lewisham"]):
|
|
rows += _town(3, "Fourway", la, start=300000 + i * 100)
|
|
reg = build_place_registry(_df(rows))
|
|
assert len(reg["town:fourway"].authorities) == 4
|
|
|
|
|
|
def test_the_redirect_target_is_the_authority_named_first():
|
|
"""They were computed separately — mode() against value_counts() — and on
|
|
an exact tie pandas does not guarantee the two agree."""
|
|
rows = (_town(26, "London", "Merton", start=300000)
|
|
+ _town(7, "London", "Wandsworth", start=400000))
|
|
for r in rows:
|
|
r["postcode"] = "SW19 1AA"
|
|
place = build_place_registry(_df(rows))["outcode:sw19"]
|
|
assert place.parent_authority == place.authorities[0][0]
|
|
|
|
|
|
def test_a_place_never_redirects_to_a_sentinel_authority():
|
|
# Deriving the parent from `authorities` inherits its sentinel filter.
|
|
rows = (_town(6, "Someplace", "Does not apply", start=300000)
|
|
+ _town(5, "Someplace", "Essex", start=400000))
|
|
reg = build_place_registry(_df(rows))
|
|
assert reg["town:someplace"].parent_authority == "Essex"
|
|
|
|
|
|
def test_spellings_of_one_place_are_merged_not_overwritten():
|
|
"""GIAS spells the same place several ways, and they share a URL.
|
|
|
|
"London" (1,819 schools) and "LONDON" (12) both slugify to `london`.
|
|
Grouping by raw value let the later group overwrite the earlier one, so
|
|
the page could have shown twelve schools instead of 1,819 — silently, and
|
|
depending on row order.
|
|
"""
|
|
rows = (_town(6, "Weston-super-Mare", "North Somerset", start=300000)
|
|
+ _town(5, "Weston-Super-Mare", "North Somerset", start=400000))
|
|
reg = build_place_registry(_df(rows))
|
|
assert len(reg["town:weston-super-mare"].urns) == 11
|
|
|
|
|
|
def test_the_merged_place_takes_its_most_common_spelling():
|
|
rows = (_town(9, "Newcastle-under-Lyme", "Staffordshire", start=300000)
|
|
+ _town(5, "NEWCASTLE-UNDER-LYME", "Staffordshire", start=400000))
|
|
reg = build_place_registry(_df(rows))
|
|
assert reg["town:newcastle-under-lyme"].name == "Newcastle-under-Lyme"
|