Skip to content

geotiff tests: consolidate VRT missing-sources cluster + _helpers/ foundation#2394

Merged
brendancol merged 2 commits into
mainfrom
issue-2393
May 26, 2026
Merged

geotiff tests: consolidate VRT missing-sources cluster + _helpers/ foundation#2394
brendancol merged 2 commits into
mainfrom
issue-2393

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2393. PR 1 of the 11-PR epic #2390.

Summary

  • New _helpers/ foundation: tiff_builders.py (was conftest.py::make_minimal_tiff), tiff_surgery.py (was top-level _tiff_surgery.py), markers.py (was the marker block in conftest.py).
  • New vrt/ directory with test_missing_sources.py folding test_vrt_missing_sources_policy_1799.py and test_vrt_missing_sources_policy_2367.py into one parametrised module.
  • conftest.py re-exports the helpers and markers so existing from .conftest import ... imports keep working. The pytest_collection_modifyitems socketserver hack stays in place until PR 11.

Coverage delta

  • Old eager byte-band coverage from _1799: 3 tests preserved as smoke checks alongside the parametrised matrix.
  • Old eager-plus-dask float coverage from _2367: 16 parametrised cases preserved (default raise x 2 readers, explicit raise x 2, warn x 2, invalid value 6 x 2 readers).
  • test_vrt_missing_sources_default_raise_1843.py left in place: covers the internal _vrt.read_vrt entry point and the XRSPATIAL_GEOTIFF_STRICT=1 env-var override, neither of which is in the consolidated module's surface. Noted in CLUSTER_AUDIT.md.
  • release_gate_geotiff.rst updated to cite the new path.

File delta

  • Removed: _tiff_surgery.py, test_vrt_missing_sources_policy_1799.py, test_vrt_missing_sources_policy_2367.py.
  • Added: _helpers/{__init__,tiff_builders,tiff_surgery,markers}.py, vrt/{__init__,test_missing_sources}.py, CLUSTER_AUDIT.md (deleted before merge).
  • find xrspatial/geotiff/tests -name 'test_*.py' | wc -l: 357 -> 356. The spec called for a drop of 2 but consolidation by definition adds one new file under vrt/; the deleted-vs-added count is -2 / +1.

Test plan

  • pytest xrspatial/geotiff/tests/vrt/ -v -- 22 passed.
  • pytest xrspatial/geotiff/tests/ -x -q -- 5706 passed, 68 skipped, 6 xfailed, 1 xpassed.
  • pytest xrspatial/geotiff/tests/test_release_gate_2321.py -v -- all green after updating release_gate_geotiff.rst.
  • CI green across numpy / cupy / dask backends.

Audit

CLUSTER_AUDIT.md maps every old file::test to its new file::test_id. Deleted on a follow-up commit on this branch before merge per the epic contract.

…undation (#2393)

PR 1 of the 11-PR epic #2390.

Foundation:
- New _helpers/ with tiff_builders.py (was conftest.py:make_minimal_tiff),
  tiff_surgery.py (relocated from _tiff_surgery.py), markers.py
  (was conftest.py: requires_gpu / requires_loopback / requires_integration
  plus the gpu_available / loopback_available probes).
- conftest.py re-exports the helpers and markers so existing
  `from .conftest import ...` imports keep working. The
  pytest_collection_modifyitems socketserver hack stays in place until
  PR 11.

VRT missing-sources consolidation:
- New vrt/ directory with test_missing_sources.py.
- Folds test_vrt_missing_sources_policy_1799.py (eager-only byte band
  smoke checks) and test_vrt_missing_sources_policy_2367.py
  (eager-plus-chunked float matrix) into one file. Parametrise over the
  eager and dask readers with id="eager" / id="dask"; bad-value matrix
  parametrised over readers and 6 invalid values. No issue numbers in
  test ids.
- test_vrt_missing_sources_default_raise_1843.py stays in place: it
  covers the internal _vrt.read_vrt entry point and the
  XRSPATIAL_GEOTIFF_STRICT env-var override, which is out of the
  consolidated module's surface.

Updated direct importers of _tiff_surgery:
- test_local_tile_byte_cap_1664.py
- test_gpu_tile_byte_cap_2026_05_18.py

Updated release_gate_geotiff.rst to cite the new
vrt/test_missing_sources.py path.

CLUSTER_AUDIT.md maps every old file::test to its new file::test_id;
deleted in a follow-up commit on this branch before merge.

Verification:
- pytest xrspatial/geotiff/tests/vrt/ -v: 22 passed
- pytest xrspatial/geotiff/tests/ -x -q: 5706 passed, 68 skipped,
  6 xfailed, 1 xpassed
@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 for #2394.

The change is a clean tests-only restructure. The new _helpers/ layout is sensible, the conftest re-exports keep every existing import working, and the parametrised VRT module preserves the old eager-byte and eager-plus-dask float coverage that lived across two files. 22 new tests collected and pass; full geotiff suite 5706 passed after the release-gate doc fix.

A few items below worth a follow-up commit before merge.

Blockers

None.

Suggestions

  1. xrspatial/geotiff/tests/CLUSTER_AUDIT.md:75 claims "the net test_*.py count drop at exactly 2", but the actual drop is 357 -> 356. The PR body has the correct -2/+1 accounting; the audit file should match. Either fix the wording or drop the verification bullet -- the file is going away before merge anyway, but until then it should not state a false invariant.

  2. xrspatial/geotiff/tests/vrt/test_missing_sources.py:79-115: _write_partial_float_vrt writes the VRT file via plain open(..., 'w') and uses no indentation in the XML, while _write_byte_missing_vrt (a few lines up) uses Path.write_text with two-space indentation. Both are valid VRT; the inconsistency is purely cosmetic from copy-pasting two different sources. Worth aligning so the file does not look like two helpers written by different people.

  3. xrspatial/geotiff/tests/conftest.py:18-22: the import block pulls _HAS_LOOPBACK (a leading-underscore private name) out of _helpers/markers.py just so pytest_collection_modifyitems can branch on it. Cleaner would be to call loopback_available() once at the top of pytest_collection_modifyitems and stash the result, or export the resolved boolean under a non-underscore name (e.g. HAS_LOOPBACK). The current pattern works but reaches into a private symbol from a sibling module.

Nits

  1. xrspatial/geotiff/tests/_helpers/tiff_builders.py:11-12: docstring is good; just a style note that the blank line between the docstring and the from __future__ import could match the conftest pattern. Not worth a churn.

  2. xrspatial/geotiff/tests/vrt/test_missing_sources.py:198-205: the comment block above test_eager_byte_invalid_policy mentions "the old _1799 file" by issue number. A passing reference in a code comment is grey area; the epic asks for no issue numbers in test names and filenames. Leave or scrub.

  3. xrspatial/geotiff/tests/vrt/test_missing_sources.py:90-91: os.path.join(str(tmp_path), ...) can be str(tmp_path / ...) for consistency with _write_byte_missing_vrt. Same outcome.

What looks good

  • Conftest re-exports preserve every existing from .conftest import make_minimal_tiff / requires_gpu import without touching the call sites.
  • Marker names are unchanged (requires_gpu, requires_loopback, requires_integration).
  • Git rename detection caught _2367.py -> vrt/test_missing_sources.py and _tiff_surgery.py -> _helpers/tiff_surgery.py, so blame keeps working.
  • release_gate_geotiff.rst was updated in the same commit, caught by the existing test_release_gate_2321.py gate.
  • The decision to leave test_vrt_missing_sources_default_raise_1843.py in place is correct: it covers xrspatial.geotiff._vrt.read_vrt (internal) plus XRSPATIAL_GEOTIFF_STRICT=1, neither of which is in the public-API surface this module owns.

Checklist

  • Conftest re-exports preserve legacy import paths.
  • Marker names unchanged.
  • Parametrised IDs are descriptive (eager / dask, bad values quoted).
  • No issue numbers in test function names or IDs.
  • Release-gate doc updated.
  • CLUSTER_AUDIT.md maps every old file::test to its new file::test_id.
  • Old files deleted in the same commit as the new ones.

- CLUSTER_AUDIT.md verification block: correct the "net test_*.py drop
  of 2" claim to reflect the actual 357 -> 356 transition. Consolidation
  by definition adds one new file; net is -1.
- vrt/test_missing_sources.py: align _write_partial_float_vrt to use
  Path.write_text with two-space XML indentation, matching
  _write_byte_missing_vrt. Drop now-unused `import os`. Replace
  `os.path.join(str(tmp_path), ...)` with `str(tmp_path / ...)`.
- conftest.py: drop the private _HAS_LOOPBACK import from
  _helpers/markers. pytest_collection_modifyitems now calls
  loopback_available() directly at entry.
- vrt/test_missing_sources.py: scrub stray "_1799" issue-number
  references from test docstrings and helper comments. The file-level
  docstring still names the two deleted source files for provenance,
  which is intentional.
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 fbecbf8.

Disposition of original findings

  • Suggestion 1 (CLUSTER_AUDIT.md net-count wording): fixed. Verification bullet now reports the actual 357 -> 356 transition and explains why consolidation lands one net deletion per cluster.
  • Suggestion 2 (XML indentation mismatch): fixed. _write_partial_float_vrt now uses Path.write_text with two-space indentation; the unused import os was dropped; the two os.path.join(str(tmp_path), ...) calls became str(tmp_path / ...). Both VRT helpers now read the same way.
  • Suggestion 3 (private _HAS_LOOPBACK import in conftest): fixed. pytest_collection_modifyitems now calls loopback_available() directly. No conftest module reaches into private names in _helpers/markers.py any more.
  • Nit 4 (style note on tiff_builders.py blank lines): dismissed. The original note said "not worth a churn"; leaving it alone keeps the diff focused.
  • Nit 5 (issue-number references in test docstrings): fixed. Scrubbed _1799 from three test docstrings and one comment. The file-level header docstring still names the two deleted source files for provenance, which is intentional audit-trail material.
  • Nit 6 (os.path.join vs tmp_path /): fixed, folded into the Suggestion 2 commit.

Verification

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

No remaining open findings.

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 missing-sources tests and add _helpers/ foundation (epic #2390 PR 1)

1 participant