mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-03 17:27:37 +08:00
Fixes the xdist collision that broke CI on PR #17764, and structurally prevents future plugin-adapter tests from reintroducing it. Problem ------- tests/gateway/test_teams.py (new in this PR) and tests/gateway/test_irc_adapter.py (already on main) both followed the same anti-pattern: sys.path.insert(0, str(_REPO_ROOT / 'plugins' / 'platforms' / '<name>')) from adapter import <Adapter> Every platform plugin ships its own adapter.py, so the bare 'from adapter import ...' races for sys.modules['adapter']. Whichever test collected first in a given xdist worker won; the other crashed at collection with ImportError, and the polluted sys.path cascaded into 19 unrelated test failures across tools/, hermes_cli/, and run_agent/ in the same worker. Fix --- 1. tests/gateway/_plugin_adapter_loader.py (new): shared helper load_plugin_adapter('<name>') that imports plugins/platforms/<name>/adapter.py via importlib.util under the unique module name plugin_adapter_<name>. Zero sys.path mutation, no possibility of collision. 2. tests/gateway/test_irc_adapter.py and tests/gateway/test_teams.py: migrated to the helper. All 'from adapter import ...' statements (including the ones inside test methods) are replaced with module-level attribute access on the loaded module. 3. tests/gateway/conftest.py: new pytest_configure guard that AST-scans every test_*.py under tests/gateway/ at session start and fails the run with a pointer to the helper if any test uses sys.path.insert into plugins/platforms/ OR a bare 'import adapter' / 'from adapter import'. Runs on the xdist controller only (skipped in workers). The next plugin adapter test that tries to reintroduce this pattern gets rejected at collection time with a clear remediation message. 4. scripts/release.py: add aamirjawaid@microsoft.com -> heyitsaamir to AUTHOR_MAP so the check-attribution workflow passes. Validation ---------- scripts/run_tests.sh tests/gateway/ 4194 passed scripts/run_tests.sh tests/gateway/test_{teams,irc}* 72 passed (both orderings) scripts/run_tests.sh <11 prev-failing test files> 398 passed Guard triggers correctly on both Path-operator and string-literal forms of the anti-pattern.
346 lines
13 KiB
Python
346 lines
13 KiB
Python
"""Shared fixtures for gateway tests.
|
|
|
|
The ``_ensure_telegram_mock`` helper guarantees that a minimal mock of
|
|
the ``telegram`` package is registered in :data:`sys.modules` **before**
|
|
any test file triggers ``from gateway.platforms.telegram import ...``.
|
|
|
|
Without this, ``pytest-xdist`` workers that happen to collect
|
|
``test_telegram_caption_merge.py`` (bare top-level import, no per-file
|
|
mock) first will cache ``ChatType = None`` from the production
|
|
ImportError fallback, causing 30+ downstream test failures wherever
|
|
``ChatType.GROUP`` / ``ChatType.SUPERGROUP`` is accessed.
|
|
|
|
Individual test files may still call their own ``_ensure_telegram_mock``
|
|
— it short-circuits when the mock is already present.
|
|
|
|
Plugin-adapter anti-pattern guard
|
|
---------------------------------
|
|
Tests for platform plugins (``plugins/platforms/<name>/adapter.py``)
|
|
must load the adapter via
|
|
:func:`tests.gateway._plugin_adapter_loader.load_plugin_adapter`, not by
|
|
adding the plugin directory to ``sys.path`` and doing a bare
|
|
``from adapter import ...``. The guard at the bottom of this file
|
|
scans test module ASTs at collection time and fails collection with a
|
|
pointer to the helper if the anti-pattern is detected.
|
|
|
|
Rationale: every plugin ships its own ``adapter.py``, and two tests each
|
|
inserting their plugin dir on ``sys.path[0]`` race for
|
|
``sys.modules["adapter"]`` in the same xdist worker. Whichever collects
|
|
first wins; the other fails with ``ImportError``, and the polluted
|
|
``sys.path`` cascades into unrelated tests. See PR #17764 for the
|
|
incident.
|
|
"""
|
|
|
|
import ast
|
|
import sys
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock
|
|
|
|
import pytest
|
|
|
|
|
|
def _ensure_telegram_mock() -> None:
|
|
"""Install a comprehensive telegram mock in sys.modules.
|
|
|
|
Idempotent — skips when the real library is already imported.
|
|
Uses ``sys.modules[name] = mod`` (overwrite) instead of
|
|
``setdefault`` so it wins even if a partial/broken import
|
|
already cached a module with ``ChatType = None``.
|
|
"""
|
|
if "telegram" in sys.modules and hasattr(sys.modules["telegram"], "__file__"):
|
|
return # Real library is installed — nothing to mock
|
|
|
|
mod = MagicMock()
|
|
mod.ext.ContextTypes.DEFAULT_TYPE = type(None)
|
|
mod.constants.ParseMode.MARKDOWN = "Markdown"
|
|
mod.constants.ParseMode.MARKDOWN_V2 = "MarkdownV2"
|
|
mod.constants.ParseMode.HTML = "HTML"
|
|
mod.constants.ChatType.PRIVATE = "private"
|
|
mod.constants.ChatType.GROUP = "group"
|
|
mod.constants.ChatType.SUPERGROUP = "supergroup"
|
|
mod.constants.ChatType.CHANNEL = "channel"
|
|
|
|
# Real exception classes so ``except (NetworkError, ...)`` clauses
|
|
# in production code don't blow up with TypeError.
|
|
mod.error.NetworkError = type("NetworkError", (OSError,), {})
|
|
mod.error.TimedOut = type("TimedOut", (OSError,), {})
|
|
mod.error.BadRequest = type("BadRequest", (Exception,), {})
|
|
mod.error.Forbidden = type("Forbidden", (Exception,), {})
|
|
mod.error.InvalidToken = type("InvalidToken", (Exception,), {})
|
|
mod.error.RetryAfter = type("RetryAfter", (Exception,), {"retry_after": 1})
|
|
mod.error.Conflict = type("Conflict", (Exception,), {})
|
|
|
|
# Update.ALL_TYPES used in start_polling()
|
|
mod.Update.ALL_TYPES = []
|
|
|
|
for name in (
|
|
"telegram",
|
|
"telegram.ext",
|
|
"telegram.constants",
|
|
"telegram.request",
|
|
):
|
|
sys.modules[name] = mod
|
|
sys.modules["telegram.error"] = mod.error
|
|
|
|
|
|
def _ensure_discord_mock() -> None:
|
|
"""Install a comprehensive discord mock in sys.modules.
|
|
|
|
Idempotent — skips when the real library is already imported.
|
|
Uses ``sys.modules[name] = mod`` (overwrite) instead of
|
|
``setdefault`` so it wins even if a partial/broken import already
|
|
cached the module.
|
|
|
|
This mock is comprehensive — it includes **all** attributes needed by
|
|
every gateway discord test file. Individual test files should call
|
|
this function (it short-circuits when already present) rather than
|
|
maintaining their own mock setup.
|
|
"""
|
|
if "discord" in sys.modules and hasattr(sys.modules["discord"], "__file__"):
|
|
return # Real library is installed — nothing to mock
|
|
|
|
from types import SimpleNamespace
|
|
|
|
discord_mod = MagicMock()
|
|
discord_mod.Intents.default.return_value = MagicMock()
|
|
discord_mod.Client = MagicMock
|
|
discord_mod.File = MagicMock
|
|
discord_mod.DMChannel = type("DMChannel", (), {})
|
|
discord_mod.Thread = type("Thread", (), {})
|
|
discord_mod.ForumChannel = type("ForumChannel", (), {})
|
|
discord_mod.Interaction = object
|
|
discord_mod.Message = type("Message", (), {})
|
|
|
|
# Embed: accept the kwargs production code / tests use
|
|
# (title, description, color). MagicMock auto-attributes work too,
|
|
# but some tests construct and inspect .title/.description directly.
|
|
class _FakeEmbed:
|
|
def __init__(self, *, title=None, description=None, color=None, **_):
|
|
self.title = title
|
|
self.description = description
|
|
self.color = color
|
|
discord_mod.Embed = _FakeEmbed
|
|
|
|
# ui.View / ui.Select / ui.Button: real classes (not MagicMock) so
|
|
# tests that subclass ModelPickerView / iterate .children / clear
|
|
# items work.
|
|
class _FakeView:
|
|
def __init__(self, timeout=None):
|
|
self.timeout = timeout
|
|
self.children = []
|
|
def add_item(self, item):
|
|
self.children.append(item)
|
|
def clear_items(self):
|
|
self.children.clear()
|
|
|
|
class _FakeSelect:
|
|
def __init__(self, *, placeholder=None, options=None, custom_id=None, **_):
|
|
self.placeholder = placeholder
|
|
self.options = options or []
|
|
self.custom_id = custom_id
|
|
self.callback = None
|
|
self.disabled = False
|
|
|
|
class _FakeButton:
|
|
def __init__(self, *, label=None, style=None, custom_id=None, emoji=None,
|
|
url=None, disabled=False, row=None, sku_id=None, **_):
|
|
self.label = label
|
|
self.style = style
|
|
self.custom_id = custom_id
|
|
self.emoji = emoji
|
|
self.url = url
|
|
self.disabled = disabled
|
|
self.row = row
|
|
self.sku_id = sku_id
|
|
self.callback = None
|
|
|
|
class _FakeSelectOption:
|
|
def __init__(self, *, label=None, value=None, description=None, **_):
|
|
self.label = label
|
|
self.value = value
|
|
self.description = description
|
|
discord_mod.SelectOption = _FakeSelectOption
|
|
|
|
discord_mod.ui = SimpleNamespace(
|
|
View=_FakeView,
|
|
Select=_FakeSelect,
|
|
Button=_FakeButton,
|
|
button=lambda *a, **k: (lambda fn: fn),
|
|
)
|
|
discord_mod.ButtonStyle = SimpleNamespace(
|
|
success=1, primary=2, secondary=2, danger=3,
|
|
green=1, grey=2, blurple=2, red=3,
|
|
)
|
|
discord_mod.Color = SimpleNamespace(
|
|
orange=lambda: 1, green=lambda: 2, blue=lambda: 3,
|
|
red=lambda: 4, purple=lambda: 5, greyple=lambda: 6,
|
|
)
|
|
|
|
# app_commands — needed by _register_slash_commands auto-registration
|
|
class _FakeGroup:
|
|
def __init__(self, *, name, description, parent=None):
|
|
self.name = name
|
|
self.description = description
|
|
self.parent = parent
|
|
self._children: dict = {}
|
|
if parent is not None:
|
|
parent.add_command(self)
|
|
|
|
def add_command(self, cmd):
|
|
self._children[cmd.name] = cmd
|
|
|
|
class _FakeCommand:
|
|
def __init__(self, *, name, description, callback, parent=None):
|
|
self.name = name
|
|
self.description = description
|
|
self.callback = callback
|
|
self.parent = parent
|
|
|
|
discord_mod.app_commands = SimpleNamespace(
|
|
describe=lambda **kwargs: (lambda fn: fn),
|
|
choices=lambda **kwargs: (lambda fn: fn),
|
|
Choice=lambda **kwargs: SimpleNamespace(**kwargs),
|
|
Group=_FakeGroup,
|
|
Command=_FakeCommand,
|
|
)
|
|
|
|
ext_mod = MagicMock()
|
|
commands_mod = MagicMock()
|
|
commands_mod.Bot = MagicMock
|
|
ext_mod.commands = commands_mod
|
|
|
|
for name in ("discord", "discord.ext", "discord.ext.commands"):
|
|
sys.modules[name] = discord_mod
|
|
sys.modules["discord.ext"] = ext_mod
|
|
sys.modules["discord.ext.commands"] = commands_mod
|
|
|
|
|
|
# Run at collection time — before any test file's module-level imports.
|
|
_ensure_telegram_mock()
|
|
_ensure_discord_mock()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Plugin-adapter anti-pattern guard
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_GATEWAY_DIR = Path(__file__).resolve().parent
|
|
_GUARD_HINT = (
|
|
"Plugin adapter tests must use "
|
|
"``from tests.gateway._plugin_adapter_loader import load_plugin_adapter`` "
|
|
"and call ``load_plugin_adapter('<plugin_name>')`` instead of inserting "
|
|
"``plugins/platforms/<name>/`` on sys.path and doing a bare ``import "
|
|
"adapter`` / ``from adapter import ...``. See the 'Plugin-adapter "
|
|
"anti-pattern guard' docstring in tests/gateway/conftest.py."
|
|
)
|
|
|
|
|
|
def _scan_for_plugin_adapter_antipattern(source: str) -> list[str]:
|
|
"""Return a list of offending-line descriptions, or [] if clean.
|
|
|
|
Flags two things:
|
|
1. ``sys.path.insert(..., <something mentioning 'plugins/platforms'>)``
|
|
2. ``import adapter`` or ``from adapter import ...`` at module level.
|
|
"""
|
|
try:
|
|
tree = ast.parse(source)
|
|
except SyntaxError:
|
|
return [] # Let pytest surface the real syntax error.
|
|
|
|
offenses: list[str] = []
|
|
|
|
for node in ast.walk(tree):
|
|
# sys.path.insert(0, ".../plugins/platforms/...")
|
|
if isinstance(node, ast.Call):
|
|
func = node.func
|
|
target_name: str | None = None
|
|
if isinstance(func, ast.Attribute):
|
|
# sys.path.insert / sys.path.append
|
|
if (
|
|
isinstance(func.value, ast.Attribute)
|
|
and isinstance(func.value.value, ast.Name)
|
|
and func.value.value.id == "sys"
|
|
and func.value.attr == "path"
|
|
and func.attr in ("insert", "append", "extend")
|
|
):
|
|
target_name = f"sys.path.{func.attr}"
|
|
|
|
if target_name is not None:
|
|
call_src = ast.unparse(node)
|
|
# Match both the string-literal form
|
|
# ``.../plugins/platforms/...`` and the Path-operator form
|
|
# ``Path(...) / 'plugins' / 'platforms' / ...`` that
|
|
# plugin tests typically use.
|
|
_src_no_ws = "".join(call_src.split())
|
|
if (
|
|
"plugins/platforms" in call_src
|
|
or "plugins\\platforms" in call_src
|
|
or "'plugins'/'platforms'" in _src_no_ws
|
|
or '"plugins"/"platforms"' in _src_no_ws
|
|
):
|
|
offenses.append(
|
|
f"line {node.lineno}: {target_name}(...) points into "
|
|
f"plugins/platforms/"
|
|
)
|
|
|
|
# Bare `import adapter` / `from adapter import ...` anywhere (module level
|
|
# OR inside functions — both are symptoms of the same pattern).
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Import):
|
|
for alias in node.names:
|
|
if alias.name == "adapter":
|
|
offenses.append(
|
|
f"line {node.lineno}: ``import adapter`` "
|
|
f"(bare — resolves to whichever plugin's adapter.py "
|
|
f"is first on sys.path)"
|
|
)
|
|
elif isinstance(node, ast.ImportFrom):
|
|
if node.module == "adapter" and node.level == 0:
|
|
offenses.append(
|
|
f"line {node.lineno}: ``from adapter import ...`` "
|
|
f"(bare — resolves to whichever plugin's adapter.py "
|
|
f"is first on sys.path)"
|
|
)
|
|
|
|
return offenses
|
|
|
|
|
|
def pytest_configure(config):
|
|
"""Reject plugin-adapter tests that use the sys.path anti-pattern.
|
|
|
|
Runs once per pytest session on the controller, BEFORE any xdist
|
|
worker is spawned. If any file under ``tests/gateway/`` matches the
|
|
anti-pattern, we fail the whole session with a clear message —
|
|
before a polluted ``sys.path`` can cascade across workers.
|
|
"""
|
|
# Only run on the xdist controller (or in non-xdist runs). Skip on
|
|
# worker subprocesses so we don't scan the filesystem N times.
|
|
if hasattr(config, "workerinput"):
|
|
return
|
|
|
|
violations: list[str] = []
|
|
for path in _GATEWAY_DIR.rglob("test_*.py"):
|
|
if path.name in {"_plugin_adapter_loader.py", "conftest.py"}:
|
|
continue
|
|
try:
|
|
source = path.read_text(encoding="utf-8")
|
|
except OSError:
|
|
continue
|
|
if "adapter" not in source and "plugins/platforms" not in source:
|
|
continue
|
|
offenses = _scan_for_plugin_adapter_antipattern(source)
|
|
if offenses:
|
|
violations.append(
|
|
f" {path.relative_to(_GATEWAY_DIR.parent.parent)}:\n "
|
|
+ "\n ".join(offenses)
|
|
)
|
|
|
|
if violations:
|
|
raise pytest.UsageError(
|
|
"Plugin-adapter-import anti-pattern detected in gateway tests:\n"
|
|
+ "\n".join(violations)
|
|
+ "\n\n"
|
|
+ _GUARD_HINT
|
|
)
|
|
|