test(gateway): isolate plugin adapter imports and guard the anti-pattern
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.
This commit is contained in:
Teknium
@ -58,6 +58,7 @@ AUTHOR_MAP = {
|
||||
"nbot@liizfq.top": "liizfq",
|
||||
"274096618+hermes-agent-dhabibi@users.noreply.github.com": "dhabibi",
|
||||
"dejie.guo@gmail.com": "JayGwod",
|
||||
"aamirjawaid@microsoft.com": "heyitsaamir",
|
||||
"johnnncenaaa77@gmail.com": "johnncenae",
|
||||
"thomasjhon6666@gmail.com": "ThomassJonax",
|
||||
"focusflow.app.help@gmail.com": "yes999zc",
|
||||
|
||||
72
tests/gateway/_plugin_adapter_loader.py
Normal file
72
tests/gateway/_plugin_adapter_loader.py
Normal file
@ -0,0 +1,72 @@
|
||||
"""Shared helper for loading platform-plugin ``adapter.py`` modules in tests.
|
||||
|
||||
Every platform plugin under ``plugins/platforms/<name>/`` ships its own
|
||||
``adapter.py``. If two tests independently do::
|
||||
|
||||
sys.path.insert(0, "plugins/platforms/irc")
|
||||
from adapter import IRCAdapter
|
||||
|
||||
sys.path.insert(0, "plugins/platforms/teams")
|
||||
from adapter import TeamsAdapter
|
||||
|
||||
…then whichever collects first in an xdist worker wins
|
||||
``sys.modules["adapter"]``, and the other raises ``ImportError`` at
|
||||
collection time. The fallout cascades across unrelated tests sharing that
|
||||
worker because ``sys.path`` is still polluted.
|
||||
|
||||
Use :func:`load_plugin_adapter` instead of ad-hoc ``sys.path`` tricks.
|
||||
It loads the adapter from an explicit file path under a unique module
|
||||
name (``plugin_adapter_<plugin_name>``), so it cannot collide with any
|
||||
other plugin's adapter module.
|
||||
|
||||
The ``tests/gateway/conftest.py`` guard rejects the anti-pattern at
|
||||
collection time so this can't regress when new plugin adapter tests are
|
||||
added.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib.util
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from types import ModuleType
|
||||
|
||||
|
||||
_REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
_PLUGINS_DIR = _REPO_ROOT / "plugins" / "platforms"
|
||||
|
||||
|
||||
def load_plugin_adapter(plugin_name: str) -> ModuleType:
|
||||
"""Import ``plugins/platforms/<plugin_name>/adapter.py`` in isolation.
|
||||
|
||||
The module is registered under the unique name
|
||||
``plugin_adapter_<plugin_name>`` in ``sys.modules``. No ``sys.path``
|
||||
mutation. Safe to call multiple times — repeat calls return the
|
||||
already-loaded module.
|
||||
"""
|
||||
module_name = f"plugin_adapter_{plugin_name}"
|
||||
cached = sys.modules.get(module_name)
|
||||
if cached is not None:
|
||||
return cached
|
||||
|
||||
adapter_path = _PLUGINS_DIR / plugin_name / "adapter.py"
|
||||
if not adapter_path.is_file():
|
||||
raise FileNotFoundError(
|
||||
f"Plugin adapter not found: {adapter_path}. "
|
||||
f"Known plugins: {sorted(p.name for p in _PLUGINS_DIR.iterdir() if p.is_dir())}"
|
||||
)
|
||||
|
||||
spec = importlib.util.spec_from_file_location(module_name, adapter_path)
|
||||
if spec is None or spec.loader is None:
|
||||
raise ImportError(f"Could not build import spec for {adapter_path}")
|
||||
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
# Register BEFORE exec so the module can find itself if needed (some
|
||||
# modules do ``sys.modules[__name__]`` reflection during import).
|
||||
sys.modules[module_name] = module
|
||||
try:
|
||||
spec.loader.exec_module(module)
|
||||
except Exception:
|
||||
sys.modules.pop(module_name, None)
|
||||
raise
|
||||
return module
|
||||
@ -12,11 +12,32 @@ ImportError fallback, causing 30+ downstream test failures wherever
|
||||
|
||||
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.
|
||||
@ -197,3 +218,128 @@ def _ensure_discord_mock() -> None:
|
||||
# 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
|
||||
)
|
||||
|
||||
|
||||
@ -7,16 +7,19 @@ import pytest
|
||||
from pathlib import Path
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
# Ensure the plugins directory is on sys.path for direct import
|
||||
_REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
_IRC_PLUGIN_DIR = _REPO_ROOT / "plugins" / "platforms" / "irc"
|
||||
if str(_IRC_PLUGIN_DIR) not in sys.path:
|
||||
sys.path.insert(0, str(_IRC_PLUGIN_DIR))
|
||||
from tests.gateway._plugin_adapter_loader import load_plugin_adapter
|
||||
|
||||
# Load plugins/platforms/irc/adapter.py under a unique module name
|
||||
# (plugin_adapter_irc) so it cannot collide with other plugin adapters
|
||||
# loaded by sibling tests in the same xdist worker.
|
||||
_irc_mod = load_plugin_adapter("irc")
|
||||
|
||||
# ── IRC protocol helpers ─────────────────────────────────────────────────
|
||||
|
||||
from adapter import _parse_irc_message, _extract_nick
|
||||
_parse_irc_message = _irc_mod._parse_irc_message
|
||||
_extract_nick = _irc_mod._extract_nick
|
||||
IRCAdapter = _irc_mod.IRCAdapter
|
||||
check_requirements = _irc_mod.check_requirements
|
||||
validate_config = _irc_mod.validate_config
|
||||
register = _irc_mod.register
|
||||
|
||||
|
||||
class TestIRCProtocolHelpers:
|
||||
@ -52,8 +55,6 @@ class TestIRCProtocolHelpers:
|
||||
|
||||
# ── IRC Adapter ──────────────────────────────────────────────────────────
|
||||
|
||||
from adapter import IRCAdapter, check_requirements, validate_config
|
||||
|
||||
|
||||
class TestIRCAdapterInit:
|
||||
|
||||
@ -494,8 +495,6 @@ class TestIRCPluginRegistration:
|
||||
# Clean up if already registered
|
||||
platform_registry.unregister("irc")
|
||||
|
||||
from adapter import register
|
||||
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
ctx.register_platform.assert_called_once()
|
||||
|
||||
@ -10,12 +10,7 @@ from unittest.mock import AsyncMock, MagicMock, patch
|
||||
import pytest
|
||||
|
||||
from gateway.config import Platform, PlatformConfig, HomeChannel
|
||||
|
||||
# Ensure the plugin directory is on sys.path for direct import (mirrors IRC pattern)
|
||||
_REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
_TEAMS_PLUGIN_DIR = _REPO_ROOT / "plugins" / "platforms" / "teams"
|
||||
if str(_TEAMS_PLUGIN_DIR) not in sys.path:
|
||||
sys.path.insert(0, str(_TEAMS_PLUGIN_DIR))
|
||||
from tests.gateway._plugin_adapter_loader import load_plugin_adapter
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@ -160,13 +155,18 @@ def _ensure_teams_mock():
|
||||
|
||||
_ensure_teams_mock()
|
||||
|
||||
# Now safe to import the adapter
|
||||
import adapter as _teams_mod
|
||||
# Load plugins/platforms/teams/adapter.py under a unique module name
|
||||
# (plugin_adapter_teams) so it cannot collide with sibling plugin adapters.
|
||||
_teams_mod = load_plugin_adapter("teams")
|
||||
|
||||
_teams_mod.TEAMS_SDK_AVAILABLE = True
|
||||
_teams_mod.AIOHTTP_AVAILABLE = True
|
||||
|
||||
from adapter import TeamsAdapter, check_requirements, check_teams_requirements, validate_config
|
||||
TeamsAdapter = _teams_mod.TeamsAdapter
|
||||
check_requirements = _teams_mod.check_requirements
|
||||
check_teams_requirements = _teams_mod.check_teams_requirements
|
||||
validate_config = _teams_mod.validate_config
|
||||
register = _teams_mod.register
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@ -276,20 +276,17 @@ class TestTeamsAdapterInit:
|
||||
class TestTeamsPluginRegistration:
|
||||
|
||||
def test_register_calls_ctx(self):
|
||||
from adapter import register
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
ctx.register_platform.assert_called_once()
|
||||
|
||||
def test_register_name(self):
|
||||
from adapter import register
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
kwargs = ctx.register_platform.call_args[1]
|
||||
assert kwargs["name"] == "teams"
|
||||
|
||||
def test_register_auth_env_vars(self):
|
||||
from adapter import register
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
kwargs = ctx.register_platform.call_args[1]
|
||||
@ -297,21 +294,18 @@ class TestTeamsPluginRegistration:
|
||||
assert kwargs["allow_all_env"] == "TEAMS_ALLOW_ALL_USERS"
|
||||
|
||||
def test_register_max_message_length(self):
|
||||
from adapter import register
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
kwargs = ctx.register_platform.call_args[1]
|
||||
assert kwargs["max_message_length"] == 28000
|
||||
|
||||
def test_register_has_setup_fn(self):
|
||||
from adapter import register
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
kwargs = ctx.register_platform.call_args[1]
|
||||
assert callable(kwargs.get("setup_fn"))
|
||||
|
||||
def test_register_has_platform_hint(self):
|
||||
from adapter import register
|
||||
ctx = MagicMock()
|
||||
register(ctx)
|
||||
kwargs = ctx.register_platform.call_args[1]
|
||||
|
||||
Reference in New Issue
Block a user