Skip to content

geotiff tests: consolidate closing cluster 16 — features / golden-corpus / tail (#2440)#2480

Merged
brendancol merged 5 commits into
mainfrom
worktree-agent-aa1de8132de1e7f5b
May 27, 2026
Merged

geotiff tests: consolidate closing cluster 16 — features / golden-corpus / tail (#2440)#2480
brendancol merged 5 commits into
mainfrom
worktree-agent-aa1de8132de1e7f5b

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closing cluster of long-tail epic #2424. Folds 26 top-level test files into their natural homes and lands the final audit pass.

Sub-PR A — features / tiers (4) → release_gates/test_features.py

  • test_features.py (relocated, kept as the spine of the merged file)
  • test_supported_features_shape_2348.py (section header)
  • test_supported_features_tiers_2137.py (section header)
  • test_unsupported_features_2349.py (section header)
  • test_vrt_stable_only_2443.py (release-contract gate, folded too)

The source-parsing dedup test's parents[1] lookup rewired to parents[2] for the deeper location.

Sub-PR B — golden-corpus relocation (13) → golden_corpus/

Moved with rename (drop _1930 suffix). The relative ._helpers import in test_http.py rewired to .._helpers after the move.

Sub-PR C — uncategorised tail (11) → individual homes

Per the issue's recommended placements. test_remaining_fail_closed_1987.py lands in unit/test_metadata.py next to the existing _1987 framework tests.

Out-of-scope cleanup

Five numbered files left at the top level by earlier clusters folded into matching homes (test_inconsistent_geokeys_2417, test_overview_kernels_2413, test_invalid_int_nodata_rejection_2441, test_compound_crs_reject_2418, test_vrt_stable_only_2443). The three remaining numbered top-level files renamed to drop the suffix. The two near-duplicate round-trip modules consolidated into one test_round_trip.py.

Citation updates

Updated release-gate paths in docs/source/reference/release_gate_geotiff.rst, docs/source/reference/geotiff.rst, golden_corpus_baselines.rst, the __init__.py docstring, and a stray code comment in _header.py. The release-gate citation meta-gate (test_release_gate_cites_only_existing_test_files) now passes.

Definition-of-done audit (epic #2424)

  • find xrspatial/geotiff/tests -name 'test_*.py' | wc -l72 (target 60–80)
  • find xrspatial/geotiff/tests -maxdepth 1 -name 'test_*.py' | wc -l5 (target ≤5)
  • Filenames matching test_*_[0-9]{4,}.py0
  • pytest xrspatial/geotiff/tests/ -q → green (5936 passed, 81 skipped, 2 xfailed)

Test plan

  • pytest xrspatial/geotiff/tests/ -q (CPU lane, golden corpus included) — green locally
  • release-gate citation meta-gate green
  • CI green across every required workflow

Tests-only. No xrspatial/geotiff/ source changes apart from the citation tweak in __init__.py / _header.py comments.

Closes #2425
Closes #2434
Closes #2438
Closes #2440
Closes #2424

…pus / tail (#2440)

Final cluster of long-tail epic #2424. Folds 26 top-level test files
into their natural homes and lands the final audit pass.

Sub-PR A — features / tiers (4 files) → release_gates/test_features.py:
- test_features.py (relocated + retained as the spine of the merged file)
- test_supported_features_shape_2348.py (section header)
- test_supported_features_tiers_2137.py (section header)
- test_unsupported_features_2349.py (section header)
- test_vrt_stable_only_2443.py (release-contract gate, folded too)

Sub-PR B — golden-corpus relocation (13 files) → golden_corpus/:
Moved with rename (drop _1930 suffix). Rewires the relative
._helpers import in test_http.py to ..\_helpers after the move.

Sub-PR C — uncategorised tail (11 files) → individual homes per the
issue's placements. test_remaining_fail_closed_1987.py lands in
unit/test_metadata.py alongside the existing _1987 framework tests.

Out-of-scope cleanup: five numbered files left at the top level by
prior clusters folded in (test_inconsistent_geokeys_2417,
test_overview_kernels_2413, test_invalid_int_nodata_rejection_2441,
test_compound_crs_reject_2418), plus the three remaining numbered
top-level files renamed to drop the suffix (test_fuzz_hypothesis_1661,
test_namespace_no_leak_1708, test_polish_1488). The two near-duplicate
round-trip modules consolidated into one test_round_trip.py.

Release-gate citations in docs/source/reference/release_gate_geotiff.rst,
docs/source/reference/geotiff.rst, golden_corpus_baselines.rst, the
package docstring, and a stray code comment in _header.py updated to
the new paths. The release-gate citation meta-gate now passes.

Definition-of-done audit (epic #2424):
- find xrspatial/geotiff/tests -name 'test_*.py' | wc -l → 72 (target 60–80)
- find xrspatial/geotiff/tests -maxdepth 1 -name 'test_*.py' | wc -l → 5 (target ≤5)
- filenames matching test_*_[0-9]{4,}.py → 0
- pytest xrspatial/geotiff/tests/ -q → green

Tests-only.

Closes #2425
Closes #2434
Closes #2438
Closes #2440
Closes #2424
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 27, 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 closing cluster 16 (#2440)

Tests-only consolidation, 56 files changed, +5960 / -6162. Per-section breakdown matches the issue spec for cluster 16 of epic #2424. File-count audit passes (72 total, 5 top-level, 0 numeric suffix). Local pytest run is green.

Blockers (must fix before merge)

  • test_round_trip.py:85 and test_round_trip.py:971 both define _assert_fixed_point. The second definition silently overrides the first when the module loads. The original helper used np.testing.assert_array_equal with explicit NaN-mask handling and pytest.approx(b, abs=1e-15, rel=1e-12) for transform tolerance. The second uses _compare_pixels(_read_array(...)) and math.isclose(a, b, abs_tol=1e-9, rel_tol=1e-9). The invariant tests at lines 149-682 now run against the looser tolerance, so the byte-equivalence pin they claim has been silently weakened. Rename one of the two helpers (e.g. _assert_property_fixed_point for the Hypothesis section) and update the call sites.
  • test_round_trip.py:82 and test_round_trip.py:967 both define _LOCKED_ATTRS. Same silent override pattern. The second adds _NO_GEOREF_KEY to the tuple, which changes what the invariant tests check. Likely harmless because the helper gates on if key in da1.attrs, but the rename suggested above should cover this one too.

Suggestions (should fix, not blocking)

  • read/test_georef.py:1979 cross-references test_remaining_fail_closed_1987.test_read_rejects_rotated_vrt, but the source file was folded into unit/test_metadata.py. Update the citation so future readers can find it.

Nits (optional)

  • release_gates/test_features.py is mixed scope after the merge: about 2850 lines of general feature regression coverage plus three sections of release-contract gates. Only the _vrt_stable_only_2443 section carries the @pytest.mark.release_gate marker. Worth a one-line note in the module docstring that the release-gate markers are localized to the contract sections, so a release engineer running pytest -m release_gate sees the right subset.
  • test_round_trip.py:689-694 re-imports math, os, numpy, pytest, xarray after the merge. Functional but redundant. If you're renaming the helpers anyway, collapse these too.

What looks good

  • Audit numbers hit the targets exactly: 72 total test files (60-80 target), 5 top-level (<=5 target), 0 files with 4+ digit suffix.
  • Release-gate citation meta-gate (test_release_gate_cites_only_existing_test_files) passes; every .rst and code-comment citation was updated.
  • Section banners (# Source: <oldname>.py) preserve the audit trail from the per-issue PRs.
  • Relative-import rewires caught and applied: ._helpers to .._helpers in golden_corpus/test_http.py, .conftest to ..conftest across the relocated TestBigEndian section.
  • parents[1] to parents[2] rewire on the source-parsing dedup test fits the new directory depth.

Checklist

  • All cited test files exist on disk
  • All four backends still discover their tests (numpy / dask / cupy / dask+cupy paths collect)
  • No filename matches test_*_[0-9]{4,}.py
  • Top-level count <= 5
  • Total count in 60-80
  • pytest xrspatial/geotiff/tests/ -q green locally
  • Helper-name collisions in test_round_trip.py resolved (see Blockers)

- test_round_trip.py: rename second-section _assert_fixed_point /
  _LOCKED_ATTRS to _assert_property_fixed_point / _PROPERTY_LOCKED_ATTRS
  so the invariant tests at lines 149-682 keep their original
  byte-equivalent tolerance (np.testing.assert_array_equal +
  pytest.approx 1e-15) and the Hypothesis property tests use their
  intended math.isclose tolerance. Both helpers now coexist instead of
  one shadowing the other on module load.
- test_round_trip.py: drop the duplicate numpy/pytest/xarray imports
  in the Hypothesis section since they're already imported at the top.
- read/test_georef.py: update stale citation pointing at the deleted
  test_remaining_fail_closed_1987.py to point at the new home in
  unit/test_metadata.py.
- release_gates/test_features.py: add a docstring note that the
  release_gate markers are localised to the contract sections so
  pytest -m release_gate selects the right subset.
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 (round 2): geotiff tests: consolidate closing cluster 16 (#2440)

Follow-up review after commit cc3af56.

Blockers

None.

Suggestions

None.

Nits

None.

Round 1 disposition

  • Blocker _assert_fixed_point collision in test_round_trip.py: fixed by renaming the second-section symbol to _assert_property_fixed_point and updating the two call sites (lines 1048, 1113). Invariant tests at lines 149-682 now resolve to the original helper with np.testing.assert_array_equal + pytest.approx(b, abs=1e-15, rel=1e-12) semantics.
  • Blocker _LOCKED_ATTRS collision in test_round_trip.py: fixed by renaming the second-section tuple to _PROPERTY_LOCKED_ATTRS. Both attr lists coexist instead of one shadowing the other.
  • Suggestion (stale citation in read/test_georef.py:1979): fixed; now points at unit/test_metadata.py::test_read_rejects_rotated_vrt.
  • Nit (mixed-scope release_gates/test_features.py): fixed; module docstring now notes the release_gate markers are localised to the contract sections so pytest -m release_gate selects the right subset.
  • Nit (duplicate imports in test_round_trip.py:689-694): fixed; math and os kept since they're unique to that section, the duplicated numpy/pytest/xarray/open_geotiff/to_geotiff lines dropped.

What looks good

  • The two helpers now coexist with distinct names instead of one silently shadowing the other on module load.
  • Local pytest run on test_round_trip.py stays green (22 passed) after the rename.
  • Full geotiff suite stays green (5936 passed, 81 skipped, 2 xfailed) after the follow-up.
  • No new helper-name or constant collisions detected across the 13 merged files.

Checklist

  • All round-1 blockers fixed
  • All round-1 suggestions fixed
  • All round-1 nits fixed
  • No new findings on round 2
  • pytest xrspatial/geotiff/tests/test_round_trip.py -q green
  • pytest xrspatial/geotiff/tests/ -q green

…2440)

Final pre-merge gate. The cluster-16 audit doc was a working artefact
used to capture per-section moves, helper renames, and citation
rewires while building the consolidation. Every prior cluster issue
deleted its audit doc in a dedicated final commit before merge
(#2474, #2476). Closing cluster follows the same convention.
…2440)

Two follow-ups after the consolidation surfaced fresh issues:

1. CI fast-lane failure (ImportError: lerc) in
   ``test_dask_numpy_parity[compression_lerc_float32]``. The original
   top-level ``test_golden_corpus_dask_numpy_1930.py`` was never
   reached by the ``pytest xrspatial/geotiff/tests/golden_corpus/``
   workflow target, so a latent bug in its ``_build_param`` (no call
   to ``fast_slow_marks_for``) was invisible. The Sub-PR B relocation
   moved the file into ``golden_corpus/`` per the issue spec, and the
   fast-lane CI lane started running it without the ``slow`` filter
   applying. Wire ``fast_slow_marks_for`` into the three dask /
   GPU backend modules (``test_dask_numpy``, ``test_dask_gpu``,
   ``test_gpu``) so the manifest's tagged-slow ``compression_*``
   fixtures (including the optional-dep ``lerc`` cell) drop out of
   ``pytest -m "not slow"`` the same way they do in the eager
   backend module. Nightly / push-to-main lanes still exercise the
   full set.

2. The merge from origin/main pulled in
   ``test_file_source_context_2449.py`` (PR #2479's coverage for the
   new ``_FileSource`` context-manager protocol). That landed at the
   top level with a numeric suffix, pushing the Definition-of-Done
   counts off-target (top-level=6, suffixed=1). Folded into
   ``read/test_basic.py`` alongside the other input-source variants
   so the counts return to 5 top-level / 0 suffixed.

Verified locally:
- ``pytest xrspatial/geotiff/tests/golden_corpus/ -m "not slow" -q``
  green with 30 deselected (the slow compression cells, including
  ``lerc``).
- ``pytest xrspatial/geotiff/tests/golden_corpus/ -q`` green (full
  lane: 339 passed, 25 skipped).
- ``pytest xrspatial/geotiff/tests/ -q`` green (5941 passed, 81
  skipped, 2 xfailed).
@brendancol brendancol merged commit 5678a46 into main May 27, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment