test(hooks): isolate test_hooks_cli from test_cli side effect (#1510) - #1547
Conversation
Nine save/log/precompact tests in test_hooks_cli.py passed only because test_cli.py (alphabetically earlier) created ~/.mempalace in the session tmp HOME as a side effect, satisfying the _palace_root_exists() kill-switch. Run in isolation they short-circuited and failed (9 failed, 80 passed, 1 skipped). Add a module autouse fixture that points PALACE_ROOT/STATE_DIR at a per-test palace root that exists, so every test is robust standalone and future tests don't inherit the trap. Kill-switch tests that need the absent path call _redirect_palace_root after the fixture; monkeypatch last-write-wins keeps their absent/file root and teardown restores the real module value. Isolation: pytest tests/test_hooks_cli.py -> 100 passed, 1 skipped. Ordering preserved: test_cli + test_hooks_cli -> 165 passed. Closes #1510
There was a problem hiding this comment.
Pull request overview
Adds a module-level autouse fixture in tests/test_hooks_cli.py that points PALACE_ROOT/STATE_DIR at a per-test existing directory, eliminating the hidden ordering dependency on test_cli.py creating ~/.mempalace (issue #1510).
Changes:
- New
_isolated_existing_palace_rootautouse fixture setsPALACE_ROOT,STATE_DIR, and resets_state_dir_initialized. - Relies on
monkeypatchlast-write-wins so absent-root kill-switch tests calling_redirect_palace_rootafter the fixture still work.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request introduces an autouse pytest fixture in tests/test_hooks_cli.py to isolate the PALACE_ROOT and STATE_DIR environments, preventing tests from relying on or affecting the user's local filesystem. The reviewer identified that _MINE_PID_DIR also needs to be patched to avoid filesystem leaks and recommended explicitly creating the state directory to match the fixture's documentation.
| root = tmp_path / ".mempalace" | ||
| root.mkdir(exist_ok=True) | ||
| monkeypatch.setattr(hooks_cli_mod, "PALACE_ROOT", root) | ||
| monkeypatch.setattr(hooks_cli_mod, "STATE_DIR", root / "hook_state") | ||
| monkeypatch.setattr(hooks_cli_mod, "_state_dir_initialized", False) |
There was a problem hiding this comment.
The fixture patches PALACE_ROOT and STATE_DIR, but it misses _MINE_PID_DIR. Since _MINE_PID_DIR is initialized at module import time (line 277 of hooks_cli.py), it retains its original value pointing to the real ~/.mempalace even after STATE_DIR is patched. This causes tests that spawn mine processes (like _spawn_mine or _ingest_transcript) to create PID files in the real user directory, leading to filesystem leaks and potential conflicts with a running instance.
Additionally, the docstring promises an existing STATE_DIR, but the code only creates the PALACE_ROOT. I've updated the suggestion to create both and ensure _MINE_PID_DIR is also isolated.
| root = tmp_path / ".mempalace" | |
| root.mkdir(exist_ok=True) | |
| monkeypatch.setattr(hooks_cli_mod, "PALACE_ROOT", root) | |
| monkeypatch.setattr(hooks_cli_mod, "STATE_DIR", root / "hook_state") | |
| monkeypatch.setattr(hooks_cli_mod, "_state_dir_initialized", False) | |
| root = tmp_path / ".mempalace" | |
| state_dir = root / "hook_state" | |
| state_dir.mkdir(parents=True, exist_ok=True) | |
| monkeypatch.setattr(hooks_cli_mod, "PALACE_ROOT", root) | |
| monkeypatch.setattr(hooks_cli_mod, "STATE_DIR", state_dir) | |
| monkeypatch.setattr(hooks_cli_mod, "_MINE_PID_DIR", state_dir / "mine_pids") | |
| monkeypatch.setattr(hooks_cli_mod, "_state_dir_initialized", False) |
Review feedback: _MINE_PID_DIR is derived from STATE_DIR at module import (hooks_cli.py:277), so patching STATE_DIR alone left mine-spawning tests writing PID files under the import-time location instead of the per-test root. Patch _MINE_PID_DIR too, and create the state dir so the fixture's 'existing' docstring is accurate. Isolation still 100 passed/1 skipped; ordering still 165 passed.
…ROOT The MemPalace#1547 isolation fixture (autouse) now creates tmp_path/.mempalace and sets PALACE_ROOT, so the manual setup in the MEMPALACE_WING override test double-created the dir and raised FileExistsError after the develop merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ROOT The MemPalace#1547 isolation fixture (autouse) now creates tmp_path/.mempalace and sets PALACE_ROOT, so the manual setup in the MEMPALACE_WING override test double-created the dir and raised FileExistsError after the develop merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ROOT The MemPalace#1547 isolation fixture (autouse) now creates tmp_path/.mempalace and sets PALACE_ROOT, so the manual setup in the MEMPALACE_WING override test double-created the dir and raised FileExistsError after the develop merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ROOT The MemPalace#1547 isolation fixture (autouse) now creates tmp_path/.mempalace and sets PALACE_ROOT, so the manual setup in the MEMPALACE_WING override test double-created the dir and raised FileExistsError after the develop merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ROOT The MemPalace#1547 isolation fixture (autouse) now creates tmp_path/.mempalace and sets PALACE_ROOT, so the manual setup in the MEMPALACE_WING override test double-created the dir and raised FileExistsError after the develop merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ROOT The MemPalace#1547 isolation fixture (autouse) now creates tmp_path/.mempalace and sets PALACE_ROOT, so the manual setup in the MEMPALACE_WING override test double-created the dir and raised FileExistsError after the develop merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Problem
Nine save/log/precompact tests in
test_hooks_cli.pyonly pass in the full suite becausetest_cli.py(alphabetically earlier) creates~/.mempalacein the session tmp HOME as a side effect, satisfying the_palace_root_exists()kill-switch. Run in isolation (pytest tests/test_hooks_cli.py) they short-circuit: 9 failed, 80 passed, 1 skipped.Fix
Module-scoped
autousefixture that pointsPALACE_ROOT/STATE_DIRat a per-test palace root that exists, so every test is robust standalone and future tests don't inherit the same trap. Tests that exercise the absent-root kill-switch call_redirect_palace_root(or set their own root) after the fixture;monkeypatch's last-write-wins keeps their absent/file root and teardown still restores the real module value. Chose this over decorating the 9 tests (fragile — new tests re-trap) and over mkdir-ing real~/.mempalace(the coupling the issue warned against).Verification
pytest tests/test_hooks_cli.py→ 100 passed, 1 skipped (was 9 failed).pytest tests/test_cli.py tests/test_hooks_cli.py→ 165 passed, 1 skipped.Closes #1510