fix(tests): live module must not import the developer's .env at collection - #80343
fix(tests): live module must not import the developer's .env at collection#80343rodrigogs wants to merge 1 commit into
Conversation
230a6f8 to
fe309e7
Compare
e87875d to
76aa4c0
Compare
|
Hi — heads-up that CI has never actually run on this PR: every workflow run (CI + Docker Build) since it was opened ends in |
cc04ca5 to
1163d9f
Compare
|
Rebased onto current Conflict-free, and 492 commits of drift changed nothing this PR depends on:
The single commit replayed with no conflicts and the resulting diff is byte-identical to the pre-rebase one. This repository does not run CI on pull requests from forks, so the checks tab stays empty and |
…ction
Two tests in tests/tools/test_web_tools_config.py passed alone and failed in a
wide run:
TestBackendSelection::test_fallback_no_keys_defaults_to_firecrawl
AssertionError: assert 'searxng' == 'firecrawl'
TestCheckWebApiKey::test_null_backend_value_does_not_crash
AssertionError: assert True is False
tests/run_agent/test_sequential_chats_live.py is skipped unless
HERMES_LIVE_TESTS=1, but the skip governs EXECUTION only. Its _load_user_env()
call sat at module scope, so pytest ran it during COLLECTION — before any
fixture, including the hermetic _hermetic_environment autouse fixture that
scrubs credential-shaped variables. It copied every key from ~/.hermes/.env into
os.environ via setdefault.
On this machine that file holds 23 keys, and roughly 12 of them (SEARXNG_URL,
TELEGRAM_*, WHATSAPP_*, HERMES_SPOTIFY_*) match neither _CREDENTIAL_NAMES nor
the _CREDENTIAL_SUFFIXES heuristic, so the scrubber never removed them. With
SEARXNG_URL leaked, _get_backend() hit its searxng candidate and returned
searxng instead of firecrawl. The suite's outcome therefore depended on what the
developer happened to have configured locally.
Two properties made this hard to bisect, and are worth recording:
* It is collection-time, not execution-order. Selecting only the two victims
out of the full tree still fails with zero other tests executing:
`pytest tests/ -k '<the two names>'` -> 2 failed, 26400 deselected.
* Order is irrelevant. Listing the victim file FIRST still fails, because
pytest imports every collected module before running anything. This is why
a pairwise bisect over 39 files, and a prefix bisect over the ordered node
ids, both found nothing: feeding the full 180-item ordered prefix passes.
Fix: import only the key this module actually needs (OPENROUTER_API_KEY), and
only when live runs are enabled. A skipped module should not touch shared
process state merely by being imported.
The added regression test walks every test module's AST and fails on any that
copies file contents into os.environ at import time. It deliberately targets
that combination rather than module-scope env writes in general: a fixed
`os.environ["TERMINAL_ENV"] = "local"` is deterministic and reviewable, and two
modules use it on purpose. It is reading the developer's filesystem that makes
the result differ per machine.
1163d9f to
1abd151
Compare
|
Force-pushed a metadata-only fix so the contributor attribution check can pass.
Every commit's author is now |
Symptom
Two tests passed in isolation and failed in a wide run:
Root cause
tests/run_agent/test_sequential_chats_live.pyis skipped unlessHERMES_LIVE_TESTS=1, but the skip governs execution, not import. Its_load_user_env()call sat at module scope, so pytest ran it during collection — before any fixture, including the hermetic_hermetic_environmentautouse fixture that scrubs credential-shaped variables. It copied every key from~/.hermes/.envintoos.environviasetdefault.On the machine where this reproduced, that file holds 23 keys. Roughly 12 of them —
SEARXNG_URL,TELEGRAM_*,WHATSAPP_*,HERMES_SPOTIFY_*— match neither_CREDENTIAL_NAMESnor the_CREDENTIAL_SUFFIXESheuristic intests/conftest.py, so the scrubber never removed them. WithSEARXNG_URLleaked,_get_backend()reached its searxng candidate and returnedsearxnginstead offirecrawl.The suite's outcome therefore depended on what the developer happened to have configured locally.
Why this was hard to bisect
Two properties, both worth recording for the next person:
It is collection-time, not execution-order. Selecting only the two victims out of the full tree still fails, with zero other tests executing:
Order is irrelevant. Listing the victim file first still fails, because pytest imports every collected module before running anything:
This is why a pairwise bisect over the 39 candidate files found nothing, and why feeding pytest the full 180-item ordered prefix of node ids passes cleanly. Delta debugging over the collected file set converged on a single file in 22 runs.
Fix
Import only the key the module actually needs (
OPENROUTER_API_KEY), and only when live runs are enabled. A skipped module should not touch shared process state merely by being imported.The added regression test walks every test module's AST and fails on any that copies file contents into
os.environat import time. It deliberately targets that combination rather than module-scope env writes in general: a fixedos.environ["TERMINAL_ENV"] = "local"is deterministic and reviewable, andtests/tools/test_code_execution.pyandtest_code_execution_modes.pyboth use it on purpose. Reading the developer's filesystem is what makes the result differ per machine.Verification
Shortest reproducer, 0.31s, before the fix:
After:
2 passed, 1 skipped in 0.31s.-k "parallel or web_tools or lazy_deps": both victims pass. The only remaining failures in that selection areTestParallelClientConfig, fixed independently by fix(web): a benched lazy-install must not disable an importable SDK #80022 — with both branches applied the selection is 213 passed, 5 skipped, 0 failed.git stashthe module, test goes red), so it is not a tautology.tests/tools/in full passes;ruffclean on both touched files.Found alongside #79839, #79840 and #80022 while investigating a single incident — independent subsystems, separate PRs.