test: fix mock isolation and environment-leak failures across the suite - #28134
Closed
Jb8777 wants to merge 1 commit into
Closed
test: fix mock isolation and environment-leak failures across the suite#28134Jb8777 wants to merge 1 commit into
Jb8777 wants to merge 1 commit into
Conversation
alt-glitch
added
type/test
The full suite was failing ~95 tests under `-n auto` xdist load-balancing
on a dev box. Root causes were all upstream test-infra issues, not real
production bugs:
1. Per-file `_ensure_discord_mock` / `_ensure_telegram_mock` helpers
guarded with `hasattr(sys.modules[lib], "__file__")`. MagicMock
auto-creates any attribute on access, so once any per-file mock
landed, every subsequent installer bailed and tests inherited
whichever mock won the worker's import race.
2. The central `gateway/conftest.py` mock set values on
`mod.constants.ChatType.SUPERGROUP` but `sys.modules["telegram.constants"]
= mod` (parent) — so `from telegram.constants import ChatType` resolves
to `mod.ChatType` (different MagicMock, no values set).
3. `gateway.platforms.discord` captures `discord.AllowedMentions` at
first import; if a non-gateway test imports it before `gateway/conftest.py`
loads, the production reference is bound to a bare MagicMock.
4. `model_tools._tool_defs_cache` and `tools.registry._check_fn_cache`
key on toolsets + config-mtime, NOT on env-config monkeypatches, so
a prior test populating the cache causes vercel-hide tests to return
stale results.
5. `hermes_cli/model_switch.py:1715` makes a live HTTP probe via
`fetch_api_models(api_key, api_url)` against each custom_providers
endpoint, leaking the dev box's real Ollama models into assertions.
6. `cli.CLI_CONFIG` (module-level state) is populated at first import. If
that happens before per-test HERMES_HOME isolation, tests see the dev
box's real `delegation.max_concurrent_children` value (5 instead of 3).
7. `test_cosign_missing_marker_clears_when_cosign_appears` asserted
cosign was absent without patching `shutil.which`; fails on machines
that actually have cosign installed.
Fixes:
* tests/gateway/conftest.py: replace `__file__` bail-out with a
sentinel-attribute (`_hermes_test_mock`) check via `vars()`. Enrich
the central mock with AllowedMentions, Permissions, opus,
`app_commands.autocomplete`, Forbidden/NotFound/HTTPException/
RateLimited/ConnectionClosed exception classes, the full MessageType
enum, an Object factory, and ChatType strings mirrored onto
`mod.ChatType` so the two import paths resolve to the same values.
Do NOT mirror ParseMode — telegram_approval_buttons / model_picker
tests assert `"MARKDOWN_V2" in repr(parse_mode)` and rely on the
MagicMock repr.
* tests/conftest.py: install central discord/telegram mocks at
root-conftest module-load time, before any test file imports
gateway.platforms.*.
* tests/gateway/*.py, tests/tools/test_send_message_tool.py: rewrite
27 per-file `_ensure_*_mock` bodies as 3-line delegations to central.
* tests/tools/test_terminal_tool_requirements.py: autouse fixture
clears both `_tool_defs_cache` and `_check_fn_cache` around each test.
* tests/hermes_cli/test_model_switch_custom_providers.py and
test_list_picker_providers.py: autouse fixture monkeypatches
`hermes_cli.models.fetch_api_models` to return [] so the live probe
doesn't leak real Ollama models into test assertions.
* tests/run_agent/test_agent_guardrails.py: autouse fixture pins
`MAX_CONCURRENT_CHILDREN=3` and patches
`tools.delegate_tool._load_config` to {}.
* tests/tools/test_delegate.py: `@patch _load_config` on the two
failing TestDelegateTask/TestBlockedTools tests with the same
rationale.
* tests/tools/test_tirith_security.py::test_cosign_missing_marker_
clears_when_cosign_appears: wrap the first assertion in
`patch("shutil.which", return_value=None)` so it's hermetic.
Result: 23,889 / 23,889 pass, ~8:40 runtime under default `-n auto`,
no deselects, no skip-list. 33 files changed, +270 / -690.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Jb8777
force-pushed
the
test-mock-isolation-fixes
branch
from
May 19, 2026 05:39
a780005 to
70c26eb
Compare
16 tasks
Contributor
|
This looks implemented on current Evidence:
|
teknium1
added
the
sweeper:implemented-on-main
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a class of cross-test contamination bugs that cause non-deterministic failures in the test suite — both locally and in CI. The fixes fall into three categories:
1. Mock-installer "first-test wins" pattern (gateway/)
~27 gateway test files each had their own
_ensure_discord_mock/_ensure_telegram_mockhelper that bailed out viahasattr(mock, "__file__"). SinceMagicMockauto-fakes any attribute access, that check always passed once any test file had run its installer. Subsequent tests then inherited whatever mock shape the first installer happened to register, dropping attributes other tests needed (e.g.discord.AllowedMentions, the fulltelegram.constants.ChatTypestring enum,discord.app_commands.autocomplete,discord.Forbidden/NotFound/HTTPException/RateLimited/ConnectionClosed).Fix: centralise the installers in
tests/gateway/conftest.pywith a comprehensive mock surface and a sentinel-attribute (_hermes_test_mock) check resolved viavars()rather thangetattr(). Per-file helpers now delegate to the central installer.One subtle quirk: setting
sys.modules["telegram.constants"] = mod(the parent telegram mock) meansfrom telegram.constants import ChatTyperesolves tomod.ChatType, notmod.constants.ChatType. The fix mirrors ChatType strings onto both paths. ParseMode is deliberately not mirrored — several telegram tests assert\"MARKDOWN_V2\" in repr(parse_mode)and rely on MagicMock's default repr.2. Module-state and cache leakage (tools/, hermes_cli/, run_agent/)
Several tests captured real environment state at module-import time:
model_tools._tool_defs_cacheandtools.registry._check_fn_cachepersisted across tests, defeating latermonkeypatchof backend probeshermes_cli/model_switch.py:fetch_api_modelsmade live HTTP probes to Ollama, leaking real local models into model-picker assertionstools.delegate_tool._load_configread the developer's realdelegation.max_concurrent_childrenbefore HERMES_HOME isolation, soMAX_CONCURRENT_CHILDRENfroze at the local value (5 on this machine) instead of the test-expected default (3)Fix: per-file autouse fixtures clear both caches and pin
fetch_api_models/_load_configto deterministic stubs.3. Environment-binary detection (tools/test_tirith_security.py)
test_cosign_missing_marker_clears_when_cosign_appearsasserted that the "cosign missing" marker is set when cosign is absent — but the first assertion ran without monkeypatchingshutil.which, so on a developer machine with cosign installed the test failed.Fix: wrap the first assert in
patch(\"shutil.which\", return_value=None).Related Issue
No issue filed — surfaced while running `pytest tests/ -q` locally.
Type of Change
Changes Made
How to Test
Targeted: `pytest tests/gateway tests/tools/test_terminal_tool_requirements.py tests/hermes_cli/test_model_switch_custom_providers.py tests/hermes_cli/test_list_picker_providers.py tests/run_agent/test_agent_guardrails.py tests/tools/test_delegate.py tests/tools/test_tirith_security.py -q` — all pass (5806 passed, 8 skipped).
Subset comparison (`tests/tools tests/plugins tests/gateway` under default `-n auto`):
The 27 errors are pre-existing (unrelated namespace-package monkeypatch issue in `test_hindsight_provider.py` where pytest's `monkeypatch.setattr("plugins.memory.hindsight.X", ...)` fails when a worker hasn't transitively imported `plugins.memory` yet). Same count on both branches — out of scope for this PR.
Checklist
Code
Documentation & Housekeeping