Skip to content

geotiff tests: consolidate unit cluster + slim conftest, closes #2390#2421

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

geotiff tests: consolidate unit cluster + slim conftest, closes #2390#2421
brendancol merged 2 commits into
mainfrom
issue-2414

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2414
Closes #2390

Final PR of the GeoTIFF test consolidation epic. Three deliverables.

Unit-level consolidation

New xrspatial/geotiff/tests/unit/ directory holds the pure-unit
helper tests:

  • unit/test_header.py — relocated from test_header.py
  • unit/test_geotags.py — relocated from test_geotags.py
  • unit/test_safe_xml.py — relocated from test_gdal_metadata_xml_escape_1614.py
  • unit/test_compression.py — merged from test_packbits_jit_2048.py and
    test_packbits_jit_2049.py

Files that bundle a unit slice with end-to-end IO (test_lz4.py,
test_lerc.py, test_jpeg.py, test_jpeg2000.py,
test_mixed_bps.py, test_mixed_sample_format.py,
test_compression_level.py, test_compression_docstring_1644.py,
test_extra_tags_safe_filter_1657.py, test_lerc_max_z_error.py)
stay where they are. Splitting them is a future-epic problem.

The release-gate row for writer.gdal_metadata_xml in
docs/source/reference/release_gate_geotiff.rst was updated to cite
the relocated unit/test_safe_xml.py.

conftest slim + explicit @requires_loopback

Dropped the pytest_collection_modifyitems socketserver hook in
xrspatial/geotiff/tests/conftest.py. Every HTTP/loopback test
across the suite now carries @requires_loopback explicitly. Markers
added on:

  • parity/test_backend_matrix.py — the matrix and error-matrix tests
  • parity/test_pixel_equality.py — the two miniswhite HTTP tests
  • test_golden_corpus_http_1930.py — module-level pytestmark
  • test_read_geotiff_gpu_url_eager_2161.py — three HTTP cases
  • test_remote_sidecar_byte_order_2314.py — four HTTP cases
  • test_remote_sidecar_chunked_2239.py — four HTTP cases
  • test_sidecar_max_cloud_bytes_2121.py — three HTTP cases
  • test_sidecar_ovr_2112.py — four HTTP cases
  • test_parallel_strip_decode_2100.py — the HTTP class plus one outside
  • test_parallel_strip_decode_sparse_2100.py — the HTTP class
  • write/test_cog.py — rows 5 and 6

The conftest still re-exports make_minimal_tiff and the marker
names so legacy from .conftest import ... keep resolving.

Cleanup

Deleted three stale audit files (CLUSTER_AUDIT_PR5.md,
CLUSTER_AUDIT_PR8.md, CLUSTER_AUDIT_PR9.md) that leaked into
main from prior PRs.

