Skip to content

Add release-gate tests for stable GeoTIFF features (#2340)#2353

Merged
brendancol merged 3 commits into
mainfrom
issue-2340-pr6
May 24, 2026
Merged

Add release-gate tests for stable GeoTIFF features (#2340)#2353
brendancol merged 3 commits into
mainfrom
issue-2340-pr6

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

PR 6 of 6 in the GeoTIFF release contract epic (#2340). Adds one small, deterministic release-gate test per stable feature so a release engineer can run pytest -m release_gate and know the next release does not silently regress a stable promise.

Summary

  • New release_gate pytest marker registered in setup.cfg. Tests run by default; the marker exists so the release-gate subset can be selected with pytest -m release_gate (~4 seconds, 45 tests).
  • One file per stable feature category from the epic.
  • Each test asserts the contract from the user's point of view: round-trip pixels, attrs['crs'], attrs['transform'], and attrs['nodata'] end to end.

Coverage map

Stable feature (per epic #2340) New release-gate file
Local GeoTIFF read test_release_gate_local_read.py
Local GeoTIFF write test_release_gate_local_write.py
Stable lossless codecs (none, deflate, lzw, packbits, zstd) test_release_gate_codecs.py
COG read + write for stable codecs test_release_gate_cog.py
CRS / transform / nodata attrs round-trip test_release_gate_attrs_contract.py
Windowed reads test_release_gate_windowed_read.py
Dask reads parity vs eager test_release_gate_dask_parity.py

The codec file also pins a cross-check between its local STABLE_LOSSLESS_CODECS constant and the codec.* entries tagged stable in SUPPORTED_FEATURES, so a future tier change cannot silently desync the gate from the runtime tier table.

Out of scope (other PRs in epic #2340)

  • SUPPORTED_FEATURES edits (PR 1)
  • Release-contract docs page (PR 2)
  • Public docstring audit (PR 3)
  • Opt-in enforcement for experimental / internal-only paths (PR 4)
  • Unsupported-combination error messages (PR 5)

Test plan

  • pytest xrspatial/geotiff/tests/test_release_gate_local_read.py -- 4 tests pass
  • pytest xrspatial/geotiff/tests/test_release_gate_local_write.py -- 4 tests pass
  • pytest xrspatial/geotiff/tests/test_release_gate_codecs.py -- 11 tests pass
  • pytest xrspatial/geotiff/tests/test_release_gate_cog.py -- 15 tests pass
  • pytest xrspatial/geotiff/tests/test_release_gate_attrs_contract.py -- 4 tests pass
  • pytest xrspatial/geotiff/tests/test_release_gate_windowed_read.py -- 4 tests pass
  • pytest xrspatial/geotiff/tests/test_release_gate_dask_parity.py -- 3 tests pass
  • pytest -m release_gate xrspatial/geotiff/tests/ -- 45 tests pass, 5412 deselected, ~4 seconds
  • Pre-existing test_release_gate_2321.py still passes

PR 6 of 6 in the GeoTIFF release contract epic. Adds a small,
deterministic release-gate test per stable feature so a release
engineer can run `pytest -m release_gate` and know the next release
does not silently regress a stable promise.

Coverage:
- Local read: pixels, crs, transform, nodata round-trip.
- Local write: pixels, crs, transform, nodata round-trip via the
  public to_geotiff API.
- Stable lossless codecs (none, deflate, lzw, packbits, zstd) on
  uint16 and float32, plus a cross-file parity check against
  SUPPORTED_FEATURES.
- COG write/read for every stable lossless codec.
- Canonical attrs contract: canonical keys present, georef_status='full',
  contract version stamp shape, and a full write-read-write-read cycle.
- Windowed reads: subset returned, crs preserved, transform origin
  shifts to the window, full-extent matches unwindowed.
- Dask read parity: pixels and canonical attrs match eager, lazy
  reads stay dask-backed.

Registers the `release_gate` pytest marker in setup.cfg. Tests run by
default; release engineers can filter with `pytest -m release_gate`.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 24, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Add release-gate tests for stable GeoTIFF features (#2340)

Blockers

None.

Suggestions

  • test_release_gate_dask_parity.py: two of the three tests call open_geotiff(path, chunks=8) without first running pytest.importorskip("dask"). The third does. If dask ever moves out of the test environment, the unprotected two will error at collection while the third skips. Add the import-skip to all three for consistency, or hoist it to module scope.

Nits

  • test_release_gate_codecs.py line 119: from xrspatial.geotiff import SUPPORTED_FEATURES is inside the test body; the other release-gate files import at module top. Easy hoist.
  • _make_data_array is duplicated between test_release_gate_local_write.py and test_release_gate_cog.py. The duplication is on purpose (each gate is self-contained), but worth flagging in case a follow-up wants to share it.

What looks good

  • One file per stable feature, each one small and fast — 45 tests in about 4 seconds.
  • Assertion messages name the contract being checked, so a failure tells the reader which release-notes claim broke.
  • test_release_gate_codec_stable_set_matches_supported_features pins the local stable-codec list against SUPPORTED_FEATURES, so a tier change can't desync the gate from the runtime constant.
  • Temp filenames include _2340.tif, matching the project's parallel-worktree convention.
  • The release_gate marker is registered in setup.cfg and tests run by default; pytest -m release_gate picks just the gates when a release engineer wants the short loop.

Checklist

  • Tests assert documented contract behaviour, not just that the code runs.
  • Stable lossless codec set matches SUPPORTED_FEATURES.
  • Round-trip pixel, CRS, transform, and nodata checks present for every stable feature.
  • Windowed read transform origin shift is verified explicitly.
  • Dask parity test compares pixels and canonical attrs against the eager backend.
  • Temp filenames are unique across files and parametrize cases.
  • release_gate marker registered in setup.cfg.

…2340)

- test_release_gate_dask_parity.py: move pytest.importorskip("dask") to
  module scope so all three tests skip uniformly when dask is absent.
  Previously only the lazy-shape test was protected; the two parity
  tests would error at collection in a dask-less environment.
- test_release_gate_codecs.py: hoist `SUPPORTED_FEATURES` import to the
  top of the module to match the convention used by the other
  release-gate files.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review

Disposition of the original review:

  • Suggestion (dask import-skip): Fixed. pytest.importorskip("dask") is now at module scope in test_release_gate_dask_parity.py, so all three tests skip uniformly when dask is absent.
  • Nit (SUPPORTED_FEATURES import): Fixed. Hoisted to module top in test_release_gate_codecs.py.
  • Nit (_make_data_array duplication): Dismissed. Each release-gate file is intentionally self-contained so a single file can be cited from the release-contract checklist without pulling in shared helpers. A follow-up can consolidate if it wants to.

No new findings on the updated diff. Marker registration, test isolation, and assertion-message specificity are unchanged.

@brendancol
Copy link
Copy Markdown
Contributor Author

PR Review: Add release-gate tests for stable GeoTIFF features (#2340)

Test-only PR (7 new test files plus a marker registration in setup.cfg), so the algorithm / backend dispatch / NaN sections of the template don't apply. Review focuses on what the tests actually pin and whether anything is silently inconsistent across the seven files.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/tests/test_release_gate_cog.py:33 redefines STABLE_LOSSLESS_CODECS = ("none", "deflate", "lzw", "packbits", "zstd") as a second copy of the constant in test_release_gate_codecs.py:34. The cross-check against SUPPORTED_FEATURES runs only against the codecs-file copy, so a future tier change that updates the codecs file but forgets the COG file would leave the COG gate parametrized on a stale list. Importing the constant from test_release_gate_codecs is the simpler fix.

Nits (optional improvements)

  • xrspatial/geotiff/tests/test_release_gate_local_write.py:51-52 hard-codes the pixel-center y/x coords as plain literals. A one-line comment showing the derivation (origin_y - half_pixel - i*pixel_height) would save a maintainer from paging through the writer's coord-to-transform logic to verify them.
  • xrspatial/geotiff/tests/test_release_gate_attrs_contract.py:96-114 checks isinstance(version, int). NumPy scalars (np.int32, np.int64) are not instances of int in CPython. If _attrs.py ever returns a numpy scalar here the test would fail with a confusing message. If the contract is "a Python int" then the current check is correct and worth a one-line comment; if it is "any integer", numbers.Integral is the broader check.
  • The float codec tests in test_release_gate_codecs.py:78-108 and the COG / windowed equivalents use np.testing.assert_array_equal for float pixels. Byte-exact is the right contract for a lossless codec, but the assertion message could say so explicitly so a future maintainer doesn't relax it to allclose.

What looks good

  • Every file has a focused module docstring naming the contract, the "out of scope" list, and the related deeper tests. A maintainer can read one file and know what it does and does not lock.
  • The cross-file parity check at test_release_gate_codecs.py:111-132 catches tier drift at the right place. Adding or removing a codec.* entry in _attrs.py will fail this assertion immediately.
  • Temp file names include the issue number (_2340.tif), matching the project's unique-name convention so the 45-test suite cannot collide on tmpdir.
  • The dask-skip hoist in the second commit is the right call. Collection-time errors in a dask-less env would have masked the parity claim entirely.
  • The pixel patterns are non-constant (row * 100 + col, arange on distinctive grids), so row / col stride confusion still fails the equality check rather than passing on a symmetric input.
  • The release_gate marker description in setup.cfg is enough for a release engineer to use without reading the PR.

Checklist

  • Algorithm matches reference -- N/A (test-only PR)
  • All implemented backends produce consistent results -- dask parity covered; gpu / vrt explicitly out of scope
  • NaN handling is correct -- N/A (lossless byte-exact checks; nodata sentinel covered)
  • Edge cases are covered by tests -- in-bounds windows; out-of-bounds delegated to existing file
  • Dask chunk boundaries handled correctly -- N/A (read-side parity only)
  • No premature materialization or unnecessary copies -- intentional materialization for parity comparison
  • Benchmark exists or is not needed -- not needed (~4 s for 45 tests)
  • README feature matrix updated -- not needed (no new public function)
  • Docstrings present and accurate -- module docstring on every file; per-test docstrings explain the contract

Self-review fix on PR #2353. test_release_gate_cog.py previously
redefined ``STABLE_LOSSLESS_CODECS`` as a local copy of the same
tuple in test_release_gate_codecs.py. The cross-check against
``SUPPORTED_FEATURES`` only runs against the codecs-file copy, so a
future tier change that updated the codecs file but forgot the COG
file would leave the COG gate silently parametrized on a stale list.

Import the tuple from the sibling file so the two files cannot drift.
@brendancol
Copy link
Copy Markdown
Contributor Author

Re-review (post self-fix)

Followed up on the Suggestion in the previous review by importing STABLE_LOSSLESS_CODECS from test_release_gate_codecs in commit 49f46db, so the COG file and the codecs file cannot drift on the stable codec list. Verified locally: 26 codec + COG release-gate tests pass.

The two Nits remain open and are intentionally cosmetic. The isinstance(version, int) check at test_release_gate_attrs_contract.py:108 matches the producer side -- _attrs.py:344 defines _ATTRS_CONTRACT_VERSION = 4 as a Python int, so the current check is exact rather than narrow.

No further actionable findings. Holding for CI.

@brendancol brendancol merged commit dfc500f into main May 24, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant