From bdd4e28e5e601a3b0756970ba06d4389a48bd07b Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Mon, 18 May 2026 13:46:37 -0700 Subject: [PATCH 1/2] geotiff: wire fast/slow split into CI and add a determinism gate (#1930) PR runs use `pytest -m "not slow"` so the six compression fixtures (tagged via `_marks.fast_slow_marks_for`) drop out of the fast lane. push-to-main, a nightly cron at 03:00 UTC, and a `workflow_dispatch` manual trigger run the full corpus with no filter. Adds `test_corpus_determinism.py`: regenerates every manifest fixture into a tmp dir once per session and md5s it against the committed bytes. Also asserts no orphan `.tif` files on disk. The module runs in about 0.3s. --- .github/workflows/test.yml | 19 ++- .../golden_corpus/test_corpus_determinism.py | 155 ++++++++++++++++++ 2 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c419f2e17..78bdc7aef 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,6 +6,15 @@ on: pull_request: branches: - '*' + # Nightly cron and a manual trigger so the full corpus (slow lane + # included) runs at least once a day. PR runs stay on the fast lane + # via `-m "not slow"`; this job has no such filter and exercises + # every fixture in the golden corpus, including the heavier + # compression cells. See issue #1930 for the fast / slow split. + schedule: + # 03:00 UTC daily. Off-peak to avoid contention with weekday PRs. + - cron: '0 3 * * *' + workflow_dispatch: jobs: run: @@ -27,5 +36,13 @@ jobs: run: | python -m pip install --upgrade pip pip install -e .[tests] - - name: Run pytest + - name: Run pytest (fast lane) + # PR triggers run the fast lane: `-m "not slow"` deselects the + # heavier corpus cells tagged via `_marks.fast_slow_marks_for` + # (today: the six compression fixtures). push-to-main and the + # nightly schedule run the full set with no filter. + if: github.event_name == 'pull_request' + run: pytest -m "not slow" + - name: Run pytest (full) + if: github.event_name != 'pull_request' run: pytest diff --git a/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py b/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py new file mode 100644 index 000000000..3f8dff253 --- /dev/null +++ b/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py @@ -0,0 +1,155 @@ +"""Corpus determinism gate (issue #1930). + +The golden corpus generator is built to be byte-deterministic: fixed +seeds, sorted iteration, and an explicit ``os.utime`` pass that pins +file mtimes to a constant epoch. This test guards that property in CI +so a regression in the generator (or a manually-edited fixture on +disk) fails the build instead of silently drifting. + +What this catches: + +* a generator-side change that flips RNG ordering, drops the mtime + normalisation, or otherwise breaks reproducibility -- the + regenerated bytes diverge from the committed bytes; +* a fixture-on-disk drift where the manifest still says X but the + committed ``.tif`` was edited (or stale) so it no longer matches + what the manifest would produce. + +The test is fast (regenerating 30 small TIFFs is a few seconds) and +carries the ``fast`` tag in spirit: it is parameterised over every +manifest entry but each parameter is a single md5 compare, so the +whole module runs comfortably inside the PR fast lane. We do not +attach the ``slow`` pytest marker so ``pytest -m "not slow"`` keeps +it in scope. + +Fixtures the manifest declares but that are not committed on disk +(today: ``example_tiled_uint16_deflate_pred2``, kept as a +schema-illustrating example) are skipped here rather than failing, +mirroring how the per-backend tests handle the same case. +""" +from __future__ import annotations + +import hashlib +import pathlib + +import pytest + +# rasterio / pyyaml are runtime deps of the generator. importorskip +# keeps minimal environments green by skipping the whole module when +# either is missing. +pytest.importorskip("yaml") +pytest.importorskip("rasterio") + +from xrspatial.geotiff.tests.golden_corpus import generate # noqa: E402 + + +FIXTURES_DIR = ( + pathlib.Path(__file__).resolve().parent / "fixtures" +) + + +def _md5(path: pathlib.Path) -> str: + h = hashlib.md5() + with path.open("rb") as f: + for chunk in iter(lambda: f.read(65536), b""): + h.update(chunk) + return h.hexdigest() + + +def _manifest_ids() -> list[str]: + """Return manifest fixture ids in sorted order (the generator's + on-disk order).""" + manifest = generate.load_manifest() + entries = generate.validate(manifest) + return sorted(e["id"] for e in entries) + + +@pytest.fixture(scope="module") +def regenerated_dir(tmp_path_factory: pytest.TempPathFactory) -> pathlib.Path: + """Regenerate the entire corpus into a module-scoped tmp dir. + + Module-scoped so the (few-second) write cost is paid once per + test session rather than per parametrised case. + """ + out = tmp_path_factory.mktemp("regen_corpus_1930") + generate.generate(output_dir=out) + return out + + +@pytest.mark.parametrize("fixture_id", _manifest_ids()) +def test_fixture_bytes_are_deterministic( + fixture_id: str, regenerated_dir: pathlib.Path +) -> None: + """The committed ``.tif`` for each manifest id matches what the + generator would produce today, byte for byte. + + Skip rather than fail when the committed file is missing -- that + means the fixture is declared but intentionally not shipped + (e.g. the schema-illustrating example fixture). The per-backend + tests handle this the same way. + """ + committed = FIXTURES_DIR / f"{fixture_id}.tif" + if not committed.exists(): + pytest.skip( + f"fixture {fixture_id!r} is in the manifest but not committed " + f"on disk; nothing to compare" + ) + regenerated = regenerated_dir / f"{fixture_id}.tif" + assert regenerated.exists(), ( + f"generator did not produce {fixture_id!r}; check the generator " + f"and manifest stayed in sync" + ) + committed_md5 = _md5(committed) + regenerated_md5 = _md5(regenerated) + assert committed_md5 == regenerated_md5, ( + f"fixture {fixture_id!r} drifted: committed md5 {committed_md5} " + f"does not match regenerated md5 {regenerated_md5}. Either the " + f"generator changed and the committed fixtures need re-running " + f"(`python -m xrspatial.geotiff.tests.golden_corpus.generate`), " + f"or the committed fixture was edited out of band." + ) + + +def test_external_overview_sidecar_is_deterministic( + regenerated_dir: pathlib.Path, +) -> None: + """Fixtures with ``external_overview: true`` ship a sidecar + ``.tif.ovr`` next to the main ``.tif``. The sidecar bytes are + part of the determinism contract too. + """ + sidecar_name = "overview_external_ovr_uint16.tif.ovr" + committed = FIXTURES_DIR / sidecar_name + if not committed.exists(): + pytest.skip( + f"sidecar {sidecar_name!r} is not committed; nothing to compare" + ) + regenerated = regenerated_dir / sidecar_name + assert regenerated.exists(), ( + f"generator did not produce {sidecar_name!r}; external_overview " + f"path may be broken" + ) + assert _md5(committed) == _md5(regenerated), ( + f"{sidecar_name!r} drifted from the committed bytes; rerun the " + f"generator and recommit, or revert the on-disk edit" + ) + + +def test_no_orphan_fixtures_on_disk() -> None: + """Every committed ``.tif`` (and ``.tif.ovr`` sidecar) corresponds + to a manifest entry. Catches stale fixtures left behind after a + manifest delete. + """ + manifest_ids = set(_manifest_ids()) + orphans: list[str] = [] + for path in sorted(FIXTURES_DIR.glob("*.tif")): + if path.stem not in manifest_ids: + orphans.append(path.name) + for path in sorted(FIXTURES_DIR.glob("*.tif.ovr")): + # sidecar stem: strip ``.tif`` to recover the fixture id + fid = path.name[: -len(".tif.ovr")] + if fid not in manifest_ids: + orphans.append(path.name) + assert not orphans, ( + f"committed fixtures {orphans!r} have no matching manifest entry; " + f"either re-add them to the manifest or remove the orphan files" + ) From 1f8036195ad53a4648a52ab1aebe560817cef43a Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Mon, 18 May 2026 13:49:56 -0700 Subject: [PATCH 2/2] Address review feedback on determinism gate (#1930) * `_md5()` passes `usedforsecurity=False` so FIPS-strict runners do not raise. Use is byte-identity only, no security claim. * External-overview sidecar test now parametrises over every manifest entry with `external_overview: true` so a future fixture is covered without a code change. Today's single sidecar still runs. * `_manifest_ids` / `_load_entries` cached at import as module-level tuples; parametrize and the orphan-file test share one manifest load + validation per session. * Docstring runtime claim swapped for the measured ~0.3s. * Workflow comment notes that GitHub Actions `schedule` triggers only fire from the default branch, so cron on a feature branch is a no-op. --- .github/workflows/test.yml | 4 ++ .../golden_corpus/test_corpus_determinism.py | 51 ++++++++++++------- 2 files changed, 38 insertions(+), 17 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 78bdc7aef..89030d22e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,6 +11,10 @@ on: # via `-m "not slow"`; this job has no such filter and exercises # every fixture in the golden corpus, including the heavier # compression cells. See issue #1930 for the fast / slow split. + # + # GitHub Actions only fires `schedule` triggers on the workflow file + # in the default branch. The cron will not run from a feature branch + # or PR head -- use `workflow_dispatch` below for an on-demand run. schedule: # 03:00 UTC daily. Off-peak to avoid contention with weekday PRs. - cron: '0 3 * * *' diff --git a/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py b/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py index 3f8dff253..a29f7c216 100644 --- a/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py +++ b/xrspatial/geotiff/tests/golden_corpus/test_corpus_determinism.py @@ -15,12 +15,12 @@ committed ``.tif`` was edited (or stale) so it no longer matches what the manifest would produce. -The test is fast (regenerating 30 small TIFFs is a few seconds) and -carries the ``fast`` tag in spirit: it is parameterised over every -manifest entry but each parameter is a single md5 compare, so the -whole module runs comfortably inside the PR fast lane. We do not -attach the ``slow`` pytest marker so ``pytest -m "not slow"`` keeps -it in scope. +The test is fast (regenerating the 30-fixture corpus measured ~0.3s +locally) and carries the ``fast`` tag in spirit: it is parameterised +over every manifest entry but each parameter is a single md5 +compare, so the whole module runs comfortably inside the PR fast +lane. We do not attach the ``slow`` pytest marker so +``pytest -m "not slow"`` keeps it in scope. Fixtures the manifest declares but that are not committed on disk (today: ``example_tiled_uint16_deflate_pred2``, kept as a @@ -49,19 +49,27 @@ def _md5(path: pathlib.Path) -> str: - h = hashlib.md5() + # ``usedforsecurity=False`` (Python 3.9+) keeps this working on + # FIPS-strict runners where ``hashlib.md5()`` otherwise raises a + # ValueError. Byte-identity comparison only, no security claim. + h = hashlib.md5(usedforsecurity=False) with path.open("rb") as f: for chunk in iter(lambda: f.read(65536), b""): h.update(chunk) return h.hexdigest() -def _manifest_ids() -> list[str]: - """Return manifest fixture ids in sorted order (the generator's - on-disk order).""" - manifest = generate.load_manifest() - entries = generate.validate(manifest) - return sorted(e["id"] for e in entries) +def _load_entries() -> list[dict]: + """Return validated manifest entries (defaults merged), sorted by id.""" + return sorted(generate.validate(generate.load_manifest()), key=lambda e: e["id"]) + + +# Cached at import so parametrize collection and the orphan-file test +# share one manifest load. Each call to ``validate()`` re-walks every +# entry, so collapsing the two callers cuts validation work in half. +_ENTRIES = _load_entries() +_MANIFEST_IDS = [e["id"] for e in _ENTRIES] +_EXTERNAL_OVR_IDS = [e["id"] for e in _ENTRIES if e.get("external_overview")] @pytest.fixture(scope="module") @@ -76,7 +84,7 @@ def regenerated_dir(tmp_path_factory: pytest.TempPathFactory) -> pathlib.Path: return out -@pytest.mark.parametrize("fixture_id", _manifest_ids()) +@pytest.mark.parametrize("fixture_id", _MANIFEST_IDS) def test_fixture_bytes_are_deterministic( fixture_id: str, regenerated_dir: pathlib.Path ) -> None: @@ -110,14 +118,23 @@ def test_fixture_bytes_are_deterministic( ) +@pytest.mark.parametrize("fixture_id", _EXTERNAL_OVR_IDS or ["__none__"]) def test_external_overview_sidecar_is_deterministic( - regenerated_dir: pathlib.Path, + fixture_id: str, regenerated_dir: pathlib.Path ) -> None: """Fixtures with ``external_overview: true`` ship a sidecar ``.tif.ovr`` next to the main ``.tif``. The sidecar bytes are part of the determinism contract too. + + Iterates over every manifest entry with ``external_overview=True`` + so a future fixture lands in this test automatically without a + code change. When no such entry exists today the placeholder id + short-circuits to a skip so pytest still reports a single + informative case. """ - sidecar_name = "overview_external_ovr_uint16.tif.ovr" + if fixture_id == "__none__": + pytest.skip("manifest has no external_overview fixtures today") + sidecar_name = f"{fixture_id}.tif.ovr" committed = FIXTURES_DIR / sidecar_name if not committed.exists(): pytest.skip( @@ -139,7 +156,7 @@ def test_no_orphan_fixtures_on_disk() -> None: to a manifest entry. Catches stale fixtures left behind after a manifest delete. """ - manifest_ids = set(_manifest_ids()) + manifest_ids = set(_MANIFEST_IDS) orphans: list[str] = [] for path in sorted(FIXTURES_DIR.glob("*.tif")): if path.stem not in manifest_ids: