Skip to content

geotiff tests: consolidate VRT validation cluster#2406

Merged
brendancol merged 4 commits into
mainfrom
issue-2395
May 26, 2026
Merged

geotiff tests: consolidate VRT validation cluster#2406
brendancol merged 4 commits into
mainfrom
issue-2395

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2395. Tracks epic #2390 (PR 2 of 11).

Summary

Folds the validator-side VRT cluster into one parametrised module under xrspatial/geotiff/tests/vrt/test_validation.py. The new file covers four matrices: validator-rule rejections, entry-point parity (read_vrt x _internal_read_vrt x open_geotiff), the narrow-except contract (exception class x default/strict mode), and path containment.

Files removed

  • xrspatial/geotiff/tests/test_vrt_validation_2321.py
  • xrspatial/geotiff/tests/test_vrt_capability_validator_2371.py
  • xrspatial/geotiff/tests/test_vrt_unsupported_2370.py
  • xrspatial/geotiff/tests/test_vrt_narrow_except_1670.py
  • xrspatial/geotiff/tests/test_vrt_path_containment_1671.py

Also removes the stale xrspatial/geotiff/tests/CLUSTER_AUDIT.md that PR 1 (#2394) left behind on main when its final cleanup commit landed after the merge.

find xrspatial/geotiff/tests -name 'test_*.py' | wc -l drops from 356 to 352 (5 deleted, 1 added).

Notes

  • Two cases that were already xfail in test_vrt_unsupported_2370.py stay under pytest.mark.xfail(strict=False) in the consolidated file (mixed source CRS, mixed source dtype widening). The validator does not yet deliver those rejections; the wrappers will start passing without an edit here when it does.
  • docs/source/reference/release_gate_geotiff.rst had two citations pointing at deleted files; both are repointed at vrt/test_validation.py so test_release_gate_cites_only_existing_test_files stays green.
  • xrspatial/geotiff/tests/CLUSTER_AUDIT_PR2.md lands with this commit and will be deleted in a follow-up commit on this branch before merge per the epic contract.
  • Tests-only restructure. No changes to xrspatial/geotiff/ source modules.

Test plan

  • pytest xrspatial/geotiff/tests/vrt/test_validation.py -v -- 81 passed, 2 xfailed
  • pytest xrspatial/geotiff/tests/ -x -q -- 5709 passed, 68 skipped, 6 xfailed, 1 xpassed
  • CI green across numpy / cupy / dask+numpy / dask+cupy

Folds the validator-side VRT files into one parametrised module:

- test_vrt_validation_2321.py
- test_vrt_capability_validator_2371.py
- test_vrt_unsupported_2370.py
- test_vrt_narrow_except_1670.py
- test_vrt_path_containment_1671.py

All five are removed in this commit. The new ``vrt/test_validation.py``
parametrises the validator-rule matrix, the entry-point matrix
(package read_vrt, internal _vrt.read_vrt, open_geotiff), the
narrow-except matrix (exception class x default/strict mode), and the
path-containment matrix.

Also removes the stale ``CLUSTER_AUDIT.md`` from PR 1 that leaked to
main, and updates two release-gate citations in
``docs/source/reference/release_gate_geotiff.rst`` so the
``test_release_gate_cites_only_existing_test_files`` gate still
passes.

Adds a temporary ``CLUSTER_AUDIT_PR2.md`` mapping every old
``file::test`` to its new ``file::test_id``; deleted in a follow-up
commit on this branch before merge per the epic #2390 contract.

Tests-only restructure. No changes to ``xrspatial/geotiff/`` source
modules. ``find xrspatial/geotiff/tests -name 'test_*.py' | wc -l``
drops from 356 to 352 (5 deleted, 1 added).
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 26, 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: geotiff tests: consolidate VRT validation cluster

Blockers

None.

Suggestions

  • xrspatial/geotiff/tests/vrt/test_validation.py:1036-1046 -- _zstd_error_or_skip() is dead code. The helper is never called; the two zstd tests use @pytest.mark.skipif(not _has_zstandard()) and import ZstdError inline. Remove the unused helper.
  • xrspatial/geotiff/tests/vrt/test_validation.py:594-609 -- test_warped_subclass_band_rejected_via_open_geotiff swallows the rejection with a four-way pytest.raises((ValueError, NotImplementedError, RuntimeError, UnsupportedGeoTIFFFeatureError)) and never checks the error message. The old test_warped_vrt_subclass_raises from test_vrt_unsupported_2370.py asserted the message named 'warp' or 'vrtwarped' (via _assert_raises_or_xfail keywords). Add a keyword assertion (e.g. assert any(k in str(excinfo.value).lower() for k in ('warp', 'vrtwarped'))) so a future regression that raises a generic ValueError without naming the failure mode actually fails the test.

Nits

  • xrspatial/geotiff/tests/vrt/test_validation.py:74-77, 1236-1240 -- _uniq and _unique_dir both build {label}_{uuid.uuid4().hex[:8]} names. _unique_dir could call _uniq and then mkdir, rather than re-rolling the uuid format inline.
  • xrspatial/geotiff/tests/vrt/test_validation.py:61-66 -- the alias dance (_package_read_vrt = _public_read_vrt) is one indirection more than needed. Either import once as _package_read_vrt directly, or drop the second name.

What looks good

  • The audit in CLUSTER_AUDIT_PR2.md maps every old file::test to its new home and notes the two xfail(strict=False) carryovers from test_vrt_unsupported_2370.py. Row count matches the old files.
  • Parametrise IDs are descriptive throughout (reject[bad-resample-alg], truthy[true], entry[package-read_vrt]). No issue numbers leak into test names.
  • Both release_gate_geotiff.rst citations are repointed to vrt/test_validation.py so test_release_gate_cites_only_existing_test_files stays green.
  • The stale PR-1 CLUSTER_AUDIT.md is removed in the same commit.
  • File count drops from 356 to 352 (5 deleted, 1 added), matching the PR description.
  • Source-TIFF and VRT filenames carry uuid suffixes so parallel test workers across worktrees do not collide.

Checklist

  • Tests-only restructure; no xrspatial/geotiff/ source modules touched
  • Entry-point matrix preserved (package read_vrt, internal _vrt.read_vrt, open_geotiff)
  • Edge cases (warped / nested / mixed-{crs,dtype,band-count} / mask / resample / path-traversal) preserved
  • No premature materialization
  • Docstrings on every public test and helper

* Drop the dead `_zstd_error_or_skip()` helper; the two zstd tests use
  module-level `skipif(not _has_zstandard())` and import `ZstdError`
  inline.
* Tighten `test_warped_subclass_band_rejected_via_open_geotiff` with a
  message-keyword assertion (`'warp'` or `'vrtwarped'`) so a regression
  that raises a generic `ValueError` without naming the failure mode
  fails the test.
* Collapse `_unique_dir` onto `_uniq` so the uuid format is in one place.
* Drop the alias dance (`_package_read_vrt = _public_read_vrt`); import
  the backend `read_vrt` directly as `_package_read_vrt`.
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 after commit 2175775

All four findings from the prior review have been addressed:

Suggestions

  • _zstd_error_or_skip() removed (commit 2175775, vrt/test_validation.py).
  • test_warped_subclass_band_rejected_via_open_geotiff now asserts the message contains 'warp' or 'vrtwarped' so a generic-ValueError regression fails the test.

Nits

  • _unique_dir now calls _uniq(label) instead of re-rolling the uuid format.
  • The alias dance is gone; the module imports the backend read_vrt as _package_read_vrt directly.

Verification

  • pytest xrspatial/geotiff/tests/vrt/test_validation.py -q: 81 passed, 2 xfailed.
  • No source modules under xrspatial/geotiff/ touched.

Only the temporary CLUSTER_AUDIT_PR2.md is left to remove before merge, per the epic contract.

The audit table served its purpose during review (mapping every old
``file::test`` to its new home in ``vrt/test_validation.py``). Per the
epic, the audit is deleted on the same branch before merge so it does
not land on main.
@brendancol brendancol merged commit 938c9d7 into main May 26, 2026
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.

Consolidate VRT validation tests (epic #2390 PR 2)

1 participant