test(bedrock,tts): skip baseline tests for optional deps removed from [all] by briandevans · Pull Request #24601 · NousResearch/hermes-agent

briandevans · 2026-05-12T23:17:21Z

Summary

CI baseline cleanup. #24515 (2026-05-12) moved several extras out of [all] to lazy-install (voice, bedrock, dingtalk, feishu, …). A clean pip install -e .[all,dev] — what CI runs — no longer pulls numpy or botocore, and three test paths fail on every CI run since:

Test path	Symptom on CI
`tests/tools/test_tts_kittentts.py`	Collection error: `ModuleNotFoundError: No module named 'numpy'`
`tests/agent/test_bedrock_adapter.py::TestResolveBedrocRegion` (3 tests)	`patch(\"botocore.session.get_session\", …)` errors: `No module named 'botocore'`
`tests/agent/test_bedrock_integration.py::TestPackaging::test_bedrock_in_all_extra`	Assertion: `\"hermes-agent[bedrock]\"` not in `pyproject.toml` (now intentional per #24515)

The fix

tests/tools/test_tts_kittentts.py — replace import numpy as np with np = pytest.importorskip(\"numpy\"). Canonical pytest optional-dep pattern: file loads when numpy is present, whole module skips at collection time when absent.
tests/agent/test_bedrock_adapter.py — add pytest.importorskip(\"botocore\") to the three TestResolveBedrocRegion methods that patch botocore.session.get_session. The two methods that don't touch botocore (test_prefers_aws_region, test_falls_back_to_default_region) keep running unconditionally.
tests/agent/test_bedrock_integration.py — replace test_bedrock_in_all_extra with test_bedrock_reachable_via_all_or_lazy. Asserts the underlying invariant — bedrock is reachable on a fresh install — via either [all] (legacy) or tools/lazy_deps.py (fix(install): use --extra all not --all-extras; drop lazy-covered extras from [all] #24515 policy). Avoids re-litigating the explicit policy decision while still catching a future regression that drops Bedrock from both reach paths.

Dingtalk's CI failures (tests/gateway/test_dingtalk.py) share the same root cause but are deliberately out of scope here — they're runtime-assertion failures (mocked SDK shape mismatch) rather than collection / patch-resolution errors and warrant a separate look.

Test plan

Focused regression — without optional deps:

uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest \\
  tests/tools/test_tts_kittentts.py \\
  tests/agent/test_bedrock_adapter.py::TestResolveBedrocRegion \\
  tests/agent/test_bedrock_integration.py::TestPackaging -v
# before: 4 failed, 1 collection error
# after:  4 passed, 4 skipped, 0 failed

Regression guard — with optional deps present:

uv run --with pytest --with pytest-xdist --with pytest-asyncio --with numpy --with boto3 python3 -m pytest \\
  tests/tools/test_tts_kittentts.py \\
  tests/agent/test_bedrock_adapter.py::TestResolveBedrocRegion \\
  tests/agent/test_bedrock_integration.py::TestPackaging -v
# 17 passed, 0 skipped — every previously-failing test now runs its real assertion

Adjacent suite — both touched test files in full, without optional deps:

uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest \\
  tests/agent/test_bedrock_adapter.py tests/agent/test_bedrock_integration.py tests/tools/test_tts_kittentts.py -v
# 164 passed, 10 skipped, 0 failed

… [all] PR NousResearch#24515 (2026-05-12) moved several extras out of `[all]` to lazy-install (voice, bedrock, dingtalk, feishu, …) so a clean `pip install -e .[all,dev]` no longer pulls numpy or botocore. Three test paths break under that policy: - tests/tools/test_tts_kittentts.py — `import numpy as np` at module top fails collection with ModuleNotFoundError. - tests/agent/test_bedrock_adapter.py::TestResolveBedrocRegion — 3 tests use `patch("botocore.session.get_session", …)` which errors when the patcher can't resolve `botocore.session`. - tests/agent/test_bedrock_integration.py::TestPackaging::test_bedrock_in_all_extra asserts `"hermes-agent[bedrock]"` is in pyproject.toml's `[all]` list, which the NousResearch#24515 policy intentionally removed. Fix: - Replace `import numpy as np` with `np = pytest.importorskip("numpy")` — the canonical pytest pattern for an optional-dep test module. When numpy is installed (production `[voice]` extra, ad-hoc `--with numpy`), the module loads normally; when absent, the whole file skips at collection time. - Add `pytest.importorskip("botocore")` to the 3 affected TestResolveBedrocRegion methods. The other tests in the class (test_prefers_aws_region, test_falls_back_to_default_region) don't touch botocore and continue to run. - Replace `test_bedrock_in_all_extra` with `test_bedrock_reachable_via_all_or_lazy`, which asserts bedrock is reachable via either `[all]` (legacy) or `tools/lazy_deps.py` (new policy). This protects the underlying invariant — fresh installs can still get to Bedrock — without re-litigating the explicit policy decision in NousResearch#24515. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Cleans up CI baseline failures introduced when optional extras (notably voice/bedrock) were moved out of [all] and into lazy-install paths, by converting hard imports/patches in tests into conditional skips and updating packaging assertions accordingly.

Changes:

Skip tests/tools/test_tts_kittentts.py at collection time when numpy is unavailable.
Skip the three Bedrock region-resolution tests that patch botocore when botocore is unavailable.
Update the Bedrock packaging test to accept either [all] inclusion or lazy-deps reachability.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`tests/tools/test_tts_kittentts.py`	Switches to `pytest.importorskip("numpy")` to avoid collection failures when `numpy` isn’t installed.
`tests/agent/test_bedrock_adapter.py`	Adds `pytest.importorskip("botocore")` to prevent patch-resolution errors when `botocore` isn’t installed.
`tests/agent/test_bedrock_integration.py`	Replaces the strict `[all]` assertion with an “in `[all]` or in `LAZY_DEPS`” reachability invariant.

Comments suppressed due to low confidence (2)

tests/agent/test_bedrock_adapter.py:133

This test is now skipped entirely when botocore isn’t installed, which reduces coverage in the default CI environment where botocore is optional. You can keep this assertion running by injecting a minimal fake botocore.session.get_session() into sys.modules with patch.dict, instead of requiring the real package.

    def test_falls_back_to_botocore_profile_region(self):
        pytest.importorskip("botocore")
        from agent.bedrock_adapter import resolve_bedrock_region

tests/agent/test_bedrock_adapter.py:142

With pytest.importorskip("botocore") the failure-path behavior (exceptions from botocore.session.get_session) won’t be exercised in CI where botocore is absent. Consider simulating the module + get_session via patch.dict(sys.modules, ...) and then raising from that stub so the error-handling branch is still covered without the optional dependency.

    def test_botocore_failure_falls_back_to_us_east_1(self):
        pytest.importorskip("botocore")
        from agent.bedrock_adapter import resolve_bedrock_region

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

    def test_defaults_to_us_east_1(self):
+        # `[bedrock]` extra moved out of `[all]` on 2026-05-12 (#24515) and is
+        # now resolved via lazy-install, so botocore can be absent from a
+        # fresh `pip install -e .[all,dev]`. Without this skip the patcher
+        # below fails to resolve `botocore.session` and the test errors.
+        pytest.importorskip("botocore")


…ertions Addresses Copilot review feedback on NousResearch#24601: The original commit used `pytest.importorskip("botocore")` so the three TestResolveBedrocRegion tests would not error when botocore is absent from a fresh `.[all,dev]` install (after `[bedrock]` left `[all]` in NousResearch#24515). Copilot pointed out that this means the CI baseline — where botocore is intentionally absent — no longer asserts the critical `resolve_bedrock_region` fallback behaviour at all; the tests just silently skip. Switch to the same `patch.dict("sys.modules", {"botocore": MagicMock(), "botocore.session": MagicMock()})` pattern already used elsewhere in this file (TestResolveAwsAuthEnvVar, TestHasAwsCredentials). The stub makes `import botocore.session` succeed even without the real package, so the assertions are exercised on every CI install profile, including the baseline that triggered this PR. Verified locally: 112 passed / 6 skipped (the skipped tests are for genuine botocore.exceptions imports, not the region-resolution path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

briandevans · 2026-05-13T00:10:19Z

@copilot Addressed in commit 11496dd:

Switched the three TestResolveBedrocRegion tests from pytest.importorskip("botocore") to the patch.dict("sys.modules", {"botocore": MagicMock(), "botocore.session": MagicMock()}) pattern already used elsewhere in this file (TestResolveAwsAuthEnvVar, TestHasAwsCredentials). The stub makes import botocore.session succeed even without the real package, so the fallback assertions in resolve_bedrock_region now run on the CI baseline that triggered this PR — your point was correct that the skip would have hidden the critical behaviour.

Verified locally with botocore not installed: 112 passed / 6 skipped (skips are unrelated botocore.exceptions tests).

The TestBedrockRegionRouting cases mock `botocore.session.get_session` directly, which fails at import time when botocore isn't installed. With botocore now lazy-installed (removed from `[all]` on 2026-05-12), CI baselines see ModuleNotFoundError on these three tests. Wrap the two failing tests with the same `patch.dict("sys.modules", ...)` guard used for the TestResolveBedrocRegion cases (commit 11496dd), so the patch target resolves whether or not the real package is present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

briandevans · 2026-05-13T01:10:53Z

Extended in commit 5d0059a: applied the same patch.dict("sys.modules", {"botocore": MagicMock(), "botocore.session": MagicMock()}) wrapper to TestBedrockRegionRouting::test_eu_region_from_botocore_profile_yields_eu_models and TestBedrockRegionRouting::test_env_var_takes_priority_over_botocore_profile (same pattern used in 11496dd for the bedrock_adapter cases). These were the two remaining botocore baseline failures.

Note: the test_transcription.py faster_whisper failures are intentionally left out of scope — they're already covered by @CharlieKerfoot's #6375 (broader sys.modules-injection approach).

…t it The 2026-05-12 policy removed boto3 from [all], so CI's default `pip install -e .[all,dev]` no longer pulls boto3 (see NousResearch#24601). The regression tests added for _require_boto3 must therefore be runnable without the real boto3 to actually guard the lazy-install path on every CI run, not just on dev machines that happen to have it. Stub boto3 via `patch.dict(sys.modules, {"boto3": <fake module>})` in both tests. Verified locally that they pass in both states — boto3 installed and fully uninstalled.

briandevans · 2026-05-18T10:14:05Z

Closing to keep the queue clean — happy to reopen if this is still useful.

Copilot AI review requested due to automatic review settings May 12, 2026 23:17

Copilot started reviewing on behalf of briandevans May 12, 2026 23:17 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have labels May 12, 2026

briandevans mentioned this pull request May 13, 2026

test(run_agent): align switch_model test with new clear-on-switch semantics #24776

Closed

3 tasks

ChyuWei mentioned this pull request May 13, 2026

fix(bedrock): route _require_boto3 through lazy_deps.ensure #24972

Closed

13 tasks

alt-glitch mentioned this pull request May 14, 2026

test: keep optional tool tests hermetic #25426

Open

briandevans closed this May 18, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(bedrock,tts): skip baseline tests for optional deps removed from [all]#24601

test(bedrock,tts): skip baseline tests for optional deps removed from [all]#24601
briandevans wants to merge 3 commits into
NousResearch:mainfrom
briandevans:fix/tests-skip-removed-all-extras

briandevans commented May 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

briandevans commented May 13, 2026

Uh oh!

briandevans commented May 13, 2026

Uh oh!

briandevans commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

briandevans commented May 12, 2026

Summary

The fix

Test plan

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

briandevans commented May 13, 2026

Uh oh!

briandevans commented May 13, 2026

Uh oh!

briandevans commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants