From 7729c183b4a2f70c3b583eee6f93e7149a4081f8 Mon Sep 17 00:00:00 2001 From: MaxFreedomPollard <272618364+MaxFreedomPollard@users.noreply.github.com> Date: Fri, 31 Jul 2026 02:33:56 -0400 Subject: [PATCH] test: restore four silently shadowed definitions and guard against more Python keeps only the last definition of a name in a scope, so a duplicate silently deletes the first. Four had accumulated, and two cost real coverage. tests/agent/test_auxiliary_client.py grew a second _clean_env autouse fixture alongside an NVIDIA feature. Being module-level with the same name, it replaced the original for all 158 tests in the file: the ANTHROPIC_API_KEY, ANTHROPIC_TOKEN, CLAUDE_CODE_OAUTH_TOKEN, OPENAI_MODEL, LLM_MODEL and NOUS_INFERENCE_BASE_URL stripping went away, and so did the _aux_unhealthy_* cache reset between tests. Two tests had already started clearing that cache by hand to work around the leak, one of them with a comment describing the pollution. The NVIDIA keys are folded into the original fixture and the duplicate removed. tests/gateway/test_mattermost.py had two copies of test_progress_send_with_invalid_thread_root_never_falls_back_flat. The surviving copy omitted the recorded 400 "invalid root_id" state, so the case the name describes never ran. Both now run under distinct names. The other two were harmless but hid the pattern: an exact duplicate in test_tts_media_routing.py, and a dead _codex_auth_store in test_credential_pool.py that nothing calls. tests/test_no_shadowed_test_definitions.py walks every test module and fails on any repeated definition in one scope, exempting the property/setter/ register family and throwaway _ callbacks. It fails on main listing exactly these four. --- tests/agent/test_auxiliary_client.py | 11 +- tests/agent/test_credential_pool.py | 17 --- tests/gateway/test_mattermost.py | 4 +- tests/gateway/test_tts_media_routing.py | 23 ---- tests/test_no_shadowed_test_definitions.py | 124 +++++++++++++++++++++ 5 files changed, 127 insertions(+), 52 deletions(-) create mode 100644 tests/test_no_shadowed_test_definitions.py diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index eb930eba788f9..73558302b6617 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -67,6 +67,7 @@ def _clean_env(monkeypatch): "OPENROUTER_API_KEY", "OPENAI_BASE_URL", "OPENAI_API_KEY", "OPENAI_MODEL", "LLM_MODEL", "NOUS_INFERENCE_BASE_URL", "ANTHROPIC_API_KEY", "ANTHROPIC_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN", + "NVIDIA_API_KEY", "NVIDIA_BASE_URL", ): monkeypatch.delenv(key, raising=False) # Module-level unhealthy cache (10-min TTL) leaks between tests; @@ -3493,16 +3494,6 @@ class TestBuildCallKwargsToolDedup: -@pytest.fixture(autouse=True) -def _clean_env(monkeypatch): - """Strip provider env vars so each test starts clean.""" - for key in ( - "OPENROUTER_API_KEY", "OPENAI_BASE_URL", "OPENAI_API_KEY", - "NVIDIA_API_KEY", "NVIDIA_BASE_URL", - ): - monkeypatch.delenv(key, raising=False) - - class TestNvidiaBillingHeaders: """NVIDIA NIM billing-origin headers are scoped to NVIDIA cloud.""" diff --git a/tests/agent/test_credential_pool.py b/tests/agent/test_credential_pool.py index e384b8f8d4317..fa3d2b2111451 100644 --- a/tests/agent/test_credential_pool.py +++ b/tests/agent/test_credential_pool.py @@ -1509,23 +1509,6 @@ class TestLeastUsedStrategy: # ── OpenAI Codex OAuth cross-process sync tests ──────────────────────────── -def _codex_auth_store(access: str, refresh: str) -> dict: - return { - "version": 1, - "active_provider": "openai-codex", - "providers": { - "openai-codex": { - "auth_mode": "chatgpt", - "tokens": { - "access_token": access, - "refresh_token": refresh, - "id_token": "id-" + access, - }, - "last_refresh": "2026-04-28T00:00:00Z", - } - }, - } - diff --git a/tests/gateway/test_mattermost.py b/tests/gateway/test_mattermost.py index c4a5a41205f9f..3166ddea53055 100644 --- a/tests/gateway/test_mattermost.py +++ b/tests/gateway/test_mattermost.py @@ -244,8 +244,8 @@ class TestMattermostSend: @pytest.mark.asyncio - async def test_progress_send_with_invalid_thread_root_never_falls_back_flat(self): - """Tool/status/progress bubbles must stay quiet when the thread is broken.""" + async def test_progress_send_with_broken_thread_and_no_recorded_error_stays_quiet(self): + """Same rule when no post error was recorded: still no flat fallback.""" self.adapter._reply_mode = "thread" self.adapter._api_get = AsyncMock(return_value={"id": "bad_root", "root_id": ""}) self.adapter._api_post = AsyncMock(return_value={}) diff --git a/tests/gateway/test_tts_media_routing.py b/tests/gateway/test_tts_media_routing.py index 990885312205b..50bc6368f16bd 100644 --- a/tests/gateway/test_tts_media_routing.py +++ b/tests/gateway/test_tts_media_routing.py @@ -196,26 +196,3 @@ class _DiscordMediaFailureAdapter(BasePlatformAdapter): async def get_chat_info(self, chat_id): return {"id": chat_id, "type": "dm"} - - -@pytest.mark.asyncio -async def test_non_streaming_media_failure_notifies_user(tmp_path, monkeypatch): - """Attachmentless send_video results must surface a user-visible notice (#66797).""" - adapter = _DiscordMediaFailureAdapter() - event = _event() - media_file = _allowed_media_path(tmp_path, monkeypatch, "clip.mp4") - adapter._message_handler = AsyncMock(return_value=f"MEDIA:{media_file}") - adapter.send_video = AsyncMock( - return_value=SendResult( - success=False, - error="Discord accepted the message but attached no files (clip.mp4)", - ) - ) - adapter.send_document = AsyncMock(return_value=SendResult(success=True, message_id="doc")) - adapter.send_voice = AsyncMock(return_value=SendResult(success=True, message_id="voice")) - adapter.send_multiple_images = AsyncMock() - - await adapter._process_message_background(event, build_session_key(event.source)) - - adapter.send_video.assert_awaited_once() - assert adapter.notices == ["⚠️ Couldn't deliver the video attachment."] diff --git a/tests/test_no_shadowed_test_definitions.py b/tests/test_no_shadowed_test_definitions.py new file mode 100644 index 0000000000000..b96752c4bb0f3 --- /dev/null +++ b/tests/test_no_shadowed_test_definitions.py @@ -0,0 +1,124 @@ +"""No test module may define the same name twice in one scope. + +Python keeps only the last definition, so a duplicate silently deletes the +first one. When the shadowed name is a test, its coverage disappears with no +error and no skip; when it is an ``autouse`` fixture, the whole module quietly +switches to the newer one's isolation rules. + +Both had already happened here: + +* ``tests/agent/test_auxiliary_client.py`` grew a second ``_clean_env`` + ``autouse`` fixture alongside an NVIDIA feature. It replaced the original for + all 158 tests in the file, dropping the ``ANTHROPIC_API_KEY`` / + ``ANTHROPIC_TOKEN`` / ``CLAUDE_CODE_OAUTH_TOKEN`` env stripping and the + ``_aux_unhealthy_*`` cache reset between tests. Individual tests had started + clearing that cache by hand to work around it. +* ``tests/gateway/test_mattermost.py`` had two copies of + ``test_progress_send_with_invalid_thread_root_never_falls_back_flat``; the + surviving one omitted the recorded 400 ``invalid root_id`` state, so the + case the name describes was never exercised. + +This guard is cheap and catches the whole class at collection time. +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +TESTS_ROOT = REPO_ROOT / "tests" + +# Decorators that legitimately repeat a name in one scope. +_REDEFINING_DECORATORS = ("overload", "setter", "getter", "deleter", "register") + +# Throwaway callbacks conventionally named `_` are not shadowing bugs. +_ALLOWED_REPEATS = {"_"} + + +def _decorator_names(node: ast.AST) -> list[str]: + out = [] + for dec in getattr(node, "decorator_list", []): + try: + out.append(ast.unparse(dec)) + except Exception: # pragma: no cover - defensive + pass + return out + + +def _duplicates_in(body, scope: str, rel: str) -> list[str]: + seen: dict[str, int] = {} + problems: list[str] = [] + for node in body: + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if node.name in _ALLOWED_REPEATS: + continue + if any( + marker in dec + for dec in _decorator_names(node) + for marker in _REDEFINING_DECORATORS + ): + seen[node.name] = node.lineno + continue + if node.name in seen: + problems.append( + f"{rel}:{node.lineno} {scope}.{node.name}() shadows the " + f"definition at line {seen[node.name]}" + ) + seen[node.name] = node.lineno + return problems + + +def _test_modules() -> list[Path]: + return sorted(TESTS_ROOT.rglob("test_*.py")) + + +def test_no_shadowed_definitions_in_test_modules(): + problems: list[str] = [] + for path in _test_modules(): + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except (SyntaxError, UnicodeDecodeError): # pragma: no cover + continue + rel = path.relative_to(REPO_ROOT).as_posix() + problems += _duplicates_in(tree.body, "", rel) + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef): + problems += _duplicates_in(node.body, node.name, rel) + + assert not problems, ( + "A duplicate definition silently deletes the earlier one. Rename or " + "remove:\n " + "\n ".join(problems) + ) + + +def test_guard_detects_a_known_duplicate_shape(): + """The guard must actually fire, not vacuously pass.""" + src = ( + "def test_a():\n pass\n\n" + "def test_a():\n pass\n" + ) + tree = ast.parse(src) + assert _duplicates_in(tree.body, "", "fake.py") + + +@pytest.mark.parametrize("decorator", ["@property", "@x.setter", "@functools.singledispatch"]) +def test_guard_allows_legitimate_redefinition(decorator): + src = ( + "class C:\n" + " def f(self):\n pass\n" + f" {decorator}\n" + " def f(self):\n pass\n" + ) + tree = ast.parse(src) + cls = tree.body[0] + problems = _duplicates_in(cls.body, "C", "fake.py") + # Only the setter/getter/register family is exempt; a bare @property + # repeat is still a real shadow. + if any(m in decorator for m in _REDEFINING_DECORATORS): + assert not problems + else: + assert problems