diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e8817e5..d025c0e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,19 @@ PyPI version — not the changelog header. ### Fixed +- **Flaky `test_deletes_old_system_log` made deterministic (@prax log-sweep tests).** + The sweep integration test reached `log_watchdog._get_system_logs_dir` through a + `_get_sweep()` wrapper and patched it by string path; a sibling test + (`test_logging_handlers.py`) `sys.modules.pop`s and reimports `log_watchdog`, + creating a second module object — so the string patch could target a different + object than the function's `__globals__`, the sweep scanned the real (empty) + `system_logs/`, removed 0 files, and `assert files_removed == 1` failed + intermittently (the same commit passed in one CI run and failed in another). + Switched to a direct `import log_watchdog as lw` + `patch.object(lw, …)` (shared + module `__dict__`) and patched `json_handler` to block real file I/O. Test-only + (1 file); prax suite 978 green, two full-repo runs 11,019 passed each, sweep tests + deterministic across repeated runs. + - **Telegram skill tests made CI-safe — full-repo collection + hermeticity (issue #691).** The 16 test files under `skills/lib/telegram/tests/` imported handlers via bare `from apps.handlers…`, which collided with other branches' `apps` packages during diff --git a/src/aipass/prax/tests/test_sweep.py b/src/aipass/prax/tests/test_sweep.py index 5167f48a..15081983 100644 --- a/src/aipass/prax/tests/test_sweep.py +++ b/src/aipass/prax/tests/test_sweep.py @@ -1,9 +1,9 @@ # =================== AIPass ==================== # Name: test_sweep.py # Description: Tests for stale log sweep in log_audit -# Version: 1.0.0 +# Version: 1.1.0 # Created: 2026-07-10 -# Modified: 2026-07-10 +# Modified: 2026-07-11 # ============================================= """ @@ -11,7 +11,6 @@ Tests for sweep_stale_logs — the 30-day stale log cleanup policy. Verifies: age-based deletion, pattern matching (.log, .jsonl, .1 siblings), directory scanning across system_logs/ and branch logs/. -All imports go through the module layer (log_audit). """ import os @@ -29,13 +28,17 @@ def _make_old_file(path: Path, age_days: int) -> None: os.utime(path, (old_time, old_time)) -def _get_sweep(): - """Import sweep_stale_logs from the module layer after conftest mocks are active.""" - mod_name = "aipass.prax.apps.modules.log_audit" - sys.modules.pop(mod_name, None) - from aipass.prax.apps.modules.log_audit import sweep_stale_logs +def _get_lw(): + """Get the current log_watchdog module from sys.modules (or import it). - return sweep_stale_logs + Returns the module object directly — callers use patch.object(lw, ...) + so patch and function always share the same __globals__. This avoids + the module-identity split that made the old _get_sweep() wrapper flaky + when other tests pop and reimport log_watchdog. + """ + import aipass.prax.apps.handlers.logging.log_watchdog as lw + + return lw def _ensure_watchdog_mock(monkeypatch): @@ -64,30 +67,25 @@ class TestSweepIntegration: def test_deletes_old_system_log(self, tmp_path): """Verify sweep deletes old files from system_logs/.""" - sweep_stale_logs = _get_sweep() + lw = _get_lw() sys_logs = tmp_path / "system_logs" sys_logs.mkdir() _make_old_file(sys_logs / "old_module.log", 45) with ( - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_system_logs_dir", - return_value=sys_logs, - ), - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_ecosystem_root", - return_value=tmp_path / "src" / "aipass", - ), + patch.object(lw, "_get_system_logs_dir", return_value=sys_logs), + patch.object(lw, "_get_ecosystem_root", return_value=tmp_path / "src" / "aipass"), + patch.object(lw, "json_handler", MagicMock()), ): - result = sweep_stale_logs() + result = lw.sweep_stale_logs() assert result["files_removed"] == 1 assert not (sys_logs / "old_module.log").exists() def test_deletes_old_branch_jsonl(self, tmp_path): """Verify sweep deletes old .jsonl files from branch logs/.""" - sweep_stale_logs = _get_sweep() + lw = _get_lw() eco = tmp_path / "src" / "aipass" branch_logs = eco / "testbranch" / "logs" @@ -95,23 +93,18 @@ class TestSweepIntegration: _make_old_file(branch_logs / "ops.jsonl", 35) with ( - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_system_logs_dir", - return_value=tmp_path / "system_logs", - ), - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_ecosystem_root", - return_value=eco, - ), + patch.object(lw, "_get_system_logs_dir", return_value=tmp_path / "system_logs"), + patch.object(lw, "_get_ecosystem_root", return_value=eco), + patch.object(lw, "json_handler", MagicMock()), ): - result = sweep_stale_logs() + result = lw.sweep_stale_logs() assert result["files_removed"] == 1 assert not (branch_logs / "ops.jsonl").exists() def test_keeps_fresh_files(self, tmp_path): """Verify sweep leaves files younger than 30 days untouched.""" - sweep_stale_logs = _get_sweep() + lw = _get_lw() sys_logs = tmp_path / "system_logs" sys_logs.mkdir() @@ -119,23 +112,18 @@ class TestSweepIntegration: fresh.write_text("fresh content\n") with ( - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_system_logs_dir", - return_value=sys_logs, - ), - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_ecosystem_root", - return_value=tmp_path / "src" / "aipass", - ), + patch.object(lw, "_get_system_logs_dir", return_value=sys_logs), + patch.object(lw, "_get_ecosystem_root", return_value=tmp_path / "src" / "aipass"), + patch.object(lw, "json_handler", MagicMock()), ): - result = sweep_stale_logs() + result = lw.sweep_stale_logs() assert result["files_removed"] == 0 assert fresh.exists() def test_deletes_rotation_siblings(self, tmp_path): """Verify sweep also removes stale .log.1 rotation backups.""" - sweep_stale_logs = _get_sweep() + lw = _get_lw() sys_logs = tmp_path / "system_logs" sys_logs.mkdir() @@ -143,16 +131,11 @@ class TestSweepIntegration: _make_old_file(sys_logs / "module.log.1", 40) with ( - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_system_logs_dir", - return_value=sys_logs, - ), - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_ecosystem_root", - return_value=tmp_path / "src" / "aipass", - ), + patch.object(lw, "_get_system_logs_dir", return_value=sys_logs), + patch.object(lw, "_get_ecosystem_root", return_value=tmp_path / "src" / "aipass"), + patch.object(lw, "json_handler", MagicMock()), ): - result = sweep_stale_logs() + result = lw.sweep_stale_logs() assert result["files_removed"] == 2 assert not (sys_logs / "module.log").exists() @@ -160,23 +143,18 @@ class TestSweepIntegration: def test_returns_structured_summary(self, tmp_path): """Verify sweep returns summary with counts and reclaimed size.""" - sweep_stale_logs = _get_sweep() + lw = _get_lw() sys_logs = tmp_path / "system_logs" sys_logs.mkdir() _make_old_file(sys_logs / "stale.log", 60) with ( - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_system_logs_dir", - return_value=sys_logs, - ), - patch( - "aipass.prax.apps.handlers.logging.log_watchdog._get_ecosystem_root", - return_value=tmp_path / "src" / "aipass", - ), + patch.object(lw, "_get_system_logs_dir", return_value=sys_logs), + patch.object(lw, "_get_ecosystem_root", return_value=tmp_path / "src" / "aipass"), + patch.object(lw, "json_handler", MagicMock()), ): - result = sweep_stale_logs() + result = lw.sweep_stale_logs() assert result["max_age_days"] == 30 assert result["files_removed"] == 1