test: clean up simulated-acquisition output instead of littering /tmp - #640
Open
Alpaca233 wants to merge 1 commit into
Open
test: clean up simulated-acquisition output instead of littering /tmp#640Alpaca233 wants to merge 1 commit into
Alpaca233 wants to merge 1 commit into
Conversation
The stub multipoint controllers pointed simulated acquisitions at a literal "/tmp/", and each run left ~180 MB of image data there forever - accumulated runs filled an entire disk on 2026-09-05. The stub factories now write under a mkdtemp dir registered in test_stubs; cleanup_leaked_hardware deletes the registered dirs at test teardown, after every writer is closed, and the watchdog breadcrumb dir is removed at session finish. A full suite run now leaves /tmp empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current cleanup helper can silently fail to delete output trees (and still deregister them), undermining the PR’s primary goal of preventing disk growth.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prevents simulated-acquisition tests from accumulating large (~180 MB/run) image output under a hard-coded /tmp/ path by routing stub controllers to per-test temp directories and cleaning those directories during the existing autouse teardown.
Changes:
- Add temp-dir creation + registration helpers for stub acquisition base paths and a cleanup routine.
- Update stub multipoint controller factories (including Qt) to use registered temp dirs instead of
"/tmp/". - Extend
cleanup_leaked_hardwareteardown to delete registered acquisition output dirs, and remove watchdog breadcrumb dirs at session end; add tests covering registration and cleanup.
File summaries
| File | Description |
|---|---|
| software/tests/control/test_stubs.py | Introduces registered temp acquisition base paths and a cleanup helper; updates stub controller base path usage. |
| software/tests/control/test_stub_acquisition_cleanup.py | Adds tests asserting stub controllers write under a registered temp dir and that cleanup removes it. |
| software/tests/control/gui_test_stubs.py | Updates Qt stub multipoint controller to use the registered temp acquisition base path. |
| software/tests/conftest.py | Hooks cleanup of registered acquisition output dirs into the autouse teardown; removes watchdog breadcrumb dir at session finish. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+29
to
33
| def cleanup_stub_acquisition_dirs(): | ||
| while _acquisition_output_dirs: | ||
| shutil.rmtree(_acquisition_output_dirs.pop(), ignore_errors=True) | ||
|
|
||
|
|
Comment on lines
+28
to
+33
| scope = control.microscope.Microscope.build_from_global_config(True) | ||
| mpc = ts.get_test_multi_point_controller(microscope=scope) | ||
| base_path = mpc.base_path | ||
|
|
||
| ts.cleanup_stub_acquisition_dirs() | ||
|
|
| # stub controller writes into is registered here so the autouse fixture in | ||
| # tests/conftest.py can delete it at test teardown - pointing them at a literal | ||
| # "/tmp/" used to accumulate output across runs until the disk filled. | ||
| _acquisition_output_dirs: list = [] |
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.
Problem
get_test_multi_point_controllerandget_test_qt_multi_point_controllerpoint simulated acquisitions at a literal"/tmp/". Every test that runs an acquisition leaves ~180 MB of simulated image data in/tmp/unit_test_experiment_*forever. Accumulated runs filled an entire 782 GB disk on a dev machine on 2026-09-05 (38 GB of test output going back days).Fix
tempfile.mkdtemp(prefix="squid_unit_test_acquisition_")and register it intest_stubs._acquisition_output_dirs.cleanup_leaked_hardware(the existing autouse teardown fixture intests/conftest.py) deletes the registered dirs at the end of each test — after controllers/microscopes are closed, so no writer is alive when the tree is removed. Peak disk usage is now one test's output instead of an unbounded accumulation.squid-test-watchdog-cleanup-<pid>, written during leaked-acquisition cleanup, never read) is removed at session finish.Testing
tests/control/test_stub_acquisition_cleanup.pycovers the registration and the cleanup./tmpcontains nounit_test_experiment_*,squid_unit_test_acquisition_*, or watchdog-cleanup leftovers afterwards (verified in the same shell).🤖 Generated with Claude Code