Skip to content

geotiff tests: consolidate writer cluster (cog, bigtiff, overview)#2410

Merged
brendancol merged 6 commits into
mainfrom
issue-2400
May 26, 2026
Merged

geotiff tests: consolidate writer cluster (cog, bigtiff, overview)#2410
brendancol merged 6 commits into
mainfrom
issue-2400

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2400. Part of epic #2390 (PR 7 of 11).

Summary

Consolidates 26 writer-side test files into a four-file write/
subpackage:

  • write/test_basic.py — generic writer paths (compression, tiling,
    kwarg order, return path, layout monkeypatch, VRT writer surface).
  • write/test_cog.py — COG writer compliance and invalid-input
    errors.
  • write/test_bigtiff.py — BigTIFF + COG compliance.
  • write/test_overview.py — overview-level and nodata-aware overview
    tests.

Tests-only restructure. No source code changed.

Test ID parity

pytest --collect-only -q reports the same 497 test ids before and
after the move. Full geotiff suite (5706 tests) passes locally on
Python 3.14.

Module-level dedup

The fold caught a few shadowing collisions when several source files
defined identically-named module-level helpers:

  • Identical _build_source_tif, _gpu_available, and
    _HAS_GPU = _gpu_available() lines: deduplicated, keeping the first
    occurrence.
  • Two _float_da helpers with different shape defaults: the (8, 8)
    variant was renamed to _float_da_small so the later (64, 64) one
    no longer shadows it.
  • The private from xrspatial.geotiff._vrt import write_vrt was
    aliased to _priv_write_vrt so it does not shadow the public
    write_vrt used by the signature-parity tests in the same module.

Doc-citation refresh

