From 992ff7ca7ceadc329c99dc90ac1d24dbfb77abff Mon Sep 17 00:00:00 2001 From: oskar Date: Thu, 25 Jun 2026 14:48:03 +0200 Subject: [PATCH] =?UTF-8?q?test(control-plane):=20isolate=20=5Fmake=5Fobse?= =?UTF-8?q?rver=5Fsimple=20module-state=20leak=20=E2=80=94=20fixes=20flaky?= =?UTF-8?q?=20test=5Fincident=5Flifecycle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test_run_once_* cases were flaky/order-dependent. Root cause: observer.observer derives OBSERVER_STATE_FILE from STATE_DIR at import time. The helper patched STATE_DIR but never OBSERVER_STATE_FILE, so run_once()/_save_checkpoint() wrote the checkpoint to the real /opt/homelab/state/observer_checkpoint.json. Those node_checkpoints (tmp paths tagged with a pytest run number) leaked across tests and across pytest runs; run_once's `file_path > checkpoint` string compare then skipped/kept events based on run-number ordering. The helper also never restored the module globals it overwrote. Replace both ad-hoc helpers with an autouse monkeypatch fixture that redirects every observer path — including OBSERVER_STATE_FILE — into the per-test tmp_path and reverts them afterward. Tests no longer touch real disk and are deterministic. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tests/test_incident_lifecycle.py | 90 +++++++------------ 1 file changed, 30 insertions(+), 60 deletions(-) diff --git a/services/control-plane/tests/test_incident_lifecycle.py b/services/control-plane/tests/test_incident_lifecycle.py index 198f215..fb3f41d 100644 --- a/services/control-plane/tests/test_incident_lifecycle.py +++ b/services/control-plane/tests/test_incident_lifecycle.py @@ -17,8 +17,23 @@ from observer.observer import Observer, _parse_ts, _atomic_write_json # Helpers # --------------------------------------------------------------------------- -def _make_observer(tmp_path: Path) -> Observer: - """Return an Observer with all runtime paths redirected to tmp_path.""" +@pytest.fixture(autouse=True) +def _redirect_observer_paths(tmp_path, monkeypatch): + """Redirect every observer.observer runtime path into a per-test tmp_path. + + Uses monkeypatch so the patches are reverted after each test — crucially + including OBSERVER_STATE_FILE, which observer.observer derives from STATE_DIR + *at import time* (OBSERVER_STATE_FILE = STATE_DIR / "observer_checkpoint.json"). + + The previous _make_observer_simple helper patched STATE_DIR but never + OBSERVER_STATE_FILE, so run_once() / _save_checkpoint() wrote the checkpoint to + the real /opt/homelab/state/observer_checkpoint.json. That stale file leaked + node_checkpoints (tmp paths tagged with a pytest run number) across tests AND + across pytest runs; the string comparison `file_path > node_checkpoints[node]` + in run_once then skipped or kept events depending purely on run-number ordering, + making the test_run_once_* cases flaky. Redirecting + restoring every path here + isolates each test from disk and from sibling tests. + """ import observer.observer as obs_mod world = tmp_path / "world" @@ -30,70 +45,25 @@ def _make_observer(tmp_path: Path) -> Observer: for d in (world, state, events, logs, repo / "inventory", repo / "hosts"): d.mkdir(parents=True, exist_ok=True) - # Minimal topology so inventory isn't empty (avoids prune-guard early-return) + # Minimal topology so inventory isn't empty (avoids prune-guard early-return). (repo / "inventory" / "topology.yaml").write_text( "nodes:\n vps:\n roles: [control-plane]\n connectivity: {}\n" ) - original_world = obs_mod.WORLD_DIR - original_state = obs_mod.STATE_DIR - original_events = obs_mod.EVENTS_DIR - original_logs = obs_mod.LOGS_DIR - original_inventory = obs_mod.INVENTORY_TOPOLOGY - original_repo = obs_mod.REPO_ROOT - original_failed_events = obs_mod.FAILED_EVENTS_DIR - - obs_mod.WORLD_DIR = world - obs_mod.STATE_DIR = state - obs_mod.EVENTS_DIR = events - obs_mod.LOGS_DIR = logs - obs_mod.INVENTORY_TOPOLOGY = repo / "inventory" / "topology.yaml" - obs_mod.REPO_ROOT = repo - obs_mod.FAILED_EVENTS_DIR = state / "observer_failed_events" - - obs = Observer() - - # Restore module-level constants (monkeypatching at module level is sufficient - # for the Observer instance which captures paths at construction time via globals) - obs_mod.WORLD_DIR = original_world - obs_mod.STATE_DIR = original_state - obs_mod.EVENTS_DIR = original_events - obs_mod.LOGS_DIR = original_logs - obs_mod.INVENTORY_TOPOLOGY = original_inventory - obs_mod.REPO_ROOT = original_repo - obs_mod.FAILED_EVENTS_DIR = original_failed_events - - return obs + monkeypatch.setattr(obs_mod, "WORLD_DIR", world) + monkeypatch.setattr(obs_mod, "STATE_DIR", state) + monkeypatch.setattr(obs_mod, "EVENTS_DIR", events) + monkeypatch.setattr(obs_mod, "LOGS_DIR", logs) + monkeypatch.setattr(obs_mod, "INVENTORY_TOPOLOGY", repo / "inventory" / "topology.yaml") + monkeypatch.setattr(obs_mod, "REPO_ROOT", repo) + monkeypatch.setattr(obs_mod, "FAILED_EVENTS_DIR", state / "observer_failed_events") + monkeypatch.setattr(obs_mod, "OBSERVER_STATE_FILE", state / "observer_checkpoint.json") -def _make_observer_simple(tmp_path: Path): - """Return an Observer instance and patch its world_state in-place.""" - import observer.observer as obs_mod - - world = tmp_path / "world" - state = tmp_path / "state" - events = tmp_path / "events" - logs = tmp_path / "logs" - repo = tmp_path / "repo" - - for d in (world, state, events, logs, repo / "inventory", repo / "hosts"): - d.mkdir(parents=True, exist_ok=True) - - (repo / "inventory" / "topology.yaml").write_text( - "nodes:\n vps:\n roles: [control-plane]\n connectivity: {}\n" - ) - - # Patch before construction - obs_mod.WORLD_DIR = world - obs_mod.STATE_DIR = state - obs_mod.EVENTS_DIR = events - obs_mod.LOGS_DIR = logs - obs_mod.INVENTORY_TOPOLOGY = repo / "inventory" / "topology.yaml" - obs_mod.REPO_ROOT = repo - obs_mod.FAILED_EVENTS_DIR = state / "observer_failed_events" - - obs = Observer() - return obs +def _make_observer_simple(tmp_path: Path) -> Observer: + """Return an Observer. All runtime paths are redirected (and restored) by the + autouse _redirect_observer_paths fixture, which shares this test's tmp_path.""" + return Observer() # ---------------------------------------------------------------------------