Replace mocked MCP tests with real integration tests #1678
Conversation
- Remove 8 mocked tests that used patch('openhands.sdk.mcp.utils.MCPClient')
- Add MCPTestServer helper class using FastMCP to spin up real HTTP/SSE servers
- Add http_mcp_server and sse_mcp_server pytest fixtures
- Add real integration tests for HTTP and SSE MCP connections
- Add tests for tool execution, schema validation, and transport inference
- Keep timeout error message test mocked (documented why)
Co-authored-by: openhands <[email protected]>
Coverage Report •
|
Co-authored-by: openhands <[email protected]>
- Add MCPTransport type alias for Literal transport values - Use .get() for TypedDict access to avoid reportTypedDictNotRequiredAccess Co-authored-by: openhands <[email protected]>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review Summary
Overall this is a good improvement replacing mocks with real integration tests. The approach using FastMCP servers is solid. However, there are several reliability and cleanup issues that should be addressed to make the tests more robust and less flaky.
Critical Issues
1. Hardcoded sleep causes flaky tests (Line 64)
The time.sleep(1.5) is fragile and can cause test failures on slower machines or under load. Recommendation: Implement a retry loop that polls the port for availability:
self._server_thread.start()
# Wait for server to be ready by polling the port
for _ in range(30): # 3 seconds max with 0.1s intervals
time.sleep(0.1)
try:
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
s.connect(("127.0.0.1", self.port))
break
except (socket.error, ConnectionRefusedError):
continue
else:
raise RuntimeError(f"Server failed to start on port {self.port}")
return self.port2. Silent exception handling hides errors (Lines 59-60, 303-304)
Catching and silently ignoring all exceptions makes debugging difficult when tests fail.
Line 59-60: The server thread silently swallows exceptions, hiding server startup failures. Consider logging or re-raising, and add proper loop cleanup:
try:
loop.run_until_complete(run_server())
except Exception as e:
import logging
logging.error(f"MCP test server failed: {e}")
raise
finally:
loop.close()Line 303-304: The broad exception handler makes the test pass even if unexpected errors occur. Be more specific:
try:
tools = create_mcp_tools(config, timeout=5.0)
assert len(tools) == 0 # No tools from failed connection
except (ConnectionError, TimeoutError, MCPTimeoutError):
pass # Expected connection errors are acceptable3. No proper cleanup of server resources (Lines 67-69)
The empty stop() method relies on daemon threads cleaning up automatically, which is unreliable. Consider implementing proper shutdown:
def stop(self):
"""Stop the server and clean up resources."""
if self._server_thread and self._server_thread.is_alive():
# Signal the server to stop (FastMCP should have a shutdown method)
# For now, daemon thread will clean up, but this should be improved
self._server_thread = None
self.port = NoneEven better would be to store a reference to the FastMCP app and call its shutdown method if available.
Minor Issues
4. Race condition in port allocation (Line 42)
There's a race condition where another process could bind to the port between _find_free_port() returning and the server starting. While rare, this can cause flaky test failures.
Consider either:
- Accepting the rare collision and retrying on bind failure
- Binding the socket in
_find_free_port()and passing the bound socket to FastMCP (if supported) - Using a test-specific port range (e.g., 50000-60000) to minimize conflicts
This is a known limitation in test infrastructure and may be acceptable with retry logic elsewhere.
5. Long timeout values (Line 130 and others)
Most tests use timeout=30.0 which seems quite long. If tests fail near this timeout, the test suite becomes very slow.
Consider:
- Reducing to 5-10 seconds for tests expected to succeed quickly
- Only using 30+ seconds for the stdio test (line 314) which downloads packages
- Document why specific timeouts are needed if they must be long
Good Practices Noted
✅ Line 314: The 120-second timeout is appropriate and well-documented since uvx may need to download packages in CI environments.
✅ Line 365-369: Good documentation explaining why the timeout test remains mocked (avoiding slow/flaky real timeouts).
|
@OpenHands fix these issues |
|
I'm on it! neubig can track my progress at all-hands.dev |
- Replace hardcoded sleep(1.5) with HTTP-based port polling for reliability - Add logging for server startup errors instead of silently swallowing exceptions - Implement proper cleanup in stop() method (reset thread and port references) - Use specific exception types in test_create_mcp_tools_connection_to_nonexistent_server - Reduce timeout values from 30s to 10s for tests expected to succeed quickly - Keep 120s timeout for stdio test (uvx may need to download packages in CI) Co-authored-by: openhands <[email protected]>
|
I've addressed all the review feedback from the PR #1678. Here's a summary of the changes: Changes Made1. ✅ Fixed hardcoded sleep (Critical Issue #1)
2. ✅ Fixed silent exception handling (Critical Issue #2)
3. ✅ Fixed broad exception handler (Critical Issue #2 - Line 303-304)
4. ✅ Implemented proper cleanup in stop() method (Critical Issue #3)
5. ✅ Reduced timeout values (Minor Issue #5)
Verification
The changes have been pushed to the |
Summary
This PR replaces the mocked MCP tests with real integration tests that spin up actual MCP servers using FastMCP.
Changes
Removed
patch("openhands.sdk.mcp.utils.MCPClient")Added
MCPTestServerhelper class that usesFastMCPto spin up real HTTP/SSE servers in background threadshttp_mcp_serverfixture - provides HTTP MCP server withgreetandadd_numberstoolssse_mcp_serverfixture - provides SSE MCP server withechoandmultiplytoolsNew Integration Tests (12 total)
test_create_mcp_tools_empty_config- error handling for empty configtest_create_mcp_tools_http_server- HTTP connection + tool discoverytest_create_mcp_tools_sse_server- SSE connection + tool discoverytest_create_mcp_tools_mixed_servers- multiple servers simultaneouslytest_create_mcp_tools_http_schema_validation- verify schemas are loaded correctlytest_create_mcp_tools_transport_inferred_from_url- HTTP auto-detectiontest_create_mcp_tools_sse_inferred_from_url- SSE auto-detection from/ssein URLtest_execute_http_tool- full round-trip tool execution over HTTPtest_execute_sse_tool- full round-trip tool execution over SSEtest_create_mcp_tools_connection_to_nonexistent_server- graceful failure handlingtest_create_mcp_tools_stdio_server- existing stdio test (unchanged)test_create_mcp_tools_timeout_error_message- timeout error formatting (kept mocked, documented why)Notes
test_create_mcp_tools_timeout_error_message, since testing real timeouts would be slow and flaky. This is documented in the test's docstring.@neubig can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:5dd9fea-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5dd9fea-python) is a multi-arch manifest supporting both amd64 and arm645dd9fea-python-amd64) are also available if needed