prax: make flaky test_deletes_old_system_log deterministic (was blocking green CI on PR659). Root cause = dual module identity: test_logging_handlers.py sys.modules.pop+reimports log_watchdog, so test_sweep's string-path patch of _get_system_logs_dir could hit a different object than the function __globals__ -> sweep scanned real (empty) system_logs -> files_removed=0 -> intermittent assert 0==1 (same commit passed one CI run, failed another). Fix: direct 'import log_watchdog as lw' + patch.object(lw,...) (shared __dict__) + patch json_handler to block real IO. Test-only (1 file). Verified: prax 978 green, interacting pair 5x deterministic, @prax full-repo 2x 11019 pass.
This commit is contained in:
AIOSAI
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user