GeoTIFF: add release gate / audit checklist (#2321 sub-task 6)#2336
Conversation
The new docs/source/reference/release_gate_geotiff.rst enumerates every GeoTIFF promise that release notes are allowed to make: local read/write, COG, HTTP/fsspec, nodata lifecycle, attrs contract, VRT supported subset, sidecar/overview interactions, and GPU experimental paths. Each row pairs a tier with a one-line acceptance and at least one regression test path. xrspatial/geotiff/tests/test_release_gate_2321.py locks the checklist against drift: * every cited test file exists on disk, * every promised SUPPORTED_FEATURES key shows up in the checklist prose, * HTTP SSRF rejects loopback hosts (with an xfail row for the uppercase-scheme case that lands with sub-PR 5 / #2326), * VRT rows resolve to non-empty test files. Refs #2321.
brendancol
left a comment
There was a problem hiding this comment.
PR Review: GeoTIFF: add release gate / audit checklist (#2321 sub-task 6)
Scope is documentation plus a meta-test that gates the doc against drift. No new kernels, no new dispatch, no new public API. The correctness / backend / performance checklist items are N/A. Findings focus on the doc-as-contract and the meta-test's failure modes.
Blockers
None.
Suggestions
-
xrspatial/geotiff/tests/test_release_gate_2321.py:153-- thexfail(raises=Exception)is wider than it needs to be. Today the uppercase scheme path raisesValueErrorfrom fsspec (Protocol not known: HTTP). Narrowraises=to(ValueError, UnsafeURLError)so a future regression that produces a different exception class (RuntimeError, anOSErrorfrom a real socket dial, etc.) does not silently xfail. When sub-PR 5 / #2326 lands, the xfail flips to a pass and the test starts enforcing theUnsafeURLErrorpromise. -
docs/source/reference/release_gate_geotiff.rst:268-- the HTTP SSRF defense row points attest_release_gate_2321.pyfor its only regression test, but the real SSRF rejection logic is already exercised by the SSRF suite (e.g.xrspatial/geotiff/tests/test_cog_http_*.pyand the HTTP source tests). Citing only the meta-gate makes the row look thinly covered. Add the existing SSRF tests to the "Regression test" cell so a reader sees the depth of coverage. -
xrspatial/geotiff/tests/test_release_gate_2321.py:62--_TEST_PATH_REmatches anyxrspatial/geotiff/tests/<word>/<word>.pypath, including non-test_files likeconftest.py. That is permissive but it would let a typo in the checklist (citingconftests.py) sneak by as long as the file exists. Tighten the regex to require atest_prefix on the leaf name, or add a second assert that every cited file starts withtest_.
Nits
-
docs/source/reference/release_gate_geotiff.rst:1-- the title overline / underline length is exact now, but the section underlines below it (Local GeoTIFF read and write, etc.) use=and are sometimes longer than the title. Mixed style. Not a build error; just visually noisy in raw rst. -
docs/source/reference/index.rst:18-- new entry placed aftergeotiff_internals. Consistent with the existing flat-alphabetical-ish ordering. Could co-locate withgeotiffinstead for navigability, but the current placement is fine and matches the surrounding pattern. -
xrspatial/geotiff/tests/test_release_gate_2321.py:96--_PROMISED_TIERS = {"stable", "advanced"}-- worth a one-line comment thatexperimentalandinternal_onlyare deliberately excluded because the checklist tags them as not-promised.
What looks good
- The meta-test approach (parse the rst, cross-reference
SUPPORTED_FEATURES) is the right shape -- a missing test file or a new public tier without a row both surface as a hard test failure, not a doc-only drift. - The
Placeholder PR cross-referencessection gives the rockout final summary a concrete list of links to swap in once sub-PRs 1-4 land. - The xfail on the uppercase SSRF case is honest -- it documents the dependency on sub-PR 5 in the test's reason string rather than hiding the gap.
- The
.. seealso::link fromgeotiff.rstkeeps the audit checklist discoverable from the main GeoTIFF reference page.
Checklist
- Algorithm matches reference/paper -- N/A (docs/tests only)
- All implemented backends produce consistent results -- N/A
- NaN handling is correct -- N/A
- Edge cases covered by tests -- meta-test covers the realistic drift modes
- Dask chunk boundaries handled correctly -- N/A
- No premature materialization -- N/A
- Benchmark exists or is not needed -- not needed
- README feature matrix updated -- not needed (per rockout brief)
- Docstrings present and accurate -- the test module's docstring documents what each gate pins
* Narrow the xfail on the uppercase HTTP scheme SSRF test to ``raises=(ValueError, UnsafeURLError)`` so a future regression that produces a different exception class does not silently xfail. * Extend the test that checks cited paths to require a ``test_`` leaf prefix, so the checklist cannot accidentally cite ``conftest.py`` or a helper module. * Add a one-line comment on ``_PROMISED_TIERS`` explaining why ``experimental`` and ``internal_only`` are excluded. * Add the dedicated SSRF suites (``test_ssrf_hardening_1664.py``, ``test_dns_rebinding_pin_issue_1846.py``) to the HTTP SSRF defense row so the row reflects the real depth of coverage, not just the meta-gate. Refs #2321.
brendancol
left a comment
There was a problem hiding this comment.
PR Review (follow-up): GeoTIFF release gate / audit checklist
Second pass after the review fixes in commit 6bafc72.
Disposition of original findings
Suggestions:
- (fixed)
xrspatial/geotiff/tests/test_release_gate_2321.py:153--raises=is now(ValueError, UnsafeURLError). A regression that raises some other exception class will fail loudly instead of silently xfailing. - (fixed)
docs/source/reference/release_gate_geotiff.rst:268-- the HTTP SSRF defense row now citestest_ssrf_hardening_1664.pyandtest_dns_rebinding_pin_issue_1846.pyalongside the meta-gate. - (fixed)
xrspatial/geotiff/tests/test_release_gate_2321.py:62-- gate 1 now also asserts every cited path's leaf name starts withtest_.
Nits:
- (dismissed) Mixed underline length in the rst: cosmetic only, the existing reference pages use the same loose style, and forcing a uniform width would just churn the diff.
- (dismissed) Toctree placement after
geotiff_internals: matches the surrounding flat ordering and the review already noted the current placement is fine. - (fixed)
_PROMISED_TIERSnow carries a comment explaining whyexperimentalandinternal_onlyare excluded.
Re-check
pytest xrspatial/geotiff/tests/test_release_gate_2321.py -- 4 passed, 1 xfailed (uppercase SSRF, pending #2326).
No new findings. Approving in spirit, but per rockout rules this stays a COMMENTED review rather than an APPROVED one.
Closes #2331. Refs #2321 (sub-task 6 of 6).
Summary
docs/source/reference/release_gate_geotiff.rst, a per-feature release checklist for the GeoTIFF module. Every row pairs a tier (stable/advanced/experimental/internal_only) with a one-line acceptance and at least one regression test path. Sections: local read/write, COG, HTTP/fsspec, nodata lifecycle, attrs contract, VRT supported subset, sidecar and overview interactions, GPU experimental paths, internal-only surfaces, and cross-cutting CI gates.xrspatial/geotiff/tests/test_release_gate_2321.py, a meta-gate that keeps the checklist honest:SUPPORTED_FEATURESkey (tiersstableoradvanced) is named in the checklist prose, so a new public tier cannot ship without a row;docs/source/reference/index.rstand adds a.. seealso::link fromdocs/source/reference/geotiff.rstso readers find the audit trail from the main GeoTIFF reference page.Backend coverage
Docs and meta-tests only. No backend-specific code paths added.
Placeholders to fill in
The checklist cites sub-PRs 1 through 5 of #2321. Only sub-PR 5 has a real PR today (#2326); the rest are tracked under #2321 as
(see #2321). Once each sub-PR lands, swap its placeholder for the real PR number both inrelease_gate_geotiff.rstand in the parent issue's tracking comment.Test plan
pytest xrspatial/geotiff/tests/test_release_gate_2321.py(4 passed, 1 xfailed)pytest xrspatial/geotiff/tests/test_supported_features_tiers_2137.py(still green; no regression)docutilsparse of the new.rstsucceeds with only the expected Sphinx-role notices (:data:,:class:,:ref:)