Skip to content

geotiff tests: consolidate VRT metadata/window/dtype cluster#2407

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

geotiff tests: consolidate VRT metadata/window/dtype cluster#2407
brendancol merged 4 commits into
mainfrom
issue-2399

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

  • Folds 28 issue-numbered VRT regression test files under xrspatial/geotiff/tests/ into three concern-oriented files under vrt/: test_metadata.py (12 files), test_window.py (9 files), test_dtype_conversion.py (7 files).
  • Tests-only restructure; no source changes. Per-file helpers and fixtures are prefixed (e.g. _holes_attr_*) so cross-file folds don't collide; test names dropped _NNNN issue suffixes where the originating file already namespaced them.
  • Internal xrspatial.geotiff._vrt.read_vrt (returns a tuple) is aliased per file so the eager and chunked sections coexist with the public read_vrt (returns a DataArray).
  • Release-gate checklist and reference docs repointed at the new paths; test_release_gate_cites_only_existing_test_files is green again.

Verification

  • pytest xrspatial/geotiff/tests/vrt/ -v -> 281 passed, 1 xfailed.
  • pytest xrspatial/geotiff/tests/ -q -> 5706 passed, 68 skipped, 6 xfailed, 1 xpassed.

Audit

A temporary xrspatial/geotiff/tests/CLUSTER_AUDIT_PR6.md maps every old file:test row to its new home. Removed on a final commit before approval.

Files NOT touched

Per the epic split, these stay with parallel PRs:

  • PR 2 (validation): 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.
  • PR 4 (parity): test_vrt_backend_parity_2321.py, test_vrt_backend_coverage_2026_05_11.py, test_vrt_finalization_parity_2162.py.
  • PR 7 (writer): all test_vrt_write*.py, test_write_vrt_*.py, test_vrt_writer_*.py.
  • PR 11 (missing-sources flavour): test_vrt_missing_sources_default_raise_1843.py, test_vrt_chunked_missing_raise_at_build_2265.py, test_vrt_chunked_missing_sources_1799.py.

Closes #2399. Part of epic #2390.

Test plan

  • pytest xrspatial/geotiff/tests/vrt/
  • pytest xrspatial/geotiff/tests/
  • CI green on numpy / cupy / dask+numpy / dask+cupy backends

Folds 28 issue-numbered VRT regression test files under
`xrspatial/geotiff/tests/` into three concern-oriented files under
`vrt/`. Tests-only restructure; no source changes.

- `vrt/test_metadata.py` (12 files folded): attrs, nodata-on-VRT,
  XML escape and size cap, single-parse, tiled metadata, cross-backend
  metadata parity.
- `vrt/test_window.py` (9 files folded): window kwarg validation,
  resample+window inverse parity, DstRect resample cap, scaled rects,
  per-source tile and pixel caps, lazy chunks, shared parsed dataset,
  threaded tiled writer.
- `vrt/test_dtype_conversion.py` (7 files folded): dataType validation,
  dtype-name resolution, int source / float VRT, multiband dtype and
  per-band nodata, resample-alg allow-list, simple-mosaic positive
  coverage.

Per-file helpers and fixtures are prefixed to avoid collisions
(`_holes_attr_*`, `_lazy_chunks_*`, ...). Test names dropped trailing
`_NNNN` suffixes where the originating file already namespaced them.
Internal `xrspatial.geotiff._vrt.read_vrt` is aliased per file (it
returns a tuple, unlike the public `read_vrt`) so eager and chunked
sections coexist.

Release-gate checklist and reference docs repointed at the new paths;
`test_release_gate_cites_only_existing_test_files` is green.

`CLUSTER_AUDIT_PR6.md` lands alongside the consolidation and maps
every old file:test to its new home. It is removed on a final commit
before the PR is approved.

Closes #2399.
Part of epic #2390.
@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 metadata/window/dtype cluster

Blockers (must fix before merge)

None. The consolidation is tests-only and the full suite is green (5706 passed, 68 skipped, 6 xfailed, 1 xpassed).

Suggestions (should fix, not blocking)

  • Duplicate from xrspatial.geotiff import ... lines. vrt/test_metadata.py:38-45, vrt/test_window.py:35-36, and vrt/test_dtype_conversion.py:28-30 each carry several lines that import from xrspatial.geotiff. They differ only in which names follow the import. The fold script produced one line per source file because it deduped on the raw text. Functional but reads like a build artefact. Collapse to one line per file listing the union of names. For vrt/test_metadata.py:38-45: from xrspatial.geotiff import (GeoTIFFFallbackWarning, MixedBandMetadataError, open_geotiff, read_geotiff_dask, read_vrt, to_geotiff, write_vrt).

  • write_vrt is imported twice in vrt/test_metadata.py. Line 41 imports write_vrt from the public surface; line 49 imports parse_vrt, write_vrt from the internal _vrt; line 52 aliases the same name as _write_vrt_internal. Public and internal happen to be the same callable today, but the duplicate import lets one shadow the other depending on textual order. Pick one; if both are needed, alias the internal one explicitly.

Nits (optional improvements)

  • tmp_2159_* / tmp_2321_* filenames inside the renamed helpers (e.g. vrt/test_metadata.py:206). These were the original temp-name conventions per CLAUDE.md (issue number in tmp paths to avoid collisions). Helpers are kept per-section anyway, so the issue-numbered tmp names are not wrong, just stale. Could rename to tmp_metadata_* later.

  • pytest.raises(..., match='Bilinear|1751') at vrt/test_metadata.py:1698. The |1751 arm matches the originating issue number in the error message. The Bilinear arm is the load-bearing one. Drop |1751 so the match clause asserts on the algorithm name only.

  • Stale TODO(#2321) comments in the metadata section at lines 1569, 1599, 1644, 1694, 1721, 1755. Several refer to "sub-PR 2" of #2321 which already landed (#2329); each test body already accepts the new VRTUnsupportedError. Delete or update.

  • Class-based test groups (TestVRTSourceNodataZero, TestVrtTiledMetadataParity, TestVrtTiledRichTagCoverage, TestVrtTiledMetadataDask) coexist with free-function tests in vrt/test_metadata.py. Pytest collects both fine; the rest of the file is free functions. Not a blocker.

What looks good

  • All 281 tests in vrt/ pass (1 xfail preserved). The wider geotiff suite (5706 tests) is green.
  • Per-file helper prefixes avoid the obvious name collisions (_holes_attr_*, _window_validation_*, etc.).
  • The tuple-returning internal read_vrt from xrspatial.geotiff._vrt is correctly aliased per file so eager and chunked sections coexist with the DataArray-returning public read_vrt.
  • The release-gate cross-reference doc update is correct and the presence test is green again.
  • CLUSTER_AUDIT_PR6.md lists the file-to-file mapping; it also calls out which files PR 2 / 4 / 7 / 11 still own.

Checklist

  • Algorithm matches reference/paper -- N/A (tests-only)
  • All implemented backends produce consistent results -- preserved from originals
  • NaN handling is correct -- preserved
  • Edge cases are covered by tests -- preserved
  • Dask chunk boundaries handled correctly -- preserved
  • No premature materialization or unnecessary copies -- N/A
  • Benchmark exists or is not needed -- N/A
  • README feature matrix updated (if applicable) -- N/A
  • Docstrings present and accurate -- consolidated module docstrings are good

…use (#2399)

- Dedupe `from xrspatial.geotiff import ...` lines in all three
  consolidated files; coalesce into one line per file.
- Drop the duplicate `write_vrt` import in `vrt/test_metadata.py`:
  the public surface keeps the bare `write_vrt`, the internal one
  (which skips CRS validation) is reached via `_write_vrt_internal`
  in the xml-escape and dtype-12bit sections.
- Trim TODO(#2321) comments that referenced sub-PR 2 (#2329, now
  landed) and replace them with a one-liner noting that the typed
  `VRTUnsupportedError` is already accepted.
- Drop the `|1751` arm from the `Bilinear` match clause in
  `test_unsupported_resample_alg_raises` so the assertion pins the
  algorithm name rather than the originating issue number.
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 fixes

Pass 2 after pushing c54427f (Address review nits).

Disposition of pass 1 findings

  • Suggestion 1 (duplicate from xrspatial.geotiff lines) -- fixed. All three consolidated files now have a single grouped import per source module.
  • Suggestion 2 (write_vrt double-import in vrt/test_metadata.py) -- fixed. Public write_vrt stays as the bare name; calls in the xml-escape section that need the internal validator-free writer route through _write_vrt_internal. Same fix in the dtype-12bit section in vrt/test_dtype_conversion.py (two call sites). Caught one regression during the fix: the public write_vrt validates CRS, and the xml-escape fixtures pass deliberately bad XML to exercise escaping. Those tests now use _write_vrt_internal.
  • Nit 1 (tmp_2159_* / tmp_2321_* filenames inside helpers) -- deferred. CLAUDE.md prefers unique tmp paths and the issue-numbered names are still unique. Renaming would touch many helper lines for no behavioural change. Can be a follow-up.
  • Nit 2 (|1751 arm in match clause) -- fixed. The Bilinear arm is the load-bearing one.
  • Nit 3 (stale TODO(#2321) comments) -- fixed. Sub-PR 2 (#2329) already landed; each TODO either deleted or replaced with a short note that the typed VRTUnsupportedError is accepted.
  • Nit 4 (class-based vs free-function tests coexist) -- deferred. Both work and pytest collects them fine; unifying would be a larger style pass.

Verification

  • pytest xrspatial/geotiff/tests/vrt/ -q -> 281 passed, 1 xfailed.
  • pytest xrspatial/geotiff/tests/ -q -> 5706 passed, 68 skipped, 6 xfailed, 1 xpassed.

No further blockers or suggestions. CLUSTER_AUDIT_PR6.md is still in tree and gets removed in a final commit before merge.

@brendancol brendancol merged commit ae7d6db 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 metadata / window / dtype tests (epic #2390 PR 6)

1 participant