Repository navigation
grass.pygrass: Close RasterRow via context manager in region tests - #7902
Merged
Merged
Conversation
test_resampling_1 and test_resampling_2 called rast.close() as a bare statement after their assertions, so a failing assertion (or any other exception) would skip it, leaving the RasterRow open. This repo's own Region.set_raster_region() documents the consequence: "All raster objects must be closed or the process will be terminated." The next test in this file, test_resampling_to_numpy, calls exactly that method on every resampling step, so an open handle carried over from an earlier test crashes it with an unrelated-looking C-level FERROR instead of the real assertion failure ever being reported. Use `with rast:` instead, matching RasterRow's documented context manager support, so the raster is always closed, including when an assertion fails. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
Python
ninsbl
approved these changes
Sep 6, 2026
echoix
added a commit
to echoix/grass
that referenced
this pull request
Sep 6, 2026
…SGeo#7902) test_resampling_1 and test_resampling_2 called rast.close() as a bare statement after their assertions, so a failing assertion (or any other exception) would skip it, leaving the RasterRow open. This repo's own Region.set_raster_region() documents the consequence: "All raster objects must be closed or the process will be terminated." The next test in this file, test_resampling_to_numpy, calls exactly that method on every resampling step, so an open handle carried over from an earlier test crashes it with an unrelated-looking C-level FERROR instead of the real assertion failure ever being reported. Use `with rast:` instead, matching RasterRow's documented context manager support, so the raster is always closed, including when an assertion fails. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
3 of 6 tasks
echoix
added a commit
to echoix/grass
that referenced
this pull request
Sep 6, 2026
Follow-up to OSGeo#7902: fixing the resource leak there turned a fatal, whole-file-crashing FERROR into three individually diagnosable failures instead. The remaining issue is real but distinct from the leak: the small region these tests set up isn't actually restricting what RasterRow reads back (rast[0] comes back with roughly 100000 elements instead of 10). Built GRASS locally and confirmed this is genuinely Windows-specific, not a state leak reproducible here: the file passes reliably on Linux both run in isolation and as part of the full 359-file gunittest suite in sequence. Marking xfail on Windows per the existing convention rather than guessing at a fix that can't be verified; needs investigation with actual Windows access.
3 of 6 tasks
echoix
added a commit
that referenced
this pull request
Sep 8, 2026
…ows (#7905) Follow-up to #7902: fixing the resource leak there turned a fatal, whole-file-crashing FERROR into three individually diagnosable failures instead. The remaining issue is real but distinct from the leak: the small region these tests set up isn't actually restricting what RasterRow reads back (rast[0] comes back with roughly 100000 elements instead of 10). Built GRASS locally and confirmed this is genuinely Windows-specific, not a state leak reproducible here: the file passes reliably on Linux both run in isolation and as part of the full 359-file gunittest suite in sequence. Marking xfail on Windows per the existing convention rather than guessing at a fix that can't be verified; needs investigation with actual Windows access.
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.
This is to address a file that completely crashes instead of failing a single test (affected the others in the file). Part of the cleanup before randomizing tests order (for at least Windows).
Note that now, this allows 3 tests to fail on Windows (still under the threshold), instead of crashing (so hidden). I have another PR stacked up for that.
The rest below is AI explaining using context managers instead of not executing close() when the code had an exception.
Description
test_raster_region.py'stest_resampling_1andtest_resampling_2calledrast.close()as a bare statement after their assertions. If an assertion(or anything else) raised, that
close()call was skipped and theRasterRowstayed open for the rest of the process.Region.set_raster_region()documents exactly why that matters:test_resampling_to_numpy, the next test in this file, callsregion.set_raster_region()on every resampling step. So a raster left openby an earlier test doesn't just fail that earlier test — it crashes the
next test with an unrelated-looking C-level
FERROR: Input window changed while maps are open for read, instead of whatever the real original failurewas.
Fixed by using
with rast:(RasterRowalready documents and supports this)instead of a manual
open()/close()pair, so the raster is always closed,assertion failure or not.
Known follow-up
Windows CI on this branch shows the fatal, whole-file-crashing
FERRORisgone, but the file still doesn't fully pass: three individual failures show
up instead (count/value mismatches in
test_resampling_1andtest_resampling_2, and an oversized array allocation intest_resampling_to_numpy). These point to a separate, pre-existing bugwhere the small test region doesn't appear to actually restrict what gets
read back on Windows. I haven't been able to reproduce it without a Windows
machine, so I'm not attempting a fix for it here — this PR only fixes the
resource leak that was masking it.
Motivation and context
Found while investigating Windows CI test-order-dependency:
test_raster_region.pywas one of a small set of consistently-failingWindows gunittest files.
How has this been tested?
Windows CI on this branch (see the follow-up note above) — I couldn't
reproduce or build GRASS on Windows locally.
Types of changes
Checklist
pre-defined prefixes
of this project.
AI wrote the message, and researched a bit.