Verification

  • pytest xrspatial/geotiff/tests/unit/ — 135 passed
  • pytest xrspatial/geotiff/tests/ — 5719 passed, 68 skipped, 6 xfailed
  • grep -n "pytest_collection_modifyitems\|socketserver.TCPServer\|_serve(" conftest.py — empty
  • Every file that uses socketserver or _serve( also carries
    @requires_loopback.
  • Loopback-denied simulation (force _HAS_LOOPBACK=False) skips the
    affected tests cleanly with no errors.

Epic-wide file-count delta

PR Cluster Top-level removed Files added
#2394 VRT missing-sources 2 1 (vrt/test_missing_sources.py)
#2404 VRT validation 3 1 (vrt/test_validation.py)
#2405 Attrs contract 4 1 (attrs/test_contract.py)
#2406 Rotated / dropped CRS 3 1 (read/test_crs.py)
#2407 VRT metadata / window / dtype ~120 3 (vrt/test_{metadata,window,dtype_conversion}.py)
#2408 Backend parity ~25 2 (parity/test_{backend_matrix,pixel_equality}.py)
#2409 Release-gate registry n/a 1 (release_gates/test_stable_features.py)
#2410 Writer / COG / BigTIFF / overview ~20 4 (write/test_*.py)
#2411 Integration cluster ~20 3 (integration/test_*.py)
#2412 Reader paths ~30 9 (read/test_*.py)
this Unit cluster + conftest slim 5 4 (unit/test_*.py)

Total cluster reduction: from 354 files at epic start to 253 today
(excluding golden_corpus/).

Test plan

  • pytest xrspatial/geotiff/tests/unit/ passes
  • pytest xrspatial/geotiff/tests/ passes under loopback-available
  • Suite passes under simulated loopback-denied (markers skip cleanly)
  • No file in the suite uses socketserver or _serve( without
    also carrying @requires_loopback
  • Conftest no longer references pytest_collection_modifyitems
  • Stale audit files removed
  • CLUSTER_AUDIT_PR11.md is deleted in a follow-up commit on
    this branch before merge

PR 11 of the GeoTIFF test consolidation epic (#2390).

* Create xrspatial/geotiff/tests/unit/ for pure-unit helper coverage:
  - unit/test_header.py: relocated from test_header.py
  - unit/test_geotags.py: relocated from test_geotags.py
  - unit/test_safe_xml.py: relocated from test_gdal_metadata_xml_escape_1614.py
  - unit/test_compression.py: merged from test_packbits_jit_2048.py
    and test_packbits_jit_2049.py
* Drop the pytest_collection_modifyitems socketserver hook in
  conftest.py. Every HTTP/loopback test now carries @requires_loopback
  explicitly (added to the parity matrix, sidecar, parallel-decode,
  remote-byte-order, and write/test_cog HTTP cases).
* Keep the helper / marker re-exports under __all__ so legacy
  conftest imports continue to resolve.
* Delete stale audit notes CLUSTER_AUDIT_PR5/8/9.md that leaked from
  earlier PRs.
* Update the release-gate rst row for writer.gdal_metadata_xml to
  cite the relocated unit/test_safe_xml.py.

CLUSTER_AUDIT_PR11.md ships in this commit and is removed in a
follow-up commit before merge.
@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 unit cluster + slim conftest

PR 11 of epic #2390. The diff lands cleanly: pure-unit files relocate under unit/, the socketserver hook in conftest.py is replaced by explicit @requires_loopback markers on every HTTP test, and three stale audit files are removed. Verification I reproduced locally:

  • pytest xrspatial/geotiff/tests/unit/ — 135 passed
  • pytest xrspatial/geotiff/tests/ — 5719 passed, 68 skipped, 6 xfailed
  • grep -n "pytest_collection_modifyitems\|socketserver.TCPServer\|_serve(" conftest.py — empty
  • grep -rL "requires_loopback" $(grep -rl "socketserver\|_serve(" xrspatial/geotiff/tests/) — empty

Blockers

None.

Suggestions

None.

Nits

  • xrspatial/geotiff/tests/CLUSTER_AUDIT_PR11.md is the audit deliverable and is expected to disappear in a follow-up commit on this branch before merge (per the epic contract). Prior PRs missed this step; the final commit on this branch should drop the file. The PR body already promises this; flagging here so it does not slip again.
  • xrspatial/geotiff/tests/unit/test_safe_xml.py is a renamed file with zero diff (the rename is the entire change). The line count in git show --stat shows 0 additions, 0 deletions. That is expected — surfacing it so reviewers reading the file list do not look for missing content.

What looks good

  • The unit/ scope is correctly conservative. Mixed files (test_lz4.py, test_lerc.py, test_jpeg.py, test_jpeg2000.py, test_compression_level.py, etc.) stay where they are; the audit table records why.
  • The @requires_loopback placement matches the loopback usage. Tests that only bind via a fixture (parity/test_backend_matrix.py::test_backend_parity_matrix*) get the marker; tests that ignore the HTTP fixture in the same module are left alone.
  • The conftest still re-exports make_minimal_tiff and the marker names under __all__, so existing from .conftest import ... imports across the suite keep resolving without a churn pass.
  • The release-gate rst row for writer.gdal_metadata_xml was updated to cite the new path, so the existing test_release_gate_cites_only_existing_test_files check stays green.
  • The two import io unused imports and one E402-shaped import in test_read_geotiff_gpu_url_eager_2161.py introduced by the marker addition have been cleaned up; the only remaining E402 warnings on touched files are pre-existing in parity/test_backend_matrix.py and were not introduced by this PR.

Checklist

  • Algorithm correctness: N/A (tests-only refactor)
  • Backend parity: N/A
  • NaN handling: N/A
  • Edge cases: marker placement audited test-by-test
  • Dask correctness: N/A
  • No premature materialization or copies: N/A
  • Benchmark: N/A
  • README feature matrix: N/A
  • Docstrings: existing docstrings preserved on move

The audit served its purpose during review. Per the epic #2390
contract, audit files are removed on a final commit on the same
branch before merge so they do not leak into main.
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

Fix disposition of the previous round:

  • Nit 1 — fixed. CLUSTER_AUDIT_PR11.md has been deleted in commit c879ce47 (the final commit on this branch). The audit no longer leaks into main on merge.
  • Nit 2 — dismissed (informational only). The zero-diff renamed unit/test_safe_xml.py is intentional; no action needed.

No new findings. PR is clean to merge.

@brendancol brendancol merged commit 2227a64 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 unit-level tests, slim conftest, close epic #2390 (PR 11) Epic: Consolidate the GeoTIFF test suite (354 files → ~50)

1 participant