Add consolidated VRT missing_sources policy matrix tests (#2367)#2372
Merged
Conversation
Cover the four-by-two contract matrix for VRT missing_sources in one file: default / explicit-raise / warn / invalid, each exercised against both the eager read_vrt path and the dask open_geotiff(.vrt, chunks=) path. Assertions cover exception types, warning class, message text, and output array values (NaN fill on the missing half, PRESENT_FILL on the present half). Complements the existing 1799 / 1843 / 1860 / 2265 tests by keeping the full matrix together so a future kwarg refactor that drops eager/dask parity regresses a single focused file. Work item for epic #2342.
brendancol
commented
May 25, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: VRT missing_sources policy matrix tests (#2367)
Blockers
None.
Suggestions
None.
Nits
test_vrt_missing_sources_policy_2367.py:113-117: the eager warn test usespytest.warns(GeoTIFFFallbackWarning) as recordand re-checks the message in the body instead of passingmatch=. Either style works; flagging only because the sibling 1799 test goes withmatch=.test_vrt_missing_sources_policy_2367.py:262-269: nice parametrize set on the bad values ('ignore','RAISE','raises', empty, trailing space,'1'). The value-validation block atvrt.py:399-402runs before chunked dispatch, so the chunked invocations hit the same error path, which is the cross-mode parity this matrix is meant to pin.
What looks good
- Four-by-two matrix in one file: default / explicit-raise / warn / invalid against both eager and dask readers.
- Output value assertions on the warn path (NaN on the missing half, 7.0 on the present half) under both readers, not just "did it warn".
- Warning class is checked as
GeoTIFFFallbackWarning, not bareUserWarning. - Build-time
vrt_holesis asserted on the dask warn path so callers can branch on partial mosaics without forcing a compute. - Temp filenames all carry
_2367so parallel runs don't collide. - The parametrize-by-reader pattern keeps eager and dask symmetric for the raise / invalid cases without duplicating the test bodies.
Checklist
- Test-only change; algorithm correctness not applicable
- Both supported read paths exercised (eager
read_vrt, daskopen_geotiff(.vrt, chunks=)) - NaN handling on the missing region asserted
- Edge cases on the bad-value side (empty string, trailing space, wrong case) covered
- Dask path's parse-time
vrt_holesand compute-time warning both pinned - No performance-critical paths touched
- No new public API; README / docs untouched on purpose
- Docstrings on every test class explain what they pin and why
Contributor
Author
|
Followed up on the Nit 1 from the prior review (sibling test parity with #1799): the eager warn assertion now uses |
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.
Closes #2367. Sub-task of epic #2342.
Adds a single test file that pins the four-by-two policy matrix for VRT
missing_sources:'raise'/'warn'/ invalid valueread_vrtand daskopen_geotiff(.vrt, chunks=...)Assertions cover the exception type and message (raise / invalid), the
GeoTIFFFallbackWarningclass plus message text (warn), and the actual output array values (NaN on the missing half, fill value on the present half).The existing tests at
test_vrt_missing_sources_policy_1799.py,test_vrt_chunked_missing_sources_1799.py,test_vrt_missing_sources_default_raise_1843.py, andtest_read_vrt_default_missing_sources_1860.pyeach pin a slice of this matrix. They stay in place; this file keeps the full matrix together so a kwarg refactor that drops eager/dask parity regresses one focused file.Test plan
pytest xrspatial/geotiff/tests/test_vrt_missing_sources_policy_2367.py(18 passed locally)