Skip to content

r.kappa: Avoid reopening NamedTemporaryFile on Windows in testsuite - #694

Closed
echoix wants to merge 3 commits into
mainfrom
claude/r-kappa-fix-separation-fjn5nx
Closed

echoix wants to merge 3 commits into
mainfrom
claude/r-kappa-fix-separation-fjn5nx

Conversation

@echoix

@echoix echoix commented Sep 6, 2026 •

Copy link
Copy Markdown
Owner

Description

test_file in raster/r.kappa/testsuite/test_r_kappa.py kept a
NamedTemporaryFile handle open while passing its path to r.kappa as the
output file. Windows rejects a second process opening a file while the
creating process still holds the handle open; POSIX allows it. Use
grass.script.core.tempfile() to get an unopened path instead, the same fix
already applied to r.mfilter's testsuite in OSGeo#7878, and drop the
now-unneeded @xfail_windows marker.

Motivation and context

Split out of #683, which bundled this fix together with unrelated Windows CI
changes (randomized test order infrastructure). This is a standalone,
independently mergeable fix.

How has this been tested?

Mirrors the already-merged, already-verified fix for the identical
anti-pattern in r.mfilter (OSGeo#7878).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • PR title provides summary of the changes and starts with one of the
    pre-defined prefixes
  • My code follows the code style
    of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

🤖 Generated with Claude Code

)

* Apply suggested fix to lib/raster3d/tilewrite.c from Copilot Autofix

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

* lib/raster3d: Add test for Rast3d_write_tile tile index bounds check

Add a pytest test covering the tileIndex bounds check in
Rast3d_write_tile: an index equal to nTiles must be rejected as out of
range, and the last valid index must be accepted. The invalid call runs
in a subprocess using the grass.lib.raster3d ctypes bindings because the
rejection is a fatal error which exits the calling process.

The out-of-range test fails without the bounds check change in this
branch and passes with it.

The test was written by Claude Code using the Claude Fable 5 model.

---------

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Vaclav Petras <wenzeslaus@gmail.com>
echoix pushed a commit that referenced this pull request Sep 6, 2026
The NamedTemporaryFile fix for test_file was bundled into this PR
together with unrelated Windows CI changes. Split it into its own PR
(#694) so it can be reviewed and merged independently; revert it here
to avoid landing it twice.

This reverts commit cfcb92f.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdSJkp8teAQ8W3dQKMRdz9
test_file kept a NamedTemporaryFile handle open while passing its path
to r.kappa as the output file. Windows rejects a second process
opening a file while the creating process still holds the handle;
POSIX allows it. Use grass.script.core.tempfile() to get an unopened
path instead, the same fix already applied to r.mfilter's testsuite in
OSGeo#7878, and drop the now-unneeded @xfail_windows marker.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@echoix
echoix force-pushed the claude/r-kappa-fix-separation-fjn5nx branch from 59eeb27 to 09cfcfc Compare September 6, 2026 12:40
echoix added a commit that referenced this pull request Sep 6, 2026
The NamedTemporaryFile fix for test_file was bundled into this PR
together with unrelated Windows CI changes. Split it into its own PR
(#694) so it can be reviewed and merged independently; revert it here
to avoid landing it twice.

This reverts commit 124fe83.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@echoix echoix closed this Sep 12, 2026
@echoix
echoix deleted the claude/r-kappa-fix-separation-fjn5nx branch September 12, 2026 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant