VRT tests: negative coverage for unsupported features (#2370)#2375
Open
brendancol wants to merge 4 commits into
Open
VRT tests: negative coverage for unsupported features (#2370)#2375brendancol wants to merge 4 commits into
brendancol wants to merge 4 commits into
Conversation
Lock in the rejection contract for VRT inputs the release does not promise to support. The cases covered are: - Warped VRT (subClass="VRTWarpedRasterBand" and GDALWarpOptions) - Nested VRT (.vrt as <SourceFilename>) - Mixed source CRS across band sources - Mixed source dtype (complex dtype is already rejected; ambiguous cross-band widening is pinned for the validator PR) - Mixed band count across sources - Complex mask source semantics not representable in attrs - Unsupported resample algorithm (already rejected -- locked in) Each test checks the exception type and that the message names the unsupported feature. Both read_vrt and open_geotiff entry points are exercised. Cases that depend on the centralized validator from sibling PR #2329 are wrapped with pytest.xfail so this PR lands independently and starts passing once #2329 merges. Sub-task of #2342.
brendancol
commented
May 25, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: VRT tests: negative coverage for unsupported features (#2375)
Test-only PR. I read the new file xrspatial/geotiff/tests/test_vrt_unsupported_2370.py and ran the suite locally (9 passed, 8 xfailed, as advertised).
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
xrspatial/geotiff/tests/test_vrt_unsupported_2370.py:121:_assert_raises_or_xfailis defined under the "Group 1 -- Warped VRT" header but it is shared by every group. Move it above the Group 1 header (or put it under a dedicated "Helpers" header) so the section layout matches the code. Cosmetic, but it trips readers scanning the headers.xrspatial/geotiff/tests/test_vrt_unsupported_2370.py:141:except BaseExceptionis broader than needed and would swallowKeyboardInterrupt/SystemExitif one ever leaked out of XML setup.except Exceptionkeeps the diagnostic xfail for genuine errors without masking interruption signals.
Nits (optional improvements)
xrspatial/geotiff/tests/test_vrt_unsupported_2370.py:79:_simple_source_xmlacceptsx_size,y_size,dst_x_size,dst_y_size,extra_innerparameters that no caller in this PR uses. Either drop them or note in the docstring that they are forward-compat for follow-up work.- The resample-alg tests overlap by design with
test_vrt_resample_alg_1751.py. A one-line note in the module docstring explaining the split (this file is the rejection-contract anchor; the #1751 file is the regression anchor) would save a future reader from de-duping them.
What looks good
- Every test checks both the exception type and that the message names the unsupported feature, matching the issue's acceptance criteria.
_assert_raises_or_xfailproduces one consistent xfail reason naming PR #2329, so a reader who sees an xfail does not have to dig into the test to learn why.- Coverage spans
read_vrtandopen_geotifffor the cases where a regression at either entry point would leak past the contract (warped, nested, resample alg). - The supported-VRT anchor test prevents the file from quietly degrading if
.vrtextension dispatch breaks. - All tmp filenames carry
2370, matching the project's collision-avoidance convention. - The two cases the current code already rejects (complex dtype
CFloat32, non-nearest resample alg) lock in real messages withpytest.raises(..., match=...)rather than the looser xfail-friendly helper.
Checklist
- Test-only change; correctness checks N/A for production code
- Four-backend coverage N/A (VRT read path is numpy-eager today)
- NaN handling N/A
- Edge cases captured via crafted VRT XML
- Dask chunk boundaries N/A
- No performance-sensitive code changed
- No README or docs update needed (test-only PR)
- Docstrings present on every test
-
Closes #2370in the PR body
- Move _assert_raises_or_xfail above the Group 1 header so it does not look like a Group 1 helper. - Narrow the diagnostic catch from BaseException to Exception so KeyboardInterrupt and SystemExit still propagate. - Drop unused parameters from _simple_source_xml and pin the helper to the 4x4 SrcRect/DstRect geometry every caller actually uses. - Document the intentional resample-alg overlap with test_vrt_resample_alg_1751.py in the module docstring.
brendancol
commented
May 25, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review (follow-up): commit 6f92559
Re-reviewed after the follow-up commit. All four findings from the first pass were addressed:
- Helper moved above the Group 1 header into a dedicated Helpers section.
except BaseExceptionnarrowed toexcept Exceptionon the diagnostic branch, with a docstring note explaining why.- Unused parameters dropped from
_simple_source_xml; the helper is now pinned to the 4x4 geometry every caller uses. - Module docstring now explains the intentional overlap with
test_vrt_resample_alg_1751.py.
Re-ran the suite locally: 9 passed, 8 xfailed -- no change in disposition, which is the expected result for these refactors.
Blockers
None.
Suggestions
None.
Nits
None.
The PR is ready as far as the test contract goes. CI is the next gate.
PR1 (#2329) landed via main while this PR was in review. The centralised validator now rejects unsupported resample algorithms at parse time with VRTUnsupportedError (a ValueError subclass) before the legacy resample-site NotImplementedError can fire. Broaden the expected exception type in the open_geotiff test to accept either rejection path so the contract test stays green under both code paths. The read_vrt parametrised test still pins NotImplementedError because that helper does not go through the new validator yet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
read_vrtandopen_geotiff(... .vrt ...)entry points are exercised.Backend coverage
Test-only PR. The VRT read path is numpy-eager; backend dispatch is not in scope.
Coordination
PR 3 of epic #2342. Sibling PR 1 (#2329) is the centralized
validate_vrt_capability. Where the current code already rejects a case (complex dtype #1783, non-nearest resample alg #1751, bad source band number), the test pins the existing behaviour. Where the rejection depends on #2329, the test is wrapped withpytest.xfail(reason="depends on centralized validate_vrt_capability (#2329)")so this PR can land on its own. Those tests will flip to passing once #2329 merges.Test plan
pytest xrspatial/geotiff/tests/test_vrt_unsupported_2370.py: 9 passed, 8 xfailedtest_vrt_resample_alg_1751,test_vrt_dtype_1783)2370to avoid collisions with parallel rockout worktreesCloses #2370.