From 565b0a7b7c473a4e6963560e682889291d5f7ab3 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:10:22 +0200 Subject: [PATCH 1/3] fix(ci): the exclusion-manifest tripwire now runs in the gate (C-30, C-46) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 2026-08-17 this repository told FAO in writing that the 76-cell exclusion list "is frozen in code and asserted against the producer in our test suite, so it cannot drift without failing loudly". It could. The assertion — `test_manifest_matches_datafactory_land_minus_land_gaul` — skipped on every pull request, because views-datafactory was not fetched in CI. The guarantee was running on one laptop. The stated reason it was not fetched did not survive checking. C-46 and `conftest.SIBLINGS` both recorded that a checkout "converts an honest skip into a FileNotFoundError — measured 2026-08-03, tried and reverted". The observation was right; the diagnosis was wrong. `test_gaul_lookup_fidelity` gated on `data/raw/gaul_admin/` being a DIRECTORY, and that directory IS tracked in views-datafactory — it holds `supplement_azores.geojson` — while the seven GAUL parquets beside it are not. So a checkout satisfied the gate, the value comparison ran, and it died. The sibling was never the obstacle; the gate asked whether a folder existed when it needed to ask whether the files it reads existed. Reproduced before changing anything, against a `git worktree --detach` of views-datafactory@HEAD — exactly what `actions/checkout` produces: before 1 failed, 37 passed, 1 skipped FileNotFoundError: .../data/raw/gaul_admin/gaul0_code.parquet after 35 passed, 4 skipped, 0 failed What changed: - `test_gaul_lookup_fidelity._HAS_DATAFACTORY` gates on the seven parquets it actually reads, not on their directory. The PRIO-GRID check at the bottom of the same file already had this shape, gating on `priogrid_cell.dbf` itself. - its skip reason now says the parquets are absent and that they are untracked upstream, instead of "checkout not found" — which was false in precisely the case that broke CI. - `run_pytest.yml` fetches views-datafactory at `ref: main`, and declares `VIEWS_DATAFACTORY`; `conftest.SIBLINGS` flips to `ci_checkout=True` with the corrected reason. `test_ci_sibling_coverage` requires those two to agree, and does. Two checks move from skipped to running in the gate: C-30's exclusion tripwire, and C-46's own `TestReleaseGate::test_land_gaul_commit_is_in_a_release_tag` — the gate that entry says had "never run anywhere but one laptop". The producer-comparison half still skips, honestly, and still needs the parquets published somewhere fetchable. C-30 is NOT re-tiered: what holds it at Tier 1 is the absence of a real global delivery exercising it end to end, and CI wiring does not supply that. Suite: 428 passed, 5 skipped, 39 xfailed under a CI-shaped sibling. The 25 failures are the pre-existing local venv drift registered as C-104, unchanged by this commit. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_pytest.yml | 22 ++++++++++++++++++++ reports/technical_risk_register.md | 8 ++++++++ tests/conftest.py | 23 +++++++++++++++------ tests/test_gaul_lookup_fidelity.py | 32 +++++++++++++++++++++++------- 4 files changed, 72 insertions(+), 13 deletions(-) diff --git a/.github/workflows/run_pytest.yml b/.github/workflows/run_pytest.yml index a782614..95e68b5 100644 --- a/.github/workflows/run_pytest.yml +++ b/.github/workflows/run_pytest.yml @@ -75,6 +75,27 @@ jobs: path: _siblings/views-appwrite fetch-depth: 0 + # Fetched for ONE check: `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 of + # which are 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. + # + # 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 +118,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..d417ef5 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -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. @@ -2150,6 +2154,10 @@ 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: `35 passed, 4 skipped`, no failures. views-datafactory is now fetched in `run_pytest.yml` and declared `ci_checkout=True`. Two checks moved from skipped to running in the gate — this entry's `TestReleaseGate::test_land_gaul_commit_is_in_a_release_tag`, and C-30's exclusion-manifest tripwire. 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_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index ee023c4..d29a7cf 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -61,16 +61,34 @@ _REGION = "land_gaul" _DATAFACTORY = sibling_repo("views-datafactory") -_HAS_DATAFACTORY = _DATAFACTORY is not None and ( - _DATAFACTORY / "data" / "raw" / "gaul_admin" -).is_dir() +_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. +_HAS_DATAFACTORY = _GAUL_ADMIN is not None and all( + (_GAUL_ADMIN / f"{src}.parquet").exists() for src in SOURCE_RENAME +) _needs_datafactory = pytest.mark.skipif( not _HAS_DATAFACTORY, 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 half — see C-46. On a developer machine, point " + "VIEWS_DATAFACTORY at a checkout that has them. Only the producer-comparison " + "half is skipped; the always-on tests still guard the committed artifact, and " + "the exclusion-manifest tripwire in test_delivery_coverage.py reads the tracked " + "pgid lists and does run in CI." ), ) From 6fb20469d2001c0b9daf81c3522f42c006a2c61e Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:14:35 +0200 Subject: [PATCH 2/3] fix(tests): gate each fidelity check on the artifact it actually reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from /review-diff on the previous commit. All four are the same defect that commit fixed, one level up: a gate asking about something other than what the test reads. `test_gaul_lookup_fidelity` had ONE sibling gate, keyed to the GAUL parquets, and applied it to four tests. Only one of them reads a parquet: test_lookup_values_match_the_producer_parquets 7 parquets (untracked) correct test_lookup_gid_set_equals_the_declared_region land_gaul_pgids.json — TRACKED test_coordinate_formula_matches_every_pg_cell priogrid_cell.dbf; self-skips test_coord_dtypes_are_wire_stable only the committed lookup So two checks that need nothing untracked, and one that needs no sibling at all, were dark in CI because they carried a mark for someone else's dependency. The last is the one worth naming: it asserts `abs(v) < 2**53` for every code column — that the §5.1 int64->float64 wire cast is lossless. That is precisely the property ADR-013 §5.1a records and the 2026-08-17 mail to FAO relies on, and it was skipped in the gate while resting on nothing but a committed fixture. Changes: - `_HAS_DATAFACTORY` -> `_HAS_GAUL_PARQUETS`, `_needs_datafactory` -> `_needs_gaul_parquets`. The old name asserted the conflation the bug was made of: a checkout can be present while these are absent, which is the normal case in CI. - new `_needs_region_pgids`, gating on the tracked pgid list, so the region-set check runs in CI. - the coordinate check loses the mark; its own `dbf.exists()` skip was already the right gate and is now the only one. - the dtype check loses the mark entirely; it is always-on. - the parquet comparison uses `_GAUL_ADMIN` instead of respelling the path, so the gate and the reader cannot drift apart — which is how this started. - module docstring rewritten: three gates, not two, with the rule stated. Measured. CI-shaped sibling (tracked files only): fidelity file goes 21 passed / 4 skipped -> 23 passed / 2 skipped, and both remaining skips name the artifact they want. Full suite CI-shaped: 430 passed / 3 skipped, up from 426 / 6 before this branch. With a full local sibling: 39 passed, nothing lost. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 15 ++++++- tests/test_gaul_lookup_fidelity.py | 68 ++++++++++++++++++++---------- 2 files changed, 60 insertions(+), 23 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index d417ef5..e746caa 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -2156,7 +2156,20 @@ Verified 2026-08-02: `grep -rn "/home/" tests/ scripts/ views_postprocessing/ -- **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: `35 passed, 4 skipped`, no failures. views-datafactory is now fetched in `run_pytest.yml` and declared `ci_checkout=True`. Two checks moved from skipped to running in the gate — this entry's `TestReleaseGate::test_land_gaul_commit_is_in_a_release_tag`, and C-30's exclusion-manifest tripwire. The producer-comparison half still skips, honestly, and still needs the parquets published somewhere fetchable. +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. diff --git a/tests/test_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index d29a7cf..1773362 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -21,13 +21,22 @@ 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** — three 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`, which + views-datafactory does **not** track. A checkout is not enough; this is the only + half that cannot run in CI (C-46). + +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. """ from __future__ import annotations @@ -76,19 +85,37 @@ #: "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. -_HAS_DATAFACTORY = _GAUL_ADMIN is not None and all( +#: +#: 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_datafactory = pytest.mark.skipif( - not _HAS_DATAFACTORY, +_needs_gaul_parquets = pytest.mark.skipif( + not _HAS_GAUL_PARQUETS, reason=( "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 half — see C-46. On a developer machine, point " - "VIEWS_DATAFACTORY at a checkout that has them. Only the producer-comparison " - "half is skipped; the always-on tests still guard the committed artifact, and " - "the exclusion-manifest tripwire in test_delivery_coverage.py reads the tracked " - "pgid lists and does run in CI." + "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" +) +_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." ), ) @@ -256,7 +283,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. @@ -265,7 +292,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()), @@ -286,10 +313,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, " @@ -297,7 +323,6 @@ def test_lookup_gid_set_equals_the_declared_region(gids): ) -@_needs_datafactory def test_coordinate_formula_matches_every_priogrid_cell(): """The committed fixture samples 219 cells; the sibling lets us check all 259,200.""" import struct @@ -450,7 +475,6 @@ def test_builder_accepts_a_clean_source(monkeypatch, tmp_path): assert result.column_names == METADATA_COLS + ["priogrid_gid"] -@_needs_datafactory def test_coord_dtypes_are_wire_stable(lookup): """Codes survive the int64->float64 wire cast losslessly (sidecar/historical §5.1).""" for col in CODE_COLS: From 58ac58def91a00bf7545a62600f0bf83e67f2c03 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Mon, 17 Aug 2026 11:22:42 +0200 Subject: [PATCH 3/3] fix(tests): address /code-review findings on the sibling-gating change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all mine, all the same defect the branch set out to fix. 1. HIGH — I broke the no-checkout case. Removing the mark from `test_coordinate_formula_matches_every_priogrid_cell` exposed that its `dbf.exists()` skip sits BELOW `dbf = _DATAFACTORY / ...`, and `_DATAFACTORY` is None when nothing resolves. Measured: `TypeError: unsupported operand type(s) for /: 'NoneType' and 'str'` — a red suite for any contributor without a views-datafactory checkout. CI never saw it, because CI now always has one. Fixed with a declared `_needs_priogrid_dbf` gate, like its neighbours. 2. MEDIUM — two `xfail(strict=True)` deploy gates began EXECUTING in CI once the sibling was fetched, and xfailed on missing files rather than on what they assert. `data/assembled/{grid.npy,provenance.json}` are untracked upstream — and measured today, `data/assembled/` is empty in the maintainer's own checkout too, so these gates have never evaluated their assertions ANYWHERE. A strict xfail that can only fail on FileNotFoundError can never XPASS, so the flip that is the whole mechanism could not fire. They now skip when the inputs are absent, which makes the state visible instead of disguising it as a passing xfail. Registered as C-108; skipping does not make the gate work. 3. LOW/MED — the newly-live C-30 tripwire gated on `land_pgids.json` while reading `land_gaul_pgids.json` too. Now that it runs in CI against views-datafactory@main, a rename upstream would have surfaced as a traceback instead of the drift assertion it exists to produce. Gate names both files. The stale `(CI has no sibling → skip)` comment is corrected — it was the last line in the repo still asserting the old state. 4. LOW — `test_the_builder_and_the_tests_resolve_the_same_datafactory` guards the fallback under `if "VIEWS_DATAFACTORY" not in os.environ`, with a comment saying that branch matters most in CI. Setting the variable in CI silenced exactly that branch and left the surviving assertion comparing $VIEWS_DATAFACTORY with itself. Now asserted unconditionally via `monkeypatch.delenv`. Also corrected two overclaims of my own: the workflow comment said the sibling was fetched "for ONE check" and the register said "two", when four began executing. Measured. CI-shaped sibling: 430 passed / 5 skipped / 37 xfailed (two former xfails are now honest skips). No sibling resolvable: no TypeError, 8 clean skips. Full local sibling: nothing lost. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/run_pytest.yml | 14 +++--- reports/technical_risk_register.md | 24 +++++++++- tests/test_datafactory_deploy_readiness.py | 21 ++++++++- tests/test_delivery_coverage.py | 15 ++++-- tests/test_gaul_lookup_fidelity.py | 55 ++++++++++++++++------ 5 files changed, 100 insertions(+), 29 deletions(-) diff --git a/.github/workflows/run_pytest.yml b/.github/workflows/run_pytest.yml index 95e68b5..09b7f24 100644 --- a/.github/workflows/run_pytest.yml +++ b/.github/workflows/run_pytest.yml @@ -75,12 +75,14 @@ jobs: path: _siblings/views-appwrite fetch-depth: 0 - # Fetched for ONE check: `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 of - # which are 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. + # 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 diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index e746caa..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 | --- @@ -1154,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 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 1773362..623c150 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -21,7 +21,7 @@ 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 the artifact each test actually reads** — three gates, not two, and the +**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 @@ -29,20 +29,24 @@ 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`, which - views-datafactory does **not** track. A checkout is not enough; this is the only - half that cannot run in CI (C-46). + * **`_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 @@ -109,6 +113,24 @@ 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=( @@ -323,13 +345,12 @@ def test_lookup_gid_set_equals_the_declared_region(gids): ) +@_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("