diff --git a/.github/workflows/run_pytest.yml b/.github/workflows/run_pytest.yml index a782614..09b7f24 100644 --- a/.github/workflows/run_pytest.yml +++ b/.github/workflows/run_pytest.yml @@ -75,6 +75,29 @@ jobs: path: _siblings/views-appwrite fetch-depth: 0 + # Fetched for the checks that need only views-datafactory's TRACKED files. The one + # that motivated it is `test_delivery_coverage.py::test_manifest_matches_datafactory_ + # land_minus_land_gaul`, C-30's drift tripwire on the 76-cell exclusion manifest: it + # reads `src/datafactory_query/{land,land_gaul}_pgids.json`, both tracked here, and + # until 2026-08-17 it ran only on a laptop — while this repository told a partner in + # writing that the manifest "cannot drift without failing loudly". It could; nothing + # in the gate was watching. Three more checks came with it (the release gate, the + # region-set check, the wire-cast dtype check); see C-46 for the full accounting. + # + # This checkout was tried on 2026-08-03 and reverted, and the revert note said the + # sibling could not be fetched because its GAUL parquets are untracked. That was the + # wrong diagnosis. `data/raw/gaul_admin/` IS tracked (it holds a geojson); only the + # parquets are not — so `test_gaul_lookup_fidelity`'s `.is_dir()` gate passed and the + # comparison died on FileNotFoundError. The gate now checks for the seven parquets + # themselves, so that half skips honestly here and still runs where they exist. + - name: Checkout views-datafactory (sibling) + uses: actions/checkout@v3 + with: + repository: views-platform/views-datafactory + ref: main + path: _siblings/views-datafactory + fetch-depth: 0 + - name: Set up Python uses: actions/setup-python@v4 with: @@ -97,6 +120,7 @@ jobs: - name: Run tests env: VIEWS_APPWRITE: ${{ github.workspace }}/_siblings/views-appwrite + VIEWS_DATAFACTORY: ${{ github.workspace }}/_siblings/views-datafactory run: | set -e poetry run pytest tests/ diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index c6a0890..571a59e 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -5,8 +5,8 @@ | Project | views-postprocessing | | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-16 | -| Total Concerns | 107 | -| Open Concerns | 27 | +| Total Concerns | 108 | +| Open Concerns | 28 | | Resolved Concerns | 80 | --- @@ -817,6 +817,10 @@ Verified 2026-06-12: of the datafactory's 64,818 `land`-region cells, 64,736 hav **Mitigation landed (S4, 2026-06-26, `sprint/fao-input-integrity`):** the 76 excluded gids are pinned as a frozen manifest in `delivery/coverage.py` (`EXCLUDED_GIDS_BY_REGION`), the count is corrected to 64,742, `assert_no_excluded_cells` is wired into the manager's `_check_coverage` **region-gated** (a no-op for unpinned `africa_me_legacy`, so its 5 ocean cells are unaffected), the 76 are disclosed in `docs/fao_excluded_cells.md`, and a test cross-checks the manifest against the datafactory sibling when present (drift tripwire). **Residual:** still Tier 1 until the live `land_gaul` run (views-platform/views-models#127) exercises it end-to-end — the guard is unit-proven but not yet run against a real global delivery. +**The tripwire now runs in the gate, and it did not until 2026-08-17.** *"When present"* meant a developer laptop: views-datafactory was not fetched in CI, so `test_manifest_matches_datafactory_land_minus_land_gaul` skipped on every pull request. That mattered more than it looked, because on 2026-08-17 this repository told FAO in writing that the exclusion list *"is frozen in code and asserted against the producer in our test suite, so it cannot drift without failing loudly"* — a guarantee the gate was not carrying. The sibling is now fetched (see C-46 for why the earlier attempt was reverted and why that reason did not survive checking), and the tripwire reads `src/datafactory_query/{land,land_gaul}_pgids.json`, both of which views-datafactory tracks. + +This does **not** move the tier. The residual above is unchanged: the guard is now enforced continuously rather than incidentally, but what holds C-30 at Tier 1 is the absence of a real global delivery exercising it end-to-end, and no CI wiring supplies that. + **RESIDUAL DISCHARGED 2026-07-27 — run-0 exercised the guard live.** The stated residual was "*still Tier 1 until the live `land_gaul` run (views-models#127) exercises it end-to-end — the guard is unit-proven but not yet run against a real global delivery.*" **Run-0 delivered on 2026-07-27** against producer run `rusty_bucket_forecasting_20260727_095355`: `region=land_gaul`, coverage gate reported **64,742 distinct cells / 28,356,996 rows** for the historical frame, the forecast leg shipped 108 shards + sidecar + manifest, and the process exited cleanly with no loud failures. The pinned count and the 76-gid exclusion manifest were both correct against a real global delivery. **Tier recalibrated from 1 to 2 during review-rr (2026-07-31):** the silent-corruption path is now guarded and proven, so what remains is regression risk under upstream change — which is exactly what the rewritten trigger watches. **MERGED: C-34 (Spatial coverage has no contract) absorbed here, review-rr 2026-07-31.** C-34 registered the absence of any expected-cell-count assertion, with the coverage decision split across three repos (views-models region string → views-datafactory cell-set → consequences here). Both concerns are now implemented by **one module** (`delivery/coverage.py`) and were discharged by **one event** (run-0), so tracking them separately doubled the maintenance without adding signal. C-34's distinctive contribution — that the trigger is an *upstream* region/cell-set change in either of two other repos — is carried into the merged trigger and Location above. C-34 remains as a forwarding stub in Resolved Concerns. @@ -1150,6 +1154,26 @@ Cross-refs: **C-80** (the same guard, the adjacent corpus gap, resolved by widen --- +### C-108: Two `xfail(strict=True)` deploy gates have never evaluated their own assertions — anywhere + +| Field | Value | +|-------|-------| +| ID | C-108 | +| Tier | 4 — no delivery correctness depends on them. Registered because `xfail(strict=True)` *reads* as an armed tripwire, and a future maintainer will believe views-datafactory#223 is being watched when nothing is watching it. | +| Source | `/code-review high` on PR #280, 2026-08-17 (finding 2), extended by measurement | +| Trigger | When views-datafactory#223 is closed, or when anyone cites these gates as evidence that the served artifact is being tracked — check they are not skipping first. | +| Owner | This repository for the gate; views-datafactory for the artifacts. | +| Location | `tests/test_datafactory_deploy_readiness.py` — `TestServedArtifactMatchesBranch::test_assembled_grid_not_older_than_gaul_parquets`, `TestServedArtifactProvenanceTracksGaul::test_provenance_includes_admin_digest` | + +Both gates read `data/assembled/grid.npy`, `data/assembled/provenance.json` and the GAUL parquets from the views-datafactory checkout. **None of those is tracked upstream, and `data/assembled/` is empty in the maintainer's own checkout** (measured 2026-08-17). So the tests were failing on a missing file, `xfail(strict=True)` was recording that as an expected failure, and the report read green. The staleness comparison and the `admin_digest` assertion — the things the gates exist to make — have never once been evaluated. + +The strict flip is the entire mechanism: when views-datafactory#223 is fixed the test should XPASS and turn the build red, forcing someone to look. A test that can only ever fail on `FileNotFoundError` can never XPASS, so the flip could not fire. ADR-014 §1 — a guarantee is attached to a check, or it is not a guarantee — and C-102's lesson recurring in a form that is harder to see, because here the guard *runs*. + +**Partially addressed in the same PR**, and deliberately only partially: both tests now `pytest.skip()` when their inputs are absent, so the state is visible in the report instead of disguised as a passing xfail. That converts a false green into an honest skip. It does **not** make the gate work — closing that needs the assembled artifacts reachable from CI, which is the same blocker as C-46's producer-comparison half and is not this repository's to solve. + +Cross-refs: **C-46** (the untracked-artifact blocker these share), **C-102** (a guard that has never run is unproven), **C-36** (the gates' original home). + + ## Disagreements ### D-12: Post-Run-0 infrastructure & naming intents — repo rename, internal-store transport, compute co-location @@ -2150,6 +2174,23 @@ Verified 2026-08-02: `grep -rn "/home/" tests/ scripts/ views_postprocessing/ -- **What is genuinely accepted, and should not be glossed:** a change merged to a sibling's `main` — a registry edition bump, say — *can* turn this repository red and block merges here until someone re-pins. That is not a defect being tolerated; it is the drift detector working, and the alternative is the state this entry was open about, where the drift was noticed only when a maintainer happened to run the suite. The cost is real and the trade is deliberate. +**The last residual closed 2026-08-17, and the reason it stayed open for two weeks is the finding.** ADR-016 added sibling checkouts but fetched only views-appwrite, so this entry's own subject — the deploy gate — still ran nowhere but a laptop. The stated blocker was that views-datafactory's GAUL parquets are untracked, so a checkout would turn honest skips into `FileNotFoundError`; that was tried on 2026-08-03 and reverted. **The observation was right and the diagnosis was wrong.** `test_gaul_lookup_fidelity` gated on `data/raw/gaul_admin/` being a *directory*, and that directory **is** tracked — it holds `supplement_azores.geojson` — while the seven parquets beside it are not. So a checkout satisfied the gate, the comparison ran, and it died. The sibling was never the problem; the gate was asking whether a folder existed when it needed to ask whether the files it reads existed. + +Reproduced 2026-08-17 against a tracked-files-only worktree (`git worktree add --detach`, which contains exactly what `actions/checkout` produces): `1 failed, 37 passed, 1 skipped`, the failure being `FileNotFoundError: .../gaul0_code.parquet`. After re-gating on the seven parquets themselves: no failures. views-datafactory is now fetched in `run_pytest.yml` and declared `ci_checkout=True`. + +**A second, larger instance of the same mistake was found while fixing the first.** All four sibling-aware tests in `test_gaul_lookup_fidelity.py` shared **one** gate keyed to the parquets — including two that read no parquet and one that reads no sibling at all. So they sat dark in CI for no reason anybody had chosen: + +| test | actually reads | was gated on | +|---|---|---| +| `test_lookup_values_match_the_producer_parquets` | the 7 GAUL parquets (untracked) | parquets — correct | +| `test_lookup_gid_set_equals_the_declared_region` | `land_gaul_pgids.json` — **tracked** | parquets | +| `test_coordinate_formula_matches_every_priogrid_cell` | `priogrid_cell.dbf`, and self-skips on it | parquets | +| `test_coord_dtypes_are_wire_stable` | **only the committed lookup** | parquets | + +The last one matters beyond tidiness: it is the check that `CODE_COLS` survive the §5.1 int64→float64 wire cast losslessly (`abs(v) < 2**53`) — the property ADR-013 §5.1a and the 2026-08-17 mail to FAO both rest on — and it needs no sibling whatsoever. Each test now gates on the artifact it reads. + +Measured across the whole change, CI goes from **426 passed / 6 skipped** to **430 passed / 3 skipped**: four checks move from skipped to running — C-30's exclusion tripwire, this entry's `TestReleaseGate::test_land_gaul_commit_is_in_a_release_tag`, the region-set check, and the wire-cast dtype check. The producer-comparison half still skips, honestly, and still needs the parquets published somewhere fetchable. + The cross-repo deploy-readiness gates introduced under C-36 are guarded by `skipif` on a **hardcoded local datafactory checkout path**, so they are **skipped in CI** and only ever execute on one developer's machine. There, `test_version_bumped_past_latest_tag` is currently **failing**: it is an `xfail(strict)` that flipped to XPASS because datafactory moved to `1.5.0`-dev past its `v1.4.0` tag — exactly the auto-flip C-36's resolution anticipated, but because of the hardcoded path the flip surfaces as a **local red** rather than a CI signal, and breaks local `pytest` runs (the suite is run with this test deselected). No correctness/reliability impact on the delivery → **Tier 4** (test hygiene). C-36 (resolved) converted these gates to strict-xfail but did not capture the local-path / CI-skip dimension. See also C-36 (the resolved strict-xfail conversion this extends), C-44 (the datafactory version-state coupling). diff --git a/tests/conftest.py b/tests/conftest.py index 8fdde29..65cf8b1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -122,13 +122,24 @@ class Sibling: SIBLINGS = { "views-datafactory": Sibling( env="VIEWS_DATAFACTORY", - ci_checkout=False, + ci_checkout=True, note=( - "PUBLIC, but its checks need the producer's raw GAUL parquets " - "(data/raw/gaul_admin/*.parquet), which are NOT in its git repository. " - "Checking it out converts an honest skip into a FileNotFoundError — measured " - "2026-08-03, tried and reverted. Closing this needs the data published " - "somewhere fetchable, not an access grant. Register C-46." + "PUBLIC, and fetched since 2026-08-17. The 2026-08-03 revert was real but " + "its cause was misread: `test_gaul_lookup_fidelity` gated on " + "`data/raw/gaul_admin/` being a DIRECTORY, and that directory IS tracked " + "(it holds supplement_azores.geojson) while the parquets beside it are not. " + "So a checkout satisfied the gate, the comparison ran, and it died on " + "FileNotFoundError — which read as 'this sibling cannot be checked out' when " + "it was 'that gate asks the wrong question'. Reproduced against a " + "tracked-files-only worktree on 2026-08-17, then fixed by gating on the " + "seven parquets themselves. " + "What the fetch buys: `test_delivery_coverage.py::" + "test_manifest_matches_datafactory_land_minus_land_gaul` reads " + "`src/datafactory_query/{land,land_gaul}_pgids.json`, both of which ARE " + "tracked, so C-30's exclusion-manifest drift tripwire now runs in the gate " + "rather than only on a laptop. The value-fidelity half still skips, honestly, " + "and closing THAT still needs the parquets published somewhere fetchable " + "rather than an access grant. Register C-46, C-30." ), ), "views-appwrite": Sibling( diff --git a/tests/test_datafactory_deploy_readiness.py b/tests/test_datafactory_deploy_readiness.py index acadef8..22be04b 100644 --- a/tests/test_datafactory_deploy_readiness.py +++ b/tests/test_datafactory_deploy_readiness.py @@ -105,7 +105,17 @@ class TestServedArtifactMatchesBranch: def test_assembled_grid_not_older_than_gaul_parquets(self): grid = _DF / "data/assembled/grid.npy" parquet = _DF / "data/raw/gaul_admin/gaul0_code.parquet" - assert grid.exists() and parquet.exists() + # Neither is tracked in views-datafactory. Before 2026-08-17 the module-level + # skipif covered that; now CI fetches the sibling, so without this the test + # xfails on a MISSING FILE rather than on the staleness it asserts — and a + # strict xfail that can only ever xfail can never flip, which is the entire + # mechanism (ADR-014 §1). + if not (grid.exists() and parquet.exists()): + pytest.skip( + "the assembled grid and/or GAUL parquets are absent — they are not " + "tracked in views-datafactory, so a checkout alone cannot answer this. " + "Skipping rather than xfailing keeps the strict flip meaningful." + ) assert grid.stat().st_mtime >= parquet.stat().st_mtime, ( "assembled grid is older than the GAUL parquets — re-assemble and " "re-export the zarr before deploying, or the served GAUL channels " @@ -127,7 +137,14 @@ class TestServedArtifactProvenanceTracksGaul: strict=True, ) def test_provenance_includes_admin_digest(self): - prov = json.loads((_DF / "data/assembled/provenance.json").read_text()) + provenance = _DF / "data/assembled/provenance.json" + if not provenance.exists(): + pytest.skip( + "data/assembled/provenance.json is absent — not tracked in " + "views-datafactory, so a checkout alone cannot answer this. Skipping " + "rather than xfailing keeps the strict flip meaningful." + ) + prov = json.loads(provenance.read_text()) sources = prov.get("sources", {}) assert "admin_digest" in sources, ( "provenance.sources has no admin_digest — GAUL parquet changes are " diff --git a/tests/test_delivery_coverage.py b/tests/test_delivery_coverage.py index 716e194..0dca72a 100644 --- a/tests/test_delivery_coverage.py +++ b/tests/test_delivery_coverage.py @@ -92,17 +92,22 @@ def test_no_excluded_cells_is_noop_when_region_unpinned(): assert assert_no_excluded_cells({62356, 94776}, excluded_for("africa_me_legacy")) is None -# Cross-check the frozen manifest against the live producer when its checkout is present -# (CI has no sibling → skip). This is the drift tripwire C-30 asks for. +# Cross-check the frozen manifest against the live producer. This is the drift tripwire +# C-30 asks for, and since 2026-08-17 it RUNS IN CI: `run_pytest.yml` fetches +# views-datafactory, and both pgid lists below are tracked there. It backs a guarantee +# given to FAO in writing, so it must fail as an assertion rather than as a traceback — +# hence the gate names both files the body reads, not just the first. _DATAFACTORY = sibling_repo("views-datafactory") _DF = None if _DATAFACTORY is None else _DATAFACTORY / "src" / "datafactory_query" @pytest.mark.skipif( - _DF is None or not (_DF / "land_pgids.json").exists(), + _DF is None or not all((_DF / f"{n}_pgids.json").exists() for n in ("land", "land_gaul")), reason=( - "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" - "views-datafactory, or place it alongside this repo" + "views-datafactory checkout not found, or it does not carry both " + "src/datafactory_query/{land,land_gaul}_pgids.json — set VIEWS_DATAFACTORY=" + "/path/to/views-datafactory, or place it alongside this repo. Both files are " + "tracked upstream, so a plain checkout is enough (this runs in CI)" ), ) def test_manifest_matches_datafactory_land_minus_land_gaul(): diff --git a/tests/test_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index ee023c4..623c150 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -21,19 +21,32 @@ That is views-datafactory#387 (square-degree area math at high latitudes), and it must not be conflated with what this file guarantees. -Split by dependency, on purpose: - * **always-on** — self-consistency of the committed artifact, plus the coordinate - formula against a committed PRIO-GRID sample. Runs anywhere, including CI. - * **skipif** — full value comparison against the views-datafactory sibling - checkout. Stronger, but absent on most machines (cf. C-46, where a hardcoded - path made a cross-repo gate invisible; here the skip is explicit and the - always-on half still guards regressions). +**Split by the artifact each test actually reads** — four gates, not two, and the +distinction is load-bearing (corrected 2026-08-17): + + * **always-on** — self-consistency of the committed artifact, its wire-cast dtype + stability, and the coordinate formula against a committed PRIO-GRID sample. + Touches no sibling; runs anywhere, including CI. + * **`_needs_region_pgids`** — needs only `src/datafactory_query/*_pgids.json`, which + views-datafactory **tracks**. A plain checkout suffices, so this runs in CI too. + * **`_needs_gaul_parquets`** — needs `data/raw/gaul_admin/*.parquet`, **untracked** + upstream. A checkout is not enough (C-46). + * **`_needs_priogrid_dbf`** — needs the PRIO-GRID shapefile, likewise untracked. + +Until 2026-08-17 all four sibling-aware tests shared one gate keyed to the parquets, +so two tests that read no parquets — and one that reads no sibling at all — sat dark +in CI for no reason. Gating a test on an artifact it does not read is the same mistake +that made a tracked directory stand in for untracked files; both are recorded in C-46. + +**The rule this file now follows: a gate names the artifact its test opens.** Anything +looser has failed here three times — a directory standing in for files, one mark +serving four dependencies, and a path built from a `None` checkout before the +`.exists()` that was supposed to guard it. """ from __future__ import annotations import json -import os from pathlib import Path import numpy as np @@ -61,16 +74,70 @@ _REGION = "land_gaul" _DATAFACTORY = sibling_repo("views-datafactory") -_HAS_DATAFACTORY = _DATAFACTORY is not None and ( - _DATAFACTORY / "data" / "raw" / "gaul_admin" -).is_dir() -_needs_datafactory = pytest.mark.skipif( - not _HAS_DATAFACTORY, +_GAUL_ADMIN = None if _DATAFACTORY is None else _DATAFACTORY / "data" / "raw" / "gaul_admin" + +#: Gate on the FILES this half reads, never on the directory that holds them. +#: +#: `data/raw/gaul_admin/` **is** tracked in views-datafactory — it carries +#: `supplement_azores.geojson` — while the seven GAUL parquets beside it are not. So +#: `.is_dir()` is true in any fresh checkout, the comparison below then runs, and it +#: dies on `FileNotFoundError: .../gaul0_code.parquet` instead of skipping. +#: +#: That is exactly what happened on 2026-08-03, and it is the whole reason +#: views-datafactory was withdrawn from CI (C-46, and the note in +#: `tests/conftest.py::SIBLINGS` that this commit corrects). The cause was read as +#: "the sibling cannot be checked out" when it was "this gate asks the wrong +#: question". The neighbouring PRIO-GRID check at the bottom of this file already +#: had it right, gating on `priogrid_cell.dbf` itself. +#: +#: Named for what it gates rather than for the repository the files live in. The old +#: name, `_HAS_DATAFACTORY`, asserted the same conflation the bug did: a checkout can +#: be present while these are absent, and that is the normal case in CI. +_HAS_GAUL_PARQUETS = _GAUL_ADMIN is not None and all( + (_GAUL_ADMIN / f"{src}.parquet").exists() for src in SOURCE_RENAME +) +_needs_gaul_parquets = pytest.mark.skipif( + not _HAS_GAUL_PARQUETS, reason=( - "views-datafactory checkout not found — set VIEWS_DATAFACTORY=/path/to/" - "views-datafactory, or place it alongside this repo. Only the " - "producer-comparison half is skipped; the always-on tests still guard the " - "committed artifact." + "the producer's GAUL parquets (data/raw/gaul_admin/*.parquet) are not present. " + "They are NOT in views-datafactory's git repository, so a checkout alone is not " + "enough and CI cannot run this comparison — see C-46. On a developer machine, " + "point VIEWS_DATAFACTORY at a checkout that has them." + ), +) + +#: The region's pgid list, which views-datafactory DOES track — so a plain checkout is +#: enough and this runs in CI. Kept separate from the parquet gate on purpose: gating a +#: test on an artifact it does not read is how the whole 2026-08-03 confusion started. +_REGION_PGIDS = ( + None if _DATAFACTORY is None + else _DATAFACTORY / "src" / "datafactory_query" / f"{_REGION}_pgids.json" +) +#: The PRIO-GRID shapefile, also untracked upstream. Declared here rather than built +#: inside the test: `_DATAFACTORY` is None when nothing resolves, and `None / "data"` +#: is a TypeError, not a skip. The test used to be shielded from that by a mark it did +#: not need; removing the mark exposed it, which is the third instance in this file of +#: a gate and its test disagreeing about what must exist. +_PRIOGRID_DBF = ( + None if _DATAFACTORY is None + else _DATAFACTORY / "data" / "raw" / "priogrid" / "shapefile" / "priogrid_cell.dbf" +) +_needs_priogrid_dbf = pytest.mark.skipif( + _PRIOGRID_DBF is None or not _PRIOGRID_DBF.exists(), + reason=( + "the PRIO-GRID shapefile (data/raw/priogrid/shapefile/priogrid_cell.dbf) is not " + "present. Like the GAUL parquets it is not tracked in views-datafactory, so a " + "checkout alone is not enough and CI cannot run this half." + ), +) + +_needs_region_pgids = pytest.mark.skipif( + _REGION_PGIDS is None or not _REGION_PGIDS.exists(), + reason=( + f"views-datafactory checkout not found, or it does not carry " + f"src/datafactory_query/{_REGION}_pgids.json — set VIEWS_DATAFACTORY=/path/to/" + "views-datafactory, or place it alongside this repo. Unlike the GAUL parquets " + "this file IS tracked upstream, so a checkout alone is enough." ), ) @@ -238,7 +305,7 @@ def test_lookup_version_stamp_resolves(lookup): # ── skipif: the full comparison against the producer ───────────────────────── -@_needs_datafactory +@_needs_gaul_parquets def test_lookup_values_match_the_producer_parquets(lookup, gids): """C-43, the core forward-check: every value against views-datafactory. @@ -247,7 +314,7 @@ def test_lookup_values_match_the_producer_parquets(lookup, gids): """ mismatches = {} for src, dst in SOURCE_RENAME.items(): - table = pq.read_table(_DATAFACTORY / "data" / "raw" / "gaul_admin" / f"{src}.parquet") + table = pq.read_table(_GAUL_ADMIN / f"{src}.parquet") source = dict( zip( (int(g) for g in table.column("gid").to_pylist()), @@ -268,10 +335,9 @@ def test_lookup_values_match_the_producer_parquets(lookup, gids): ) -@_needs_datafactory +@_needs_region_pgids def test_lookup_gid_set_equals_the_declared_region(gids): - region_file = _DATAFACTORY / "src" / "datafactory_query" / f"{_REGION}_pgids.json" - region = set(json.loads(region_file.read_text())) + region = set(json.loads(_REGION_PGIDS.read_text())) got = set(int(g) for g in gids) assert got == region, ( f"lookup gid set != {_REGION} region: {len(region - got)} missing, " @@ -279,14 +345,12 @@ def test_lookup_gid_set_equals_the_declared_region(gids): ) -@_needs_datafactory +@_needs_priogrid_dbf def test_coordinate_formula_matches_every_priogrid_cell(): """The committed fixture samples 219 cells; the sibling lets us check all 259,200.""" import struct - dbf = _DATAFACTORY / "data" / "raw" / "priogrid" / "shapefile" / "priogrid_cell.dbf" - if not dbf.exists(): - pytest.skip("PRIO-GRID shapefile not present in the datafactory checkout") + dbf = _PRIOGRID_DBF with dbf.open("rb") as fh: header = fh.read(32) n_records = struct.unpack("float64 wire cast losslessly (sidecar/historical §5.1).""" for col in CODE_COLS: @@ -519,7 +582,7 @@ def test_a_short_digest_is_refused_rather_than_truncated_silently(): builder._lookup_version("land_gaul", {"land_gaul_region": {"content_digest": "abcd"}}) -def test_the_builder_and_the_tests_resolve_the_same_datafactory(): +def test_the_builder_and_the_tests_resolve_the_same_datafactory(monkeypatch): """The one thing worth guarding about the deliberate duplication (S7 / #188). ``scripts/build_gaul_lookup._resolve_datafactory`` and @@ -538,11 +601,17 @@ def test_the_builder_and_the_tests_resolve_the_same_datafactory(): # first — and it is exercised precisely when no checkout exists. So compare the # computed paths unconditionally: gating this on a checkout being present would # skip the one case the test is for, and skip it in CI, where it matters most. - if "VIEWS_DATAFACTORY" not in os.environ: - assert builder._resolve_datafactory() == _REPO.parent / "views-datafactory", ( - "the builder's fallback and the tests' fallback resolve different " - "directories; with no environment override they would disagree silently" - ) + # Assert the fallback UNCONDITIONALLY by removing the override for the duration. + # This was `if "VIEWS_DATAFACTORY" not in os.environ`, which was correct until CI + # started setting that variable (2026-08-17) — at which point the branch stopped + # running in the one place the comment above says it matters most, and the + # assertion below degenerated into comparing $VIEWS_DATAFACTORY with itself. + monkeypatch.delenv("VIEWS_DATAFACTORY", raising=False) + assert builder._resolve_datafactory() == _REPO.parent / "views-datafactory", ( + "the builder's fallback and the tests' fallback resolve different " + "directories; with no environment override they would disagree silently" + ) + monkeypatch.undo() resolved = sibling_repo("views-datafactory") if resolved is None: