Skip to content

geotiff tests: consolidate attrs-contract cluster#2405

Merged
brendancol merged 3 commits into
mainfrom
issue-2397
May 26, 2026
Merged

geotiff tests: consolidate attrs-contract cluster#2405
brendancol merged 3 commits into
mainfrom
issue-2397

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2397. Part of epic #2390 (GeoTIFF test consolidation, PR 5 of 11).

Summary

Folds the four test_attrs_contract_*_1984.py files into one parametrised attrs/test_contract.py under a new attrs/ package. Tests are organised by contract dimension (canonical / aliases / passthrough / version) with descriptive parametrise IDs.

  • test_attrs_contract_canonical_1984.py -> attrs/test_contract.py (canonical section, 17 cases)
  • test_attrs_contract_aliases_1984.py -> attrs/test_contract.py (aliases section, 6 cases)
  • test_attrs_contract_passthrough_1984.py -> attrs/test_contract.py (passthrough section, 9 cases)
  • test_attrs_contract_version_1984.py -> attrs/test_contract.py (version section, 8 cases)

40 cases in, 40 cases out. Coverage parity is recorded in CLUSTER_AUDIT_PR5.md (deleted on the final commit before approval).

Out of scope

  • test_attrs_finalization_parity_2211.py, test_attrs_parity_1548.py, test_attrs_kwarg_parity_1561.py are parity-flavoured (PR 4 territory).
  • test_release_gate_attrs_contract.py belongs to PR 10 (release-gate registry).
  • attrs/test_roundtrip.py is deferred to a follow-up; nothing in the four source files breaks out as a clean roundtrip slice without duplicating tier coverage.

Behaviour preserved

  • Tests-only restructure. No source changes.
  • Markers come from _helpers/markers.py; no per-file GPU probe.
  • Release-gate checklist (release_gate_geotiff.rst) updated to point at attrs/test_contract.py so the cited-files audit (test_release_gate_2321.py::test_release_gate_cites_only_existing_test_files) stays green.

File delta

  • 4 source files deleted, 1 added: net -3.

Test plan

  • pytest xrspatial/geotiff/tests/attrs/ -v -> 40 passed (3 of those skipped locally on CPU-only environments due to requires_gpu).
  • pytest xrspatial/geotiff/tests/ -x -q -> 5706 passed, 68 skipped, 6 xfailed, 1 xpassed.
  • Release-gate test (test_release_gate_2321.py) green after the doc update.

Refs epic #2390.

Folds the four ``test_attrs_contract_*_1984.py`` files into one
parametrised ``attrs/test_contract.py`` under the new ``attrs/``
package. Tests are organised by contract dimension (canonical /
aliases / passthrough / version) with descriptive parametrise IDs
(``canonical[GDAL_NODATA]``, ``alias[nodatavals->nodata]``, etc.)
rather than per-issue filenames.

- 4 source files deleted, 1 new file added (-3 net).
- 40 test cases retained; coverage parity verified in
  ``CLUSTER_AUDIT_PR5.md`` (audit file deleted before merge).
- Release-gate checklist (release_gate_geotiff.rst) updated to point
  at the new path; the cited-files audit gate stays green.
- Markers come from ``_helpers/markers.py`` (no per-file gpu probe).
- Tests-only restructure; no source changes.

Part of epic #2390 (GeoTIFF test consolidation, PR 5 of 11).
@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 attrs-contract cluster

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/geotiff/tests/attrs/test_contract.py — the contract-version stamp is asserted in three places after this PR: test_canonical_value_roundtrip[canonical[contract_version]], test_passthrough_contract_version_is_current, and test_version_constant_is_current plus the six per-backend test_version_stamp_present_per_*_backend cases. The duplication was already in the source files (each tier file pinned the stamp independently), but a consolidation is a good moment to drop the redundant arms. Worth dropping test_passthrough_contract_version_is_current: the canonical fixture already exercises a stamped round-trip and the version section owns per-backend stamp coverage.

  • xrspatial/geotiff/tests/attrs/test_contract.py:296 — comment says contract-version stamp coverage "lives in the version section below". The version section asserts stamping on six backends but never asserts value-equality against _ATTRS_CONTRACT_VERSION on the canonical fixture (which carries extra_tags + the opt-in flag). The canonical fixture's stamp value check is the eager-only canonical[contract_version] case. Worth a one-line note in the docstring that the value-equality check is eager-only by design so a future reviewer does not try to "fix" the gap.

Nits (optional improvements)

  • xrspatial/geotiff/tests/attrs/test_contract.py:268-274path = str(tmp_path / f'canonical_{opener.__name__}.tif') uses the leading-underscore function name (_open_eager) in the filename. Using the pytest.param id (canonical[eager-numpy]) via request.node.callspec.id would be tidier but adds boilerplate. Fine as-is unless someone trips on it.

  • xrspatial/geotiff/tests/attrs/test_contract.py:233-242_check_canonical_* helpers all take (rd, _expected_transform) but only _check_canonical_transform uses the second arg. The leading underscore communicates intent; flagging in case ruff ARG001 ever becomes a gate.

  • xrspatial/geotiff/tests/CLUSTER_AUDIT_PR5.md — the "Roundtrip slice deferral" section is the right call but reads like a future-tense plan. A sentence on what the natural roundtrip slice would look like (one fixture, write -> read with every canonical key, assert structural equality of the attrs dict) would help PR 6/10 deciders pick it up later.

  • docs/source/reference/release_gate_geotiff.rst:440 — the "Attrs pass-through on write" row previously cited two files (passthrough + aliases) and now cites only one. PR 10's release-gate registry can decide whether to keep separate rows for passthrough vs aliases now that the citation is one file.

What looks good

  • Four source files deleted in the same commit as the new file lands; no transitional duplication.
  • CLUSTER_AUDIT_PR5.md maps every old test name to a new test ID and includes a coverage parity count (40 -> 40).
  • The release-gate cited-files audit (test_release_gate_2321.py::test_release_gate_cites_only_existing_test_files) was caught and fixed in the same PR by updating release_gate_geotiff.rst.
  • Test IDs use the contract dimension (canonical[...], alias[...], passthrough[...], version[...]) per the epic's convention. No issue numbers in IDs.
  • Marker swap from per-file _gpu_only to _helpers.markers.requires_gpu is semantically identical and consolidates one more probe site.
  • 40-case parametrise expansion verified with --collect-only; pytest xrspatial/geotiff/tests/attrs/ is 40/40 and pytest xrspatial/geotiff/tests/ is 5706 passed, 68 skipped, 6 xfailed locally.

Checklist

  • Algorithm matches reference/paper: N/A (tests-only restructure)
  • All implemented backends produce consistent results: per-backend coverage preserved across the marker swap
  • NaN handling: NaN-in-nodatavals test preserved verbatim
  • Edge cases covered: raster_type area/point both branches preserved
  • Dask chunk boundaries: N/A for this PR
  • No premature materialization: N/A
  • Benchmarks: N/A
  • README feature matrix: N/A (no new public API)
  • Docstrings present and accurate: module docstring covers the four sections and the out-of-scope boundary

…2397)

- Drop ``test_passthrough_contract_version_is_current``: the same
  stamp value is asserted by ``canonical[contract_version]`` on a
  richer fixture and by the per-backend ``version[*]`` tests.
- Switch ``_BACKEND_OPENERS`` and the version-section opener lists
  to ``(opener, label)`` tuples so tmp_path filenames use the
  parametrize label (``canonical_eager-numpy.tif``) instead of the
  leading-underscore function name (``canonical__open_eager.tif``).
- Expand the docstring above ``_BACKEND_OPENERS`` to spell out that
  value-equality on the canonical fixture is eager-only by design
  (the per-backend loop is presence-only).
- ``CLUSTER_AUDIT_PR5.md`` now records the intentional drop, updates
  the coverage parity count (40 -> 39), and adds a sentence
  describing what the deferred ``attrs/test_roundtrip.py`` slice
  should look like.

Part of epic #2390 (GeoTIFF test consolidation, PR 5 of 11).
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): post-fix pass

Follow-up after commit f5546017. The previous review's Suggestions and Nits 3/Nit 1 are all addressed; Nits 2/4 left as-is with rationale below.

Blockers

None.

Suggestions

None remaining. The redundant test_passthrough_contract_version_is_current is gone; the canonical-section docstring now spells out that value-equality is eager-only by design.

Nits

None remaining. The (opener, label) tuple swap removes the leading-underscore filename quirk in both the canonical and version sections.

Dispositions for the first-round findings

  • Suggestion 1 (redundant stamp test): fixed -- dropped, audit table reflects the intentional removal.
  • Suggestion 2 (docstring note on eager-only value check): fixed -- comment above _BACKEND_OPENERS expanded.
  • Nit 1 (filename uses leading-underscore function name): fixed -- parametrize now passes (opener, label) and the filename uses the label.
  • Nit 2 (unused param in _check_canonical_* helpers): dismissed -- the leading-underscore name signals intent and ruff ARG001 is not currently a gate. Re-applying would require either dropping the uniform signature (and threading conditional args) or adding a noqa per helper.
  • Nit 3 (audit Roundtrip slice deferral wording): fixed -- added a paragraph describing what the natural roundtrip slice would look like.
  • Nit 4 (release-gate row consolidation for PR 10): deferred -- out of scope for this PR (PR 10 owns the release-gate registry), noted in the audit's out-of-scope section.

Test status

  • pytest xrspatial/geotiff/tests/attrs/ -> 39 passed (was 40; one intentional drop).
  • pytest xrspatial/geotiff/tests/ -> 5705 passed, 68 skipped, 6 xfailed, 1 xpassed.

Ready to merge once CI is green.

@brendancol brendancol merged commit 4ccd98c 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 attrs-contract tests (epic #2390 PR 5)

1 participant