Skip to content

geotiff tests: consolidate backend-parity cluster#2408

Merged
brendancol merged 5 commits into
mainfrom
issue-2398
May 26, 2026
Merged

geotiff tests: consolidate backend-parity cluster#2408
brendancol merged 5 commits into
mainfrom
issue-2398

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2398. Part of epic #2390 (PR 4).

Summary

Fold the backend-parity cluster, the largest by line count in the geotiff test suite, into parity/test_backend_matrix.py and parity/test_pixel_equality.py.

Folded files (deleted in this PR):

  • test_backend_parity_matrix.py (matrix harness, error sub-matrix)
  • test_backend_full_parity_2211.py (golden-corpus full-parity gate)
  • test_backend_pixel_parity_matrix_1813.py (strict pixel-byte parity + cross-entry-point routing)
  • test_backend_kwarg_parity_1561.py (kwarg threading through the dispatcher)
  • test_attrs_finalization_parity_2211.py (canonical attrs parity across eager/dask/GPU/VRT)
  • test_attrs_parity_1548.py (pass-through TIFF tag attrs parity across 4 backends)
  • test_miniswhite_backend_parity_1797.py (MinIsWhite local/dask/HTTP/GPU parity)

Left for later:

  • test_vrt_backend_parity_2321.py, VRT-specific, picked up by PR 6.

Notes

  • Tests-only restructure. No source changes.
  • Markers come from _helpers/markers.py; the local _gpu_available() duplicates were removed.
  • Parametrize ids are descriptive (e.g. numpy-int16-single-band, stripped-uint8-none), no issue numbers in ids.
  • Internal helpers from the appended sections use _fp_ (full parity) and _ap_ (attrs parity) prefixes to avoid name clashes with the base harnesses.
  • docs/source/reference/release_gate_geotiff.rst rows that cited old paths now point at the consolidated files; test_release_gate_2321::test_release_gate_cites_only_existing_test_files verifies this.
  • Temporary CLUSTER_AUDIT_PR4.md maps old to new test ids. Deleted in a final commit before approval.

Verification

pytest xrspatial/geotiff/tests/parity/ -v       # 438 passed, 43 skipped
pytest xrspatial/geotiff/tests/ -x -q           # 5705 passed, 68 skipped, 6 xfailed, 1 xpassed

Backend coverage: numpy, cupy, dask+numpy, dask+cupy, VRT, HTTP/fsspec. GPU and dask+GPU rows skip explicitly with a loud reason when cupy + CUDA are unavailable.

Test plan

  • pytest xrspatial/geotiff/tests/parity/ -v passes
  • pytest xrspatial/geotiff/tests/ -x -q passes
  • test_release_gate_2321::test_release_gate_cites_only_existing_test_files passes
  • git diff --stat shows old cluster files deleted, not left behind

Fold the backend-parity cluster (the largest by lines in the test
suite) into parity/test_backend_matrix.py and parity/test_pixel_equality.py.

Folded files (deleted in this commit):

* test_backend_parity_matrix.py
* test_backend_full_parity_2211.py
* test_backend_pixel_parity_matrix_1813.py
* test_backend_kwarg_parity_1561.py
* test_attrs_finalization_parity_2211.py
* test_attrs_parity_1548.py
* test_miniswhite_backend_parity_1797.py

Updates docs/source/reference/release_gate_geotiff.rst rows that cited
the old paths; verified by test_release_gate_2321.

Adds a temporary CLUSTER_AUDIT_PR4.md mapping old to new test ids;
deleted before merge.

Closes #2398. Refs epic #2390 (PR 4).
@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: PR #2408 (geotiff tests: consolidate backend-parity cluster)

Blockers

None. Tests-only restructure; the full geotiff suite passes (5705 passed, 68 skipped, 6 xfailed, 1 xpassed), pytest xrspatial/geotiff/tests/parity/ -v passes (438 passed, 43 skipped), and test_release_gate_2321::test_release_gate_cites_only_existing_test_files is green.

Suggestions

  • xrspatial/geotiff/tests/parity/test_backend_matrix.py:1057-1066: _FP_GEOREF_KEYS and _FP_NODATA_KEYS are defined but never read after the merge. Only _FP_CANONICAL_METADATA_KEYS is consumed (by _fp_assert_canonical_metadata_attrs). Drop the two unused tuples or wire them into the helper they were meant to drive.
  • xrspatial/geotiff/tests/parity/test_backend_matrix.py:1042-1050: _FP_FIXTURES_DIR is defined only when _HAS_YAML and _HAS_RASTERIO. _fp_fixture_path() at line 1274 references it unconditionally. Caller paths are gated, so this works today, but a missing _FP_FIXTURES_DIR = None fallback leaves a NameError trap for a future refactor. Add the fallback.
  • xrspatial/geotiff/tests/parity/test_backend_matrix.py:1-13 (docstring): says "Covers three layers of parity" but the file is now four sections (high-risk matrix + error sub-matrix, full-corpus parity, canonical-attrs parity, pass-through TIFF tag parity). Update the count or list each section.
  • xrspatial/geotiff/tests/parity/test_backend_matrix.py: the appended Full-Parity section uses _fp_ prefix and the Attrs section uses _ap_ prefix; the base matrix section uses no prefix. Fine for collision avoidance, but _fp_is_nan_sentinel etc. duplicate logic already present in the base section. Either rename the duplicates or replace them with calls into the base helpers.
  • xrspatial/geotiff/tests/CLUSTER_AUDIT_PR4.md: needs deleting in a final commit before approval, per the audit plan. Make sure that lands.
  • xrspatial/geotiff/tests/parity/test_pixel_equality.py module docstring (lines 3-16): the bullet list has four items but the sentence above says "three concerns share the file". Fix the count.

