test(core): increase tool_registry coverage from 47% to 69%#6818
Conversation
Add 19 new tests covering previously untested paths in ToolRegistry: - register_function: type hint inference (int/float/bool/dict/list), required vs optional params, custom name/description, docstring fallback, executor delegation - discover_from_module: @tool decorator pickup, missing-file zero-count, TOOLS dict without tool_executor uses mock executor - has_tool / get_registered_names basic assertions - Session context injection into MCP tool calls via set_session_context() - Execution context override (contextvars) wins over session context - _convert_mcp_tool_to_framework_tool: strips CONTEXT_PARAMS from both properties and required lists - load_mcp_config: list format, dict format, graceful invalid-JSON warning - resync_mcp_servers_if_needed: returns False with no clients; returns False when credentials and ADEN_API_KEY are unchanged Coverage: 47 % → 69 % (+22 pp), all 31 tests pass, ruff clean. Relates to aden-hive#1972
PR Requirements WarningThis PR does not meet the contribution requirements. PR Author: @raiigauravv To fix:
Exception: To bypass this requirement, you can:
Micro-fix requirements (must meet ALL):
Why is this required? See #472 for details. |
github-actions
Bot
added
the
pr-requirements-warning
|
Closing PR because the contribution requirements were not resolved within the 24-hour grace period. |
github-actions
Bot
removed
the
pr-requirements-warning
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded a comprehensive test suite for ToolRegistry, introducing ~404 lines of tests that validate function registration, type-to-JSON-schema inference, module discovery, registry queries, MCP integration and conversion, MCP config loading, and resync behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/tests/test_tool_registry.py (2)
403-407: Please annotate the new test and fake-helper signatures.The added tests/helpers still omit fixture and return annotations (
tmp_path,monkeypatch,caplog, nested fake client/manager methods, etc.). Since this file is already expanding, please bring the new signatures in line with the repo standard; the same pattern repeats throughout the added block.Representative cleanup
+from __future__ import annotations + import logging import textwrap from pathlib import Path from types import SimpleNamespace +import pytest + -def test_register_function_infers_type_hints(): +def test_register_function_infers_type_hints() -> None: @@ -def test_discover_from_module_finds_tool_decorated_functions(tmp_path): +def test_discover_from_module_finds_tool_decorated_functions(tmp_path: Path) -> None: @@ - class FakeClient: - def __init__(self, config): + class FakeClient: + def __init__(self, config: object) -> None: self.config = config @@ -def test_load_mcp_config_handles_invalid_json(tmp_path, caplog): +def test_load_mcp_config_handles_invalid_json( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None:As per coding guidelines,
Use type hints on all function signaturesandUse from __future__ import annotations for modern type syntax.Also applies to: 488-490, 568-576, 626-633, 713-723, 776-785
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/test_tool_registry.py` around lines 403 - 407, The new tests and helper functions (e.g., test_register_function_infers_type_hints, any fake client/manager methods, and helper fixtures that use tmp_path, monkeypatch, caplog) are missing type annotations; add parameter and return type hints to every function signature (e.g., tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, and methods -> None or appropriate return types) and ensure nested fake client/manager methods include their parameter and return annotations; also add from __future__ import annotations at the top of the file to match the repo standard.
717-717: Prefer double-quoted literals for these JSON fixtures.These new payload strings are single-quoted. Switching them to double-quoted/triple-quoted strings—or generating them with
json.dumps()—would keep the added test data aligned with the repo’s Python style.As per coding guidelines,
Use double quotes for strings.Also applies to: 737-737, 779-779
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/test_tool_registry.py` at line 717, Replace the single-quoted JSON fixture string literals with proper double-quoted Python strings (or generate them via json.dumps) so they follow the repo style; specifically update the occurrences of the JSON payloads like '{"servers": [{"name": "s1", "transport": "http", "url": "http://localhost:9000"}]}' (and the two other similar payload strings noted in the comment) to use double-quoted/triple-quoted Python strings or json.dumps(...) instead, keeping the internal JSON quoting intact and ensuring tests still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/tests/test_tool_registry.py`:
- Around line 403-419: Add a test that covers deferred (string) annotations:
create a temporary module (e.g., via types.ModuleType or importlib with source
that begins with "from __future__ import annotations") defining a function with
annotations like "int", "float", "bool", "dict", "list", "str", then call
ToolRegistry().register_function on that function and assert the resulting tool
(use registry.get_tools()["<name>"] and inspect tool.parameters["properties"])
maps to JSON types "integer","number","boolean","object","array","string" just
like test_register_function_infers_type_hints; this ensures
register_function/ToolRegistry handles forward/string annotations correctly.
---
Nitpick comments:
In `@core/tests/test_tool_registry.py`:
- Around line 403-407: The new tests and helper functions (e.g.,
test_register_function_infers_type_hints, any fake client/manager methods, and
helper fixtures that use tmp_path, monkeypatch, caplog) are missing type
annotations; add parameter and return type hints to every function signature
(e.g., tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog:
pytest.LogCaptureFixture, and methods -> None or appropriate return types) and
ensure nested fake client/manager methods include their parameter and return
annotations; also add from __future__ import annotations at the top of the file
to match the repo standard.
- Line 717: Replace the single-quoted JSON fixture string literals with proper
double-quoted Python strings (or generate them via json.dumps) so they follow
the repo style; specifically update the occurrences of the JSON payloads like
'{"servers": [{"name": "s1", "transport": "http", "url":
"http://localhost:9000"}]}' (and the two other similar payload strings noted in
the comment) to use double-quoted/triple-quoted Python strings or
json.dumps(...) instead, keeping the internal JSON quoting intact and ensuring
tests still pass.
🪄 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: c72760d5-66fb-4360-b255-80cbf607c508
📒 Files selected for processing (1)
core/tests/test_tool_registry.py
| def test_register_function_infers_type_hints(): | ||
| """register_function should map Python type annotations to JSON schema types.""" | ||
| registry = ToolRegistry() | ||
|
|
||
| def my_func(a: int, b: float, c: bool, d: dict, e: list, f: str = "x") -> None: | ||
| pass | ||
|
|
||
| registry.register_function(my_func) | ||
|
|
||
| tool = registry.get_tools()["my_func"] | ||
| props = tool.parameters["properties"] | ||
| assert props["a"]["type"] == "integer" | ||
| assert props["b"]["type"] == "number" | ||
| assert props["c"]["type"] == "boolean" | ||
| assert props["d"]["type"] == "object" | ||
| assert props["e"]["type"] == "array" | ||
| assert props["f"]["type"] == "string" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the production code's type annotation handling
fd -t f "tool_registry.py" | grep -v test | head -5Repository: aden-hive/hive
Length of output: 96
🏁 Script executed:
# Check the relevant production code section
cat -n core/framework/runner/tool_registry.py | sed -n '100,140p'Repository: aden-hive/hive
Length of output: 1732
🏁 Script executed:
# Examine the test file structure and imports
head -50 core/tests/test_tool_registry.pyRepository: aden-hive/hive
Length of output: 1736
🏁 Script executed:
# Check if there are any tests that specifically test with deferred annotations
rg "from __future__ import annotations" core/tests/test_tool_registry.py -A 5 -B 5Repository: aden-hive/hive
Length of output: 40
🏁 Script executed:
# Look for any tests that import modules or use string annotations
rg "test.*annotation|test.*forward|test.*future" core/tests/test_tool_registry.py -iRepository: aden-hive/hive
Length of output: 40
🏁 Script executed:
# Check if the test file has any imports of annotations utilities
rg "get_type_hints|typing.get_type_hints" core/tests/test_tool_registry.pyRepository: aden-hive/hive
Length of output: 40
🏁 Script executed:
# See how the production code file imports things at the top
head -30 core/framework/runner/tool_registry.pyRepository: aden-hive/hive
Length of output: 826
🏁 Script executed:
# Check if there's a pattern in the codebase for handling annotations properly
rg "get_type_hints|from __future__ import annotations" core/framework/ --type py | head -20Repository: aden-hive/hive
Length of output: 1517
🏁 Script executed:
# Look at the full test around lines 403-419 to understand context
cat -n core/tests/test_tool_registry.py | sed -n '395,425p'Repository: aden-hive/hive
Length of output: 1336
🏁 Script executed:
# Check if production tool files use deferred annotations (which would break the identity checks)
head -5 core/framework/tools/worker_monitoring_tools.pyRepository: aden-hive/hive
Length of output: 256
🏁 Script executed:
# Let's look at a real tool file to see how it's structured
cat -n core/framework/tools/worker_monitoring_tools.py | sed -n '1,80p'Repository: aden-hive/hive
Length of output: 3745
🏁 Script executed:
# Check if typing.get_type_hints is used anywhere in the codebase for proper annotation handling
rg "get_type_hints" core/framework/Repository: aden-hive/hive
Length of output: 40
🏁 Script executed:
# Verify the actual behavior: when deferred annotations are used, what does inspect.signature return?
python3 << 'EOF'
import inspect
# Case 1: eager annotations (no from __future__)
def eager_func(a: int) -> None:
pass
# Case 2: deferred annotations (simulating what happens with from __future__)
# We need to manually set __annotations__ as strings to simulate deferred behavior
def deferred_func(a: int) -> None:
pass
deferred_func.__annotations__ = {'a': 'int', 'return': 'None'}
print("Eager annotation identity check:")
sig_eager = inspect.signature(eager_func)
param_eager = sig_eager.parameters['a']
print(f" param.annotation: {param_eager.annotation}")
print(f" param.annotation is int: {param_eager.annotation is int}")
print(f" param.annotation == int: {param_eager.annotation == int}")
print("\nDeferred annotation identity check:")
sig_deferred = inspect.signature(deferred_func)
param_deferred = sig_deferred.parameters['a']
print(f" param.annotation: {param_deferred.annotation}")
print(f" param.annotation is int: {param_deferred.annotation is int}")
print(f" param.annotation == int: {param_deferred.annotation == int}")
EOFRepository: aden-hive/hive
Length of output: 315
🏁 Script executed:
# Check if tool_registry.py has any handling for string annotations
rg "typing.get_type_hints|str.*annotation|forward ref" core/framework/runner/tool_registry.pyRepository: aden-hive/hive
Length of output: 40
🏁 Script executed:
# Look at the actual annotations in inspect.signature when deferred
python3 << 'EOF'
import inspect
# Simulate what happens when a tool module with from __future__ import annotations
# registers a function
def tool_func(x: int, y: str) -> None:
pass
# Override __annotations__ to simulate deferred behavior
tool_func.__annotations__ = {'x': 'int', 'y': 'str', 'return': 'None'}
sig = inspect.signature(tool_func)
for param_name, param in sig.parameters.items():
print(f"{param_name}: annotation={param.annotation!r}, type={type(param.annotation)}, is int: {param.annotation is int}")
EOFRepository: aden-hive/hive
Length of output: 167
🏁 Script executed:
# Check if there are any test modules that actually use from __future__ import annotations
rg "from __future__ import annotations" core/tests/ --type pyRepository: aden-hive/hive
Length of output: 2034
🏁 Script executed:
# See if there's any existing test of type inference with string annotations
rg "def test.*type" core/tests/test_tool_registry.py -A 3Repository: aden-hive/hive
Length of output: 221
🏁 Script executed:
# Check if tool_registry.py or any other code handles string annotations via get_type_hints
rg "get_type_hints" core/framework/ --type py -B 2 -A 5Repository: aden-hive/hive
Length of output: 40
🏁 Script executed:
# Verify that tool_registry.py does NOT have from __future__ import annotations
head -20 core/framework/runner/tool_registry.py | grep -E "from __future__|^import"Repository: aden-hive/hive
Length of output: 165
🏁 Script executed:
# Check what the test file imports at the top
head -30 core/tests/test_tool_registry.pyRepository: aden-hive/hive
Length of output: 1089
Test only validates eager-annotation path; will miss schema inference failures with deferred annotations.
test_register_function_infers_type_hints() uses bare runtime annotations, but core/framework/runner/tool_registry.py:108-128 infers schema types via identity checks (param.annotation is int). When a tool module uses from __future__ import annotations (as many production tools do—e.g., worker_monitoring_tools.py), those annotations are strings at runtime. The identity checks fail and silently default to "string" for all parameters.
Add a test case with deferred annotations (e.g., by creating a temporary module with from __future__ import annotations and registering functions from it) to verify the schema mapping works correctly in that scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/tests/test_tool_registry.py` around lines 403 - 419, Add a test that
covers deferred (string) annotations: create a temporary module (e.g., via
types.ModuleType or importlib with source that begins with "from __future__
import annotations") defining a function with annotations like "int", "float",
"bool", "dict", "list", "str", then call ToolRegistry().register_function on
that function and assert the resulting tool (use registry.get_tools()["<name>"]
and inspect tool.parameters["properties"]) maps to JSON types
"integer","number","boolean","object","array","string" just like
test_register_function_infers_type_hints; this ensures
register_function/ToolRegistry handles forward/string annotations correctly.
…e#6818) * test(core): increase tool_registry coverage from 47% to 69% Add 19 new tests covering previously untested paths in ToolRegistry: - register_function: type hint inference (int/float/bool/dict/list), required vs optional params, custom name/description, docstring fallback, executor delegation - discover_from_module: @tool decorator pickup, missing-file zero-count, TOOLS dict without tool_executor uses mock executor - has_tool / get_registered_names basic assertions - Session context injection into MCP tool calls via set_session_context() - Execution context override (contextvars) wins over session context - _convert_mcp_tool_to_framework_tool: strips CONTEXT_PARAMS from both properties and required lists - load_mcp_config: list format, dict format, graceful invalid-JSON warning - resync_mcp_servers_if_needed: returns False with no clients; returns False when credentials and ADEN_API_KEY are unchanged Coverage: 47 % → 69 % (+22 pp), all 31 tests pass, ruff clean. Relates to aden-hive#1972 * style: fix formatting in test_tool_registry.py * fix: accept **kwargs in fake_load_registry to match updated signature --------- Co-authored-by: hundao <alchemy_wimp@hotmail.com>
2 participants
Summary
This PR adds 19 new unit tests for
ToolRegistryincore/tests/test_tool_registry.py, increasing branch coverage from 47% to 69% (+22 pp).What's covered
register_function()@tooldecorator discoverydiscover_from_module()picks up@tool-decorated functionstool_executor→ mock executorhas_tool()/get_registered_names()workspace_id/agent_idfromset_session_context()set_execution_context()wins over session context via contextvars_convert_mcp_tool_to_framework_tool()CONTEXT_PARAMSfrom LLM schema properties and required listload_mcp_config()resync_mcp_servers_if_needed()Falsewith no clients; returnsFalsewhen nothing changedTest results
Ruff: ✅ All checks passed
Relates to #1972
Fix #1972
Summary by CodeRabbit