tests: remove the temporary directory make_nifti_image creates - #9042
tests: remove the temporary directory make_nifti_image creates#9042aymuos15 wants to merge 2 commits into
Conversation
make_nifti_image creates a mkdtemp directory to hold the image but returns only the file path, so the directory outlives every call. The four callers that delete the file still leave the directory behind; running tests/data/test_nifti_rw.py alone left 65 of them in the system temp dir. Register the directory for removal at interpreter exit when the helper created it, and leave caller-supplied directories alone. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/data/test_make_nifti.py (3)
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local variable
dir.The loop variable shadows Python's
dirbuilt-in. Rename it todirectoryand update downstream references. Keep the externaldirkeyword only wheremake_nifti_imagerequires that API name.Ruff reports this shadowing. As per path instructions, use sensible PEP8-style variable names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/data/test_make_nifti.py` at line 36, Rename the loop variable dir to directory in the affected test and update all downstream references, while retaining the external dir keyword only in make_nifti_image calls where the API requires it.Sources: Path instructions, Linters/SAST tools
58-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck caller-owned retention after child exit.
This assertion runs before
TemporaryDirectoryexits. It would also pass ifmake_nifti_imageincorrectly registeredcaller_dirforatexitcleanup. Create the directory in the parent process, invoke the helper in a subprocess, and assert that the directory still exists after the child exits.As per path instructions, cover the changed ownership contract with a unit test. The PR objective requires caller-provided directories to remain after interpreter exit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/data/test_make_nifti.py` around lines 58 - 62, Update test_caller_owned_dir_kept to create the caller-owned directory before launching a subprocess, invoke make_nifti_image in that child process using the directory, then assert the directory still exists after the child exits. Ensure the test verifies retention across interpreter shutdown rather than only before TemporaryDirectory cleanup.Source: Path instructions
49-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for discarded and deleted outputs.
The subprocess test keeps
created_fileand leaves the generated file in place. Add one case that discards the return value and another that removes the generated file before exit. Assert that the temporary directory is absent after the child exits in both cases.As per path instructions, add unit-test coverage for the changed behavior. The PR objective also names discarded-return and removed-file cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/data/test_make_nifti.py` around lines 49 - 56, Add coverage in test_temp_dir_removed_at_exit for both cleanup scenarios: run a subprocess that discards make_nifti_image’s return value, and another that deletes the generated file before exiting. Assert the temporary directory is absent after each child process exits, while retaining the existing created-file case.Source: Path instructions
tests/test_utils.py (1)
396-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the Google-style contract for
make_nifti_image.The changed docstring states ownership only. Add
Argsentries for every parameter, aReturnsentry for the generated path, and aRaisessection for documented exceptions. State that a missingdiris created and removed at interpreter exit, while a supplieddirremains caller-owned.As per path instructions, Python definitions require Google-style documentation for parameters, returns, and raised exceptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils.py` around lines 396 - 397, Complete the Google-style docstring for make_nifti_image by documenting every parameter under Args, the generated image path under Returns, and all documented exceptions under Raises. Preserve the ownership behavior: an omitted dir is created temporarily and removed at interpreter exit, while a supplied dir remains caller-owned.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/data/test_make_nifti.py`:
- Line 36: Rename the loop variable dir to directory in the affected test and
update all downstream references, while retaining the external dir keyword only
in make_nifti_image calls where the API requires it.
- Around line 58-62: Update test_caller_owned_dir_kept to create the
caller-owned directory before launching a subprocess, invoke make_nifti_image in
that child process using the directory, then assert the directory still exists
after the child exits. Ensure the test verifies retention across interpreter
shutdown rather than only before TemporaryDirectory cleanup.
- Around line 49-56: Add coverage in test_temp_dir_removed_at_exit for both
cleanup scenarios: run a subprocess that discards make_nifti_image’s return
value, and another that deletes the generated file before exiting. Assert the
temporary directory is absent after each child process exits, while retaining
the existing created-file case.
In `@tests/test_utils.py`:
- Around line 396-397: Complete the Google-style docstring for make_nifti_image
by documenting every parameter under Args, the generated image path under
Returns, and all documented exceptions under Raises. Preserve the ownership
behavior: an omitted dir is created temporarily and removed at interpreter exit,
while a supplied dir remains caller-owned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f9f9272-b116-400b-a64d-091fa5edf24b
📒 Files selected for processing (2)
tests/data/test_make_nifti.pytests/test_utils.py
1 participant
Fixes #9041
Description
make_nifti_imageintests/test_utils.pycreates atempfile.mkdtemp()directory to hold the image it writes, but returns only the file path. Its docstring made the caller responsible for cleanup:Of the 15 call sites across 9 test files, 11 discard the returned path entirely. The remaining four (
tests/data/test_nifti_rw.py,tests/integration/test_integration_sliding_window.py) do try to honour the contract, but theyos.removethe file and never remove the directory holding it, so they leak an empty directory per call instead of a populated one. Runningtests/data/test_nifti_rw.pyon its own left 65 directories in the system temp directory.Both callers that tried to clean up got it wrong the same way, because the helper creates two things and hands back one. Rather than editing 15 call sites, this makes the helper own what it created: when
diris not supplied, the directory it makes is registered for removal at interpreter exit. A caller-supplieddiris untouched; that one still belongs to the caller.ignore_errors=Trueis deliberate: several callers already delete the file inside the directory, and a test is free to delete what it was handed. Without it, an exit-time handler would turn a passing run into a traceback at shutdown over a directory that is already in the desired state.tempfile.TemporaryDirectorymakes the same call in its own finalizer.tests/data/test_make_nifti.pyalso built its own temp directory at module scope, inside a loop that ran three times, leaking three directories at import, before any test ran. That is hoisted to a single directory with the same exit-time cleanup.Changes
tests/test_utils.py:make_nifti_imageregisters the directory it creates for removal at exit; docstring updated to state who owns what. Addsatexit/shutilimports.tests/data/test_make_nifti.py: hoists the module-levelmkdtemp()out of the parameter loop (3 calls → 1) and registers it for cleanup. Adds two tests:test_temp_dir_removed_at_exit: runs the helper in a subprocess and asserts the directory is gone once that process exits. This is the regression test for the leak; it fails without the change.test_caller_owned_dir_kept: asserts a caller-supplieddiris not removed, guarding the other direction.Verification
Counting entries left in the system temp directory by
test_nifti_rw.py,test_make_nifti.py,test_invert.pyandtest_splitdimd.py:The leaked directories were a mix of empty ones (callers that removed the file but not the directory) and populated ones (callers that removed nothing). Both are gone.
Tests over every file that calls the helper (
tests/data/test_make_nifti.py,tests/data/test_nifti_rw.py,tests/data/utils/test_decollate.py,tests/transforms/inverse/,tests/transforms/test_inverse_collation.py,tests/transforms/utility/test_splitdimd.py):The two failures are
test_nifti_rw.py::test_write_2dand::test_write_3d, which fail identically on an unmodifieddevcheckout in this environment (aTypeError: '>=' not supported between instances of 'NoneType' and 'int'and a shape mismatch, a nibabel version issue). They are unrelated to this change and are left alone.tests/integration/test_integration_sliding_window.pywas also checked separately for the subprocess path: it leaves nothing behind, and its one failure likewise predates this change.Notes
Cleanup happens at interpreter exit, so directories still accumulate within a single long-running process and a hard-killed process (
SIGKILL,os._exit) still leaks. What changes is that a normally-terminating test run no longer leaves anything behind, and a new caller cannot reintroduce the leak by forgetting to clean up.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.