test: hermeticize inference and CLI env-sensitive tests#4
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 30b5af62847ee8d4d519dae829cec9e2fcf99a84 and 3f310cb. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds environment variable cleanup, deterministic monkeypatching, and assertion relaxation across the test suite to reduce flakiness from local developer overrides, implicit environment resolution, and brittle exact-count assertions. ChangesTest Suite Hermeticization and Determinism Improvements
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/conftest.py (1)
356-359: ⚡ Quick winRemove redundant
OPENROUTER_BASE_URLdeletion.
OPENROUTER_BASE_URLis already listed in_CREDENTIAL_NAMES(line 145) and will be removed by the credential filter loop (lines 302-304). The explicit deletion here is redundant and contradicts the comment stating these are env vars "that don't match the generic credential-shaped env-var filter above."The other three variables (
CUSTOM_BASE_URL,OPENCODE_ZEN_BASE_URL,OPENCODE_GO_BASE_URL) are correctly placed here since they're not in_CREDENTIAL_NAMES.♻️ Remove redundant line
monkeypatch.delenv("CUSTOM_BASE_URL", raising=False) -monkeypatch.delenv("OPENROUTER_BASE_URL", raising=False) monkeypatch.delenv("OPENCODE_ZEN_BASE_URL", raising=False) monkeypatch.delenv("OPENCODE_GO_BASE_URL", raising=False)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 356 - 359, The explicit call to monkeypatch.delenv("OPENROUTER_BASE_URL", raising=False) is redundant because OPENROUTER_BASE_URL is included in the _CREDENTIAL_NAMES set and already removed by the credential filter loop (see the loop that iterates over _CREDENTIAL_NAMES); remove only the redundant monkeypatch.delenv("OPENROUTER_BASE_URL", raising=False) line and leave the other deletions for CUSTOM_BASE_URL, OPENCODE_ZEN_BASE_URL, and OPENCODE_GO_BASE_URL in place so the comment about non-credential-shaped env vars remains accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/providers/test_plugin_discovery.py`:
- Around line 49-59: The test_all_profiles_register currently asserts a
hardcoded count (expected_min_profiles) which flakes; instead after calling
_clear_provider_caches() and list_providers() compute discovered names (as you
already do into names) and assert that the set of bundled provider names is a
subset of discovered names—e.g. obtain the canonical bundled list from Providers
(add or call a helper such as providers.get_bundled_provider_names() or
providers.list_bundled_provider_manifests() or read
providers.BUNDLED_PLUGIN_DIR) and replace the len(names) >=
expected_min_profiles assertion with assert
set(bundled_names).issubset(set(names)), keeping references to
test_all_profiles_register, _clear_provider_caches, list_providers and names.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 356-359: The explicit call to
monkeypatch.delenv("OPENROUTER_BASE_URL", raising=False) is redundant because
OPENROUTER_BASE_URL is included in the _CREDENTIAL_NAMES set and already removed
by the credential filter loop (see the loop that iterates over
_CREDENTIAL_NAMES); remove only the redundant
monkeypatch.delenv("OPENROUTER_BASE_URL", raising=False) line and leave the
other deletions for CUSTOM_BASE_URL, OPENCODE_ZEN_BASE_URL, and
OPENCODE_GO_BASE_URL in place so the comment about non-credential-shaped env
vars remains accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38e0ef5b-745a-4131-9b80-f41829cef10b
📥 Commits
Reviewing files that changed from the base of the PR and between c128257 and 30b5af62847ee8d4d519dae829cec9e2fcf99a84.
📒 Files selected for processing (6)
tests/conftest.pytests/hermes_cli/test_gateway_service.pytests/hermes_cli/test_ollama_cloud_auth.pytests/hermes_cli/test_runtime_provider_resolution.pytests/hermes_cli/test_web_server.pytests/providers/test_plugin_discovery.py
| def test_all_profiles_register(): | ||
| """After discovery, the registry must contain at least the expected bundled profiles.""" | ||
| _clear_provider_caches() | ||
| from providers import list_providers | ||
|
|
||
| profiles = list_providers() | ||
| names = sorted(p.name for p in profiles) | ||
| assert len(names) == 33, f"Expected 33 profiles, got {len(names)}: {names}" | ||
| expected_min_profiles = 34 | ||
| assert len(names) >= expected_min_profiles, ( | ||
| f"Expected at least {expected_min_profiles} profiles, got {len(names)}: {names}" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace count-threshold assertion with an invariant over discovered plugin names.
This still behaves like a change-detector test and will flap as provider inventory evolves. Assert that discovered provider names are a superset of bundled plugin directories (or manifest names) instead of enforcing >= 34.
Proposed refactor
def test_all_profiles_register():
- """After discovery, the registry must contain at least the expected bundled profiles."""
+ """After discovery, all bundled provider plugins should be registered."""
_clear_provider_caches()
from providers import list_providers
profiles = list_providers()
names = sorted(p.name for p in profiles)
- expected_min_profiles = 34
- assert len(names) >= expected_min_profiles, (
- f"Expected at least {expected_min_profiles} profiles, got {len(names)}: {names}"
- )
+ plugins_dir = REPO_ROOT / "plugins" / "model-providers"
+ bundled = sorted(
+ p.name for p in plugins_dir.iterdir()
+ if p.is_dir() and (p / "plugin.yaml").exists() and (p / "__init__.py").exists()
+ )
+ missing = sorted(set(bundled) - set(names))
+ assert not missing, f"Bundled providers missing from registry: {missing}"
# Spot-check representative providers from different categories
for required in (As per coding guidelines, “DO NOT write change-detector tests (fail whenever expected-to-change data updates: model catalogs, config version, enumeration counts, hardcoded lists); … write invariant tests instead.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/providers/test_plugin_discovery.py` around lines 49 - 59, The
test_all_profiles_register currently asserts a hardcoded count
(expected_min_profiles) which flakes; instead after calling
_clear_provider_caches() and list_providers() compute discovered names (as you
already do into names) and assert that the set of bundled provider names is a
subset of discovered names—e.g. obtain the canonical bundled list from Providers
(add or call a helper such as providers.get_bundled_provider_names() or
providers.list_bundled_provider_manifests() or read
providers.BUNDLED_PLUGIN_DIR) and replace the len(names) >=
expected_min_profiles assertion with assert
set(bundled_names).issubset(set(names)), keeping references to
test_all_profiles_register, _clear_provider_caches, list_providers and names.
4d7758d to
3f310cb
Compare
* test: hermeticize web server wrapper alias check * test: remove redundant openrouter env cleanup --------- Co-authored-by: Hermes Agent <hermes-agent@users.noreply.github.com>
…y prefixed Companion to the NousResearchGH-25255 incoming-strip fix from @hayka-pacha. Without this, build_anthropic_kwargs unconditionally added 'mcp_' to every tool name in step 3, so a native MCP server tool registered as 'mcp_composio_X' was sent as 'mcp_mcp_composio_X' on the wire. The incoming strip only removes ONE prefix, which still worked on first call, but on subsequent calls the model pattern-matched the single-prefixed form from message history and produced names that stripped to 'composio_X' — registry miss, dispatch fail. The history-rewrite block (#4) already has this guard. Apply the same guard to the schema-rewrite block (#3) so round-trip is symmetric. Added 4 outgoing-side tests. Existing 7 incoming-side tests still pass. Author map: hayka-pacha added for PR NousResearch#25270 salvage attribution. Refs NousResearchGH-25255.
Summary
Validation
Notes
This is a fork PR for test isolation only. Changes are minimal and scoped to tests.
Summary by CodeRabbit