diff --git a/tests/conftest.py b/tests/conftest.py index 5fcf02113..97e249b41 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -159,3 +159,43 @@ def _serve_test_static(): os.environ["ODYSSEUS_TEST_STATIC_ORIGIN"] = previous_origin server.shutdown() server.server_close() + + +@pytest.fixture(autouse=True) +def _no_leaked_module_stubs(): + """Fail the test that leaves a bare ``src.*``/``core.*`` stub behind. + + Several test modules install empty stand-in modules so an import-heavy + production module can be loaded under the mocks above. When one of those + writes is not undone, the stub stays in ``sys.modules`` for the rest of the + session and every later test that imports the real module silently gets an + empty one instead. The suite still passes as a whole, because the victims + usually run before the leak; it only breaks under a different collection + order, which is why this class of bug reaches CI green. + + This fixture is declared in the root conftest, so it is set up before any + test-module fixture and torn down after all of them — a stub that a test's + own teardown removes is not reported. The leaked entries are dropped here + as well as reported, so the failure stays attributed to the test that + introduced it instead of cascading into the rest of the run. + + Bare stubs present before the test starts are ignored: this guards against + new leaks, it does not police import state the session began with. + """ + from tests.helpers.import_state import bare_module_stubs, clear_module + + before = bare_module_stubs() + yield + leaked = sorted(bare_module_stubs() - before) + if not leaked: + return + for name in leaked: + clear_module(name) + pytest.fail( + "test left bare module stub(s) in sys.modules: " + + ", ".join(leaked) + + ". Register the stub through monkeypatch.setitem(sys.modules, ...) " + "or tests.helpers.import_state.preserve_import_state so it is undone " + "at teardown.", + pytrace=False, + ) diff --git a/tests/helpers/import_state.py b/tests/helpers/import_state.py index 0eea62d9d..f58c86c37 100644 --- a/tests/helpers/import_state.py +++ b/tests/helpers/import_state.py @@ -31,6 +31,7 @@ safe for callers that pass both a parent package and a child module. """ import sys +import types from contextlib import contextmanager _ABSENT = object() @@ -167,3 +168,33 @@ def preserve_import_state(*module_names): # Phase 2: restore all parent-package attributes. for name, (_, saved_attr) in saved.items(): _restore_parent_attr(name, saved_attr) + + +# Names under these prefixes are the ones a leaked stub actually breaks: a +# later test doing ``import src.x`` or ``import core.x`` silently gets the +# empty stub instead of the real module. +_GUARDED_PREFIXES = ("src.", "core.") + + +def bare_module_stubs(): + """Return the ``src.*``/``core.*`` names currently bound to a bare stub. + + A bare stub is a plain :class:`types.ModuleType` with no on-disk + ``__file__`` — the object ``types.ModuleType(name)`` produces. That is the + same "is this a fake?" test the ``clear_fake_*`` helpers above use, so a + module imported from disk is never reported. + + ``MagicMock`` stand-ins are deliberately out of scope: they answer every + attribute, so they fail loudly at use rather than silently, and several + test modules install them on purpose. + """ + found = set() + for name, mod in list(sys.modules.items()): + if not name.startswith(_GUARDED_PREFIXES): + continue + if type(mod) is not types.ModuleType: + continue + if getattr(mod, "__file__", None): + continue + found.add(name) + return found diff --git a/tests/test_auth_regressions.py b/tests/test_auth_regressions.py index 62b479748..b7403f4ca 100644 --- a/tests/test_auth_regressions.py +++ b/tests/test_auth_regressions.py @@ -21,7 +21,7 @@ from unittest.mock import MagicMock # (Same trick as test_null_owner_gates.py — the real modules instantiate # SQLAlchemy declarative classes at import-time which blow up under the # conftest's `sqlalchemy.*` MagicMock stubs.) -def _ensure_stub(name: str, **attrs): +def _ensure_stub(monkeypatch, name: str, **attrs): """Create or augment a stub module with the given attributes. Augments existing entries because earlier-run tests may have already stubbed the same module with a different attribute set. @@ -48,7 +48,7 @@ def _ensure_stub(name: str, **attrs): *parent_name.split("."), ) parent.__path__ = [real_path] if os.path.isdir(real_path) else [] - sys.modules[parent_name] = parent + monkeypatch.setitem(sys.modules, parent_name, parent) else: parent = sys.modules[parent_name] else: @@ -58,17 +58,17 @@ def _ensure_stub(name: str, **attrs): mod = sys.modules.get(name) if mod is None: mod = types.ModuleType(name) - sys.modules[name] = mod + monkeypatch.setitem(sys.modules, name, mod) for k, v in attrs.items(): if not hasattr(mod, k): - setattr(mod, k, v) + monkeypatch.setattr(mod, k, v, raising=False) if parent is not None and not hasattr(parent, child_name): - setattr(parent, child_name, mod) + monkeypatch.setattr(parent, child_name, mod, raising=False) return mod @pytest.fixture(autouse=True) def _auth_regressions_stubs(monkeypatch): - db = _ensure_stub("core.database", + db = _ensure_stub(monkeypatch, "core.database", SessionLocal=MagicMock(), ScheduledTask=MagicMock(), TaskRun=MagicMock(), ModelEndpoint=MagicMock(), Session=MagicMock(), ChatMessage=MagicMock(), CalendarCal=MagicMock(), CalendarEvent=MagicMock(), @@ -76,17 +76,18 @@ def _auth_regressions_stubs(monkeypatch): GalleryImage=MagicMock(), GalleryAlbum=MagicMock(), Note=MagicMock(), McpServer=MagicMock(), ) - auth = _ensure_stub("core.auth", AuthManager=MagicMock()) - ep = _ensure_stub("src.endpoint_resolver", + auth = _ensure_stub(monkeypatch, "core.auth", AuthManager=MagicMock()) + ep = _ensure_stub(monkeypatch, "src.endpoint_resolver", resolve_endpoint=MagicMock(return_value=("", "", {})), normalize_base=MagicMock(), build_chat_url=MagicMock(), build_models_url=MagicMock(), build_headers=MagicMock(), ) - monkeypatch.setitem(sys.modules, "core.database", db) - monkeypatch.setitem(sys.modules, "core.auth", auth) - monkeypatch.setitem(sys.modules, "src.endpoint_resolver", ep) + # _ensure_stub now registers each stub through monkeypatch itself, so the + # whole set is undone at teardown. Re-setting them here would capture the + # stub as the restore target and leave it behind for the rest of the run. + assert db and auth and ep from fastapi import HTTPException @@ -293,7 +294,7 @@ def test_research_spinoff_rejects_wrong_owner(): # pop_notifications owner filter # --------------------------------------------------------------------------- -def test_pop_notifications_owner_filtered(): +def test_pop_notifications_owner_filtered(monkeypatch): """pop_notifications(owner='alice') must return only alice's items. bob's and legacy ownerless items stay behind in the queue.""" # Build a minimal scheduler instance that we can hit directly. @@ -302,11 +303,15 @@ def test_pop_notifications_owner_filtered(): import sys, types from unittest.mock import MagicMock as _MM # `task_scheduler` pulls in lots of helpers — stub the ones it uses. + # monkeypatch.setitem, not a bare assignment: a plain write leaves these + # empty stubs in sys.modules for the rest of the session, and every later + # test that imports a real name from one of them fails with + # "cannot import name ... (unknown location)". The stubs above in this file + # already use monkeypatch for the same reason. for s in ["src.builtin_actions", "src.ai_interaction", "src.endpoint_resolver", "src.agent_loop", "src.session_manager"]: if s not in sys.modules: - mod = types.ModuleType(s) - sys.modules[s] = mod + monkeypatch.setitem(sys.modules, s, types.ModuleType(s)) from src.task_scheduler import TaskScheduler sch = TaskScheduler.__new__(TaskScheduler) # bypass __init__ network etc. sch._pending_notifications = [] diff --git a/tests/test_document_rich_color_reset_and_contrast.py b/tests/test_document_rich_color_reset_and_contrast.py index 64c237a7b..c74f3576b 100644 --- a/tests/test_document_rich_color_reset_and_contrast.py +++ b/tests/test_document_rich_color_reset_and_contrast.py @@ -20,6 +20,12 @@ def test_color_controls_have_theme_reset_and_split_palettes(): def test_rich_colors_follow_theme_and_undo_as_one_edit(): + # Two input conventions in here are platform-sensitive and must stay that + # way. Palette entries are opened with a plain click: on macOS a + # Control+click is delivered as `contextmenu`, so the menu item's `click` + # handler never runs and nothing is applied. Undo uses Playwright's + # `ControlOrMeta` alias because the editor's undo accelerator is Cmd+Z on + # macOS and Ctrl+Z everywhere else. script = r""" import { chromium } from 'playwright'; const browser = await chromium.launch({ headless: true }); @@ -58,24 +64,24 @@ def test_rich_colors_follow_theme_and_undo_as_one_edit(): labels: [...document.querySelectorAll('.rich-color-palette-label')].map(item => item.textContent), reset: document.querySelector('.rich-color-reset')?.textContent.trim(), })); - await page.locator('#doc-md-dd-menu .doc-overflow-item').filter({ hasText: 'Lemon' }).click({ modifiers: ['Control'] }); + await page.locator('#doc-md-dd-menu .doc-overflow-item').filter({ hasText: 'Lemon' }).click(); const highlighted = await page.locator('#doc-email-richbody p').nth(0).locator('span').evaluate(span => ({ color: getComputedStyle(span).color, background: getComputedStyle(span).backgroundColor, })); - await page.locator('#doc-email-richbody').press('Control+z'); + await page.locator('#doc-email-richbody').press('ControlOrMeta+z'); const highlightUndone = await page.locator('#doc-email-richbody p').nth(0).innerHTML(); await selectParagraph(1); await openMenu('color'); - await page.locator('.rich-color-reset').click({ modifiers: ['Control'] }); + await page.locator('.rich-color-reset').click(); const defaultColor = await page.locator('#doc-email-richbody p').nth(1).locator('span').evaluate(span => ({ style: span.getAttribute('style'), color: getComputedStyle(span).color, })); await page.evaluate(() => document.documentElement.style.setProperty('--fg', '#88cc44')); const changedThemeColor = await page.locator('#doc-email-richbody p').nth(1).locator('span').evaluate(span => getComputedStyle(span).color); - await page.locator('#doc-email-richbody').press('Control+z'); + await page.locator('#doc-email-richbody').press('ControlOrMeta+z'); const colorUndone = await page.locator('#doc-email-richbody p').nth(1).innerHTML(); console.log(JSON.stringify({ palette, highlighted, highlightUndone, defaultColor, changedThemeColor, colorUndone })); await browser.close(); diff --git a/tests/test_inspect_media_tool.py b/tests/test_inspect_media_tool.py index f6ebc64ae..b00a41baf 100644 --- a/tests/test_inspect_media_tool.py +++ b/tests/test_inspect_media_tool.py @@ -1,5 +1,6 @@ import asyncio import base64 +import functools import io import json from pathlib import Path @@ -20,6 +21,28 @@ from src.tool_execution import _active_workspace from src.tool_schemas import FUNCTION_TOOL_SCHEMAS +@functools.lru_cache(maxsize=None) +def _ffmpeg_has_encoder(name: str) -> bool: + """Whether the ffmpeg on PATH was built with the named encoder. + + Codec support is a build option, not something the project requires. The + Homebrew ffmpeg on macOS ships without libwebp, for instance, so a test + that asserts a successful `.webp` export there fails on the build rather + than on the tool. + """ + if not shutil.which("ffmpeg"): + return False + listed = subprocess.run( + ["ffmpeg", "-hide_banner", "-loglevel", "error", "-encoders"], + check=False, capture_output=True, text=True, + ) + return any( + line.split()[1:2] == [name] or f"(codec {name})" in line + for line in listed.stdout.splitlines() + if line.strip() + ) + + def test_media_timestamp_parser_accepts_units_and_four_field_timecodes(): assert _parse_seconds("0m", default=-1) == 0 assert _parse_seconds("30m", default=-1) == 1800 @@ -491,13 +514,16 @@ def test_inspect_media_exports_final_decodable_frame_at_exact_duration(tmp_path: result = asyncio.run(InspectMediaTool().execute(json.dumps({ "path": "/workspace/video.mp4", "timestamp": "end", - "output_path": "/workspace/final.webp", + # PNG, not WebP: this asserts that the *final* frame is decodable at + # the exact duration, so it must not also depend on an optional + # ffmpeg encoder. WebP export is covered separately below. + "output_path": "/workspace/final.png", }), {})) finally: _active_workspace.reset(token) assert result["exit_code"] == 0, result - assert (tmp_path / "final.webp").stat().st_size > 0 + assert (tmp_path / "final.png").stat().st_size > 0 token = _active_workspace.set(str(tmp_path)) try: @@ -514,6 +540,67 @@ def test_inspect_media_exports_final_decodable_frame_at_exact_duration(tmp_path: assert Image.open(io.BytesIO(base64.b64decode(high_detail["images"][0]["data"]))).size == (768, 432) +@pytest.mark.skipif(not shutil.which("ffmpeg") or not shutil.which("ffprobe"), reason="ffmpeg required") +@pytest.mark.skipif(not _ffmpeg_has_encoder("webp"), reason="ffmpeg built without a webp encoder") +def test_inspect_media_exports_a_webp_still(tmp_path: Path): + """A `.webp` output_path is passed straight through to ffmpeg. + + Guarded on the encoder rather than asserted unconditionally: WebP is a + build option (Homebrew's macOS ffmpeg omits it) and the project does not + require it. When the encoder is missing the tool reports ffmpeg's failure + with `exit_code` 1, which is covered by + `test_inspect_media_reports_a_missing_encoder_instead_of_crashing`. + """ + video = tmp_path / "video.mp4" + subprocess.run([ + "ffmpeg", "-hide_banner", "-loglevel", "error", "-f", "lavfi", + "-i", "testsrc2=size=320x180:rate=4:duration=2", "-pix_fmt", "yuv420p", + "-y", str(video), + ], check=True) + token = _active_workspace.set(str(tmp_path)) + try: + result = asyncio.run(InspectMediaTool().execute(json.dumps({ + "path": "/workspace/video.mp4", + "timestamp": "end", + "output_path": "/workspace/final.webp", + }), {})) + finally: + _active_workspace.reset(token) + + assert result["exit_code"] == 0, result + assert Image.open(tmp_path / "final.webp").format == "WEBP" + + +@pytest.mark.skipif(not shutil.which("ffmpeg") or not shutil.which("ffprobe"), reason="ffmpeg required") +@pytest.mark.skipif(_ffmpeg_has_encoder("webp"), reason="needs an ffmpeg built without webp") +def test_inspect_media_reports_a_missing_encoder_instead_of_crashing(tmp_path: Path): + """An export in a format this ffmpeg cannot encode fails as a tool error. + + The tool does not probe the encoder list, so the only contract it can keep + is to surface ffmpeg's own failure rather than raise or write a truncated + file. Asserted only on builds that actually lack the encoder. + """ + video = tmp_path / "video.mp4" + subprocess.run([ + "ffmpeg", "-hide_banner", "-loglevel", "error", "-f", "lavfi", + "-i", "testsrc2=size=320x180:rate=4:duration=2", "-pix_fmt", "yuv420p", + "-y", str(video), + ], check=True) + token = _active_workspace.set(str(tmp_path)) + try: + result = asyncio.run(InspectMediaTool().execute(json.dumps({ + "path": "/workspace/video.mp4", + "timestamp": "end", + "output_path": "/workspace/final.webp", + }), {})) + finally: + _active_workspace.reset(token) + + assert result["exit_code"] == 1 + assert "ffmpeg still extraction failed" in result["error"] + assert not (tmp_path / "final.webp").exists() + + @pytest.mark.skipif(not shutil.which("ffmpeg") or not shutil.which("ffprobe"), reason="ffmpeg required") def test_inspect_media_rejects_ambiguous_multi_frame_single_image_export(tmp_path: Path): video = tmp_path / "video.mp4" diff --git a/tests/test_scheduler_restart_doublefire.py b/tests/test_scheduler_restart_doublefire.py index 9f0c87372..ca90c55bc 100644 --- a/tests/test_scheduler_restart_doublefire.py +++ b/tests/test_scheduler_restart_doublefire.py @@ -21,12 +21,21 @@ def _test_utcnow(): return datetime.now(timezone.utc).replace(tzinfo=None) -def _stub_heavy(): +def _stub_heavy(monkeypatch): + """Stub the heavy modules ``task_scheduler`` imports, for this test only. + + Registered through ``monkeypatch.setitem`` so every entry is removed at + teardown. A bare ``sys.modules[name] = ...`` leaves an empty module behind + for the rest of the session, and any later test that imports the real one + silently gets the stub instead - a failure that only shows up under a + different collection order. + """ for name in [ "src.builtin_actions", "src.ai_interaction", "src.endpoint_resolver", "src.agent_loop", "src.session_manager", ]: - sys.modules.setdefault(name, types.ModuleType(name)) + if name not in sys.modules: + monkeypatch.setitem(sys.modules, name, types.ModuleType(name)) def _setup_isolated_db(): @@ -74,7 +83,7 @@ def test_scheduler_utcnow_preserves_naive_utc_contract(): def _drive_scheduler(monkeypatch, pre_start_setup=None): """Build a TaskScheduler bypassing __init__ and run start() + two polls.""" - _stub_heavy() + _stub_heavy(monkeypatch) cd, ScheduledTask, TaskRun = _setup_isolated_db() from src.task_scheduler import TaskScheduler