Nits

  • xrspatial/geotiff/tests/parity/test_pixel_equality.py:27: from pathlib import Path is now used only twice. Inline the type hint or leave as-is.
  • xrspatial/geotiff/tests/parity/test_backend_matrix.py:65-66: from pathlib import Path is imported alongside import pathlib. One is redundant. Path is used in type hints; pathlib is used by the appended FP section. Pick one.
  • Both consolidated files redeclare a per-file _skip_no_gpu even though _helpers/markers.py already exports requires_gpu for the same purpose. Switch to the shared marker in a follow-up to remove the duplication (skip-reason wording differs slightly, so the swap needs a deliberate decision).
  • xrspatial/geotiff/tests/parity/test_backend_matrix.py:1140-1158: the _fp_read_* functions all take a _fixture_id parameter they ignore. Use *_ instead of named-and-ignored.

What looks good

  • The base matrix harness and the error sub-matrix moved verbatim, so parametrize ids stay stable for anyone tracking failures across the rename.
  • Markers source-of-truth is now _helpers/markers.py; the per-file _gpu_available() duplicates are gone in both consolidated files.
  • Release-gate doc citations got updated, and test_release_gate_2321::test_release_gate_cites_only_existing_test_files verifies the move at collection time. Future test renames will fail loudly.
  • CLUSTER_AUDIT_PR4.md traces every old test id to a new one and justifies the one dropped row (test_backend_specific_keys_carveout_is_documented).
  • The _fp_/_ap_ namespacing on appended internals avoids collisions with the base section's _materialise, _BACKENDS, _FIXTURES, etc.

Checklist

  • Algorithm matches reference -- tests-only restructure, no algorithm changes.
  • All implemented backends produce consistent results -- the consolidated suite is itself the parity gate; 438 parity tests pass.
  • NaN handling is correct -- _fp_assert_pixels_close uses equal_nan=True; the _is_nan_sentinel helper is unchanged.
  • Edge cases covered by tests -- inherited from the seven source files; nothing dropped beyond the one documented case.
  • Dask chunk boundaries handled correctly -- inherited.
  • No premature materialization or unnecessary copies -- inherited.
  • Benchmark exists or is not needed -- not needed (test reorganisation).
  • README feature matrix updated -- not applicable.
  • Docstrings present and accurate -- mostly, see the "three layers" / "three concerns" nits.

* Fix module docstrings on test_backend_matrix.py and
  test_pixel_equality.py: three -> four sections.
* Drop unused _FP_GEOREF_KEYS and _FP_NODATA_KEYS constants;
  only _FP_CANONICAL_METADATA_KEYS is consumed.
* Add _FP_FIXTURES_DIR = None fallback so the gated path never
  raises NameError if a future refactor drops the guard.
* Replace per-file _skip_no_gpu definition with the shared
  requires_gpu marker from _helpers/markers.py in both files.
* Replace the ignored _fixture_id parameter on _fp_read_* with
  *_ so the unused-arg signal is gone.
* Drop redundant ``from pathlib import Path`` in
  test_backend_matrix.py; alias Path = pathlib.Path to keep
  existing signatures working.
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 (post-fix)

Disposition of original findings

Suggestions:

  • Unused _FP_GEOREF_KEYS / _FP_NODATA_KEYS: fixed. Both tuples removed (commit a3b5137).
  • _FP_FIXTURES_DIR NameError trap: fixed. Added _FP_FIXTURES_DIR = None fallback in the else branch.
  • "three layers" docstring drift on test_backend_matrix.py: fixed. Now lists the four sections.
  • Helper duplication (_fp_is_nan_sentinel etc.): dismissed. After a second pass, the FP/AP helpers use different tolerance and sentinel semantics from the base section's helpers; the base section does not actually define _is_nan_sentinel, so there is nothing to deduplicate. No code change.
  • CLUSTER_AUDIT_PR4.md deletion: fixed. Deleted in commit 8d249d2.
  • "three concerns" docstring on test_pixel_equality.py: fixed.

Nits:

  • Redundant from pathlib import Path alongside import pathlib: fixed. Aliased Path = pathlib.Path after the imports.
  • Path import in test_pixel_equality.py only used twice: dismissed. Used in fixture builder signatures; inlining pathlib.Path everywhere is noisier than keeping the alias.
  • Per-file _skip_no_gpu vs shared requires_gpu: fixed. Both files now _skip_no_gpu = requires_gpu from _helpers/markers.py.
  • _fixture_id unused parameter on _fp_read_*: fixed. Signature is now (path, *_).

Other actions

  • Merged origin/main (a8e73cc) into the branch; no conflicts.
  • pytest xrspatial/geotiff/tests/parity/ -q: 438 passed, 43 skipped.

No new findings.

# Conflicts:
#	docs/source/reference/release_gate_geotiff.rst
@brendancol brendancol merged commit cf34034 into main May 26, 2026
6 of 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 backend-parity tests (epic #2390 PR 4)

1 participant