The release-gate checklist (docs/source/reference/release_gate_geotiff.rst)
and the user-facing GeoTIFF doc (docs/source/reference/geotiff.rst)
referenced several of the deleted filenames. Those rows were re-pointed
at the new write/*.py locations so the rst-parity gate
(test_release_gate_2321.py) keeps passing.

Audit deliverable

xrspatial/geotiff/tests/CLUSTER_AUDIT_PR7.md ships with this PR and
is deleted in a follow-up commit on the same branch before approval
per the epic contract.

Out of scope

  • HTTP-side COG tests (integration cluster, PR 9).
  • Reader-side files (PR 8).
  • VRT non-writer files (PRs 1, 2, 4, 6).

Test plan

  • pytest xrspatial/geotiff/tests/write/ -v
  • pytest xrspatial/geotiff/tests/ -x -q
  • CI (numpy / cupy / dask+numpy / dask+cupy) green

…2400)

Folds 26 writer/COG/BigTIFF/overview test files into a four-file
write/ subpackage per epic #2390 PR 7:

- write/test_basic.py: generic writer paths (compression, tiling,
  kwarg order, return path, layout monkeypatch, VRT writer surface).
- write/test_cog.py: COG writer compliance and invalid-input errors.
- write/test_bigtiff.py: BigTIFF + COG compliance.
- write/test_overview.py: overview-level and nodata-aware overview tests.

Tests-only restructure. No source changes.

The fold preserves 497 test ids (collect-only counts match before and
after). Module-level dedup removed identical-body shadowed helpers
(`_build_source_tif`, `_gpu_available`, `_HAS_GPU`) and renamed the
one private-import `write_vrt` so it does not shadow the public
re-export used by the kwarg-order signature tests in the same file.

The release gate checklist
(docs/source/reference/release_gate_geotiff.rst) and the user-facing
GeoTIFF docs (docs/source/reference/geotiff.rst) had their citations
re-pointed at the new file paths so the rst-parity gate
(test_release_gate_2321.py) keeps passing.

CLUSTER_AUDIT_PR7.md is in this commit and is deleted in a follow-up
commit on the same branch before merge per the epic contract.

Closes #2400
Part of #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 writer cluster (cog, bigtiff, overview)

Blockers

  • pytest.importorskip("rasterio") called at module scope mid-file in test_basic.py:314 and test_cog.py:410, 1396. When the source files were separate modules, each importorskip only skipped its own file. After folding, these calls still run at module-import time, so a single missing rasterio skips every prior test in the same module, including tests that do not need rasterio. Affected sections:

    • test_basic.py:314 skips folded sections from test_writer.py, test_writer_matrix.py, test_writer_kwarg_order_1922.py, and test_writer_return_path_1938.py if rasterio is absent.
    • test_cog.py:410 and :1396 skip earlier sections such as TestCOGWriter (which does not need rasterio) when rasterio is missing.

    Fix: replace each module-top pytest.importorskip("rasterio") with per-test calls inside the test functions that actually need rasterio, or apply a scoped pytestmark only to the affected sections.

Suggestions

  • _helpers/markers.py:gpu_available is not reused. The epic states that markers come from _helpers/markers.py. test_overview.py:26-35 redefines _gpu_available() inline and test_cog.py:15 imports from ..conftest rather than from .._helpers.markers. Pointing all of them at the shared helper would prevent further drift.

  • Redundant duplicate import lines in test_basic.py:31-48 and test_cog.py:28-34. The migration deduplicated identical full-line imports but did not collapse overlapping from xrspatial.geotiff import a, b, c / from xrspatial.geotiff import b, c, d pairs. Six lines all import to_geotiff and four all import write_vrt. The code works, but the noise will hide a future import drift.

  • Comment-only references to deleted filenames still in source. xrspatial/geotiff/_writer.py:140 and xrspatial/geotiff/_attrs.py:339 still cite the deleted test_cog_requires_tiled_2312.py and test_bigtiff_cog_compliance_2286.py. Same for xrspatial/geotiff/tests/test_overview_block_order_2308.py:113 and xrspatial/geotiff/tests/test_release_gate_cog.py:12,16. No gate fails on these, but they read as broken pointers.

Nits

  • # Folded from: <old_filename> banners still embed the issue numbers (e.g. # Folded from: test_writer_kwarg_order_1922.py). The epic says "drop issue numbers from filenames". The audit MD already captures the file map; the in-source banners could shrink to thematic names.

  • test_basic.py:48 aliases write_vrt to _priv_write_vrt but the alias is single-use in one folded section. A brief inline comment next to the import would help a reader who arrives without the PR body.

  • Module docstrings on the four new files list every folded source filename verbatim. That information lives in CLUSTER_AUDIT_PR7.md, which is itself temporary. Once the audit MD is removed before merge, the docstrings will be the only record. Fine either way; just calling out the duplication.

What looks good

  • 497 test IDs collected on both sides of the move (verified with --collect-only -q).
  • Both verification commands from the epic pass locally.
  • The _priv_write_vrt aliasing is the right call: without it the private write_vrt (signature vrt_path, ...) shadowed the public one (signature path, ...) and broke the signature-parity test in the same module.
  • Release-gate rst and geotiff.rst doc citations were updated together with the file move; the rst-parity gate (test_release_gate_2321.py) passes.

Checklist

  • Algorithm matches reference/paper: N/A (tests-only)
  • Backend parity: no source changes
  • NaN handling: no source changes
  • Edge cases preserved via verbatim fold
  • Docstrings present on all four new files

- Blocker: rasterio importorskip moved from module scope to per-test
  in write/test_basic.py and write/test_cog.py compliance section.
  Mid-file pytest.importorskip("rasterio") was skipping ALL prior
  tests in the same module when rasterio was absent. Each rasterio-
  using test now does its own importorskip and binds the module
  locally.
- Suggestion: write/test_overview.py, write/test_basic.py, and
  write/test_cog.py now use ``gpu_available`` from
  ``_helpers/markers.py`` instead of defining a local copy or routing
  through conftest.
- Suggestion: collapsed redundant ``from xrspatial.geotiff import ...``
  duplicates in write/test_basic.py and write/test_cog.py into one
  parenthesised group each. The private write_vrt alias is now next
  to a comment explaining why it exists.
- Suggestion: comment-only filename refs updated in
  xrspatial/geotiff/_writer.py, _attrs.py,
  tests/test_overview_block_order_2308.py, and
  tests/test_release_gate_cog.py to point at the new write/*.py paths.
- Nit: section banners changed from ``# Folded from: test_*_NNNN.py``
  to ``# Section: <thematic name>`` so the in-source markers no longer
  reference removed filenames.
- Nit: module docstrings on the four new files trimmed to describe
  the topical coverage rather than re-list deleted source filenames.

Verification: ``pytest xrspatial/geotiff/tests/write/ -v`` 497 pass;
``pytest xrspatial/geotiff/tests/ -x -q`` 5706 pass / 68 skipped.
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 (follow-up after commit 23b45eb)

Disposition of original findings

Blocker

  • Mid-module pytest.importorskip("rasterio") skip leakage. Fixed. write/test_basic.py:325 and the 8 affected tests in the write/test_cog.py compliance section now do rasterio = pytest.importorskip("rasterio") as the first statement in each test body. The compliance section's module-level binding was removed; the parity section's redundant module-top importorskip (whose tests already had per-test guards) was also removed. write/test_bigtiff.py keeps its module-top importorskip because that file is entirely rasterio-dependent (single source folded).

Suggestions

  • Reuse _helpers/markers.py:gpu_available. Fixed. write/test_overview.py, write/test_basic.py, and write/test_cog.py now import gpu_available from .._helpers.markers instead of redefining it inline or routing through conftest.
  • Collapse redundant import lines. Fixed. write/test_basic.py:31-48 and write/test_cog.py:28-34 are now one parenthesised from xrspatial.geotiff import (...) block each, plus the private write_vrt alias which carries an explanatory comment.
  • Comment-only references to deleted filenames. Fixed. xrspatial/geotiff/_writer.py:140, xrspatial/geotiff/_attrs.py:339, xrspatial/geotiff/tests/test_overview_block_order_2308.py:113, and xrspatial/geotiff/tests/test_release_gate_cog.py:12,16 now point at write/test_cog.py / write/test_bigtiff.py.

Nits

  • Section banner issue numbers. Fixed. # Folded from: test_*_NNNN.py banners renamed to # Section: <thematic name> (e.g. # Section: COG external-interop compliance suite).
  • Comment next to the _priv_write_vrt alias. Fixed inline with the import-collapse: the alias now carries a short comment explaining the shadowing concern.
  • Module docstrings re-list source filenames. Fixed. All four module docstrings now describe the topical coverage rather than re-listing the deleted source files.

Verification

pytest xrspatial/geotiff/tests/write/ -v : 497 passed.
pytest xrspatial/geotiff/tests/ -x -q : 5706 passed, 68 skipped, 6 xfailed, 1 xpassed.

No remaining findings from the first-pass review. CLUSTER_AUDIT_PR7.md is still in place per the epic; it gets deleted in a separate commit on this branch before merge.

Per the epic #2390 contract, the per-PR cluster audit is a temporary
deliverable for the review pass. It is removed in a final commit on
the branch before the PR is approved so it does not land on main.
PR 7 consolidated test_cog_writer_compliance.py and test_cog_parity_2286.py
into write/test_cog.py. The dedicated cog-validator CI workflow still
pointed at the old paths and exited 4 (no tests collected). Point at the
new consolidated module so the strict-validator gate runs again.
# Conflicts:
#	docs/source/reference/release_gate_geotiff.rst
#	xrspatial/geotiff/tests/test_release_gate_cog.py
@brendancol brendancol merged commit 2fd4f90 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 writer / COG / BigTIFF tests (epic #2390 PR 7)

1 participant