test(fetch_tools): add MCP mock server for integration testing #68
Conversation
Add the Node.js version of stackone-ai SDK as a submodule in vendor/stackone-ai-node. This provides access to the MCP mock server implementation for testing Python SDK's fetch_tools functionality against a real MCP protocol server.
Add Node.js tooling required to run the MCP mock server for testing: - bun: Fast JavaScript runtime used to run the mock server - pnpm: Package manager for installing Node dependencies - typescript-go: TypeScript compiler
Add pytest fixtures and Hono-based MCP mock server for testing fetch_tools against a real MCP protocol implementation: - tests/conftest.py: Session-scoped fixture that starts bun server automatically when tests require mcp_mock_server - tests/mocks/serve.ts: Standalone HTTP server importing createMcpApp from stackone-ai-node submodule, exposes /mcp and /actions/rpc The server is started once per test session and provides the same tool configurations as the Node SDK tests for consistency.
Replace monkeypatch-based tests with integration tests using the actual MCP protocol via the Hono mock server. This provides more realistic coverage of the fetch_tools() implementation. Changes: - Rename test_toolset_mcp.py to test_fetch_tools.py for clarity - Use mcp_mock_server fixture for most tests (real MCP protocol) - Keep monkeypatch tests only for schema normalisation logic - Add integration marker for MCP server tests - Add tests for MCP headers, tool creation, and RPC execution The tests now exercise the full MCP client flow including _fetch_mcp_tools(), header building, and tool response parsing.
There was a problem hiding this comment.
Pull request overview
This PR replaces monkeypatched test mocks with a real MCP protocol integration test setup. The tests now use an actual MCP mock server imported from the stackone-ai-node submodule, providing end-to-end validation of MCP protocol communication instead of testing stubbed method calls.
Key Changes
- Added stackone-ai-node as a Git submodule to access its MCP mock server implementation
- Created pytest fixtures that automatically start a Bun-based HTTP server for integration tests
- Rewrote all fetch_tools tests to use real MCP protocol communication instead of monkeypatching
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/stackone-ai-node | Added submodule reference to the Node SDK containing the MCP mock server |
| tests/test_toolset_mcp.py | Removed old monkeypatched tests |
| tests/test_fetch_tools.py | New integration tests using real MCP server communication |
| tests/mocks/serve.ts | TypeScript server that imports from the submodule and exposes MCP endpoints |
| tests/conftest.py | Pytest configuration with session-scoped fixture for starting the mock server |
| pyproject.toml | Added integration test marker |
| flake.nix | Added bun, pnpm, and typescript-go to support running the mock server |
| .gitmodules | Configured the stackone-ai-node submodule |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Node.js for MCP mock server | ||
| bun | ||
| pnpm | ||
| typescript-go |
There was a problem hiding this comment.
The package name 'typescript-go' appears unusual for TypeScript tooling. Verify this is the correct package name in nixpkgs. The standard TypeScript package is typically named 'typescript' or 'nodePackages.typescript'.
| typescript-go | |
| typescript |
|
|
||
| @pytest.fixture | ||
| def mcp_server_url(mcp_mock_server: str) -> str: | ||
| """Alias for mcp_mock_server for clearer test naming.""" | ||
| return mcp_mock_server |
There was a problem hiding this comment.
The fixture 'mcp_server_url' is defined but never used in the test files. This creates an unnecessary alias that duplicates the 'mcp_mock_server' fixture without adding value. Consider removing this fixture to reduce code duplication.
| @pytest.fixture | |
| def mcp_server_url(mcp_mock_server: str) -> str: | |
| """Alias for mcp_mock_server for clearer test naming.""" | |
| return mcp_mock_server |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="flake.nix">
<violation number="1" location="flake.nix:99">
P3: Misleading comment: Bun is not Node.js. Consider updating the comment to reflect that Bun (a separate JavaScript runtime) is being used, e.g., `# Bun runtime for MCP mock server`.</violation>
</file>
<file name="tests/conftest.py">
<violation number="1" location="tests/conftest.py:20">
P3: `SO_REUSEADDR` is set after `bind()`, making it ineffective. Socket options must be set before binding. Since this function just finds a free port (binding to port 0), the `SO_REUSEADDR` line is unnecessary and can be removed.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| # security | ||
| gitleaks | ||
|
|
||
| # Node.js for MCP mock server |
There was a problem hiding this comment.
P3: Misleading comment: Bun is not Node.js. Consider updating the comment to reflect that Bun (a separate JavaScript runtime) is being used, e.g., # Bun runtime for MCP mock server.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At flake.nix, line 99:
<comment>Misleading comment: Bun is not Node.js. Consider updating the comment to reflect that Bun (a separate JavaScript runtime) is being used, e.g., `# Bun runtime for MCP mock server`.</comment>
<file context>
@@ -95,6 +95,11 @@
# security
gitleaks
+
+ # Node.js for MCP mock server
+ bun
+ pnpm
</file context>
| # Node.js for MCP mock server | |
| # Bun runtime for MCP mock server |
| """Find a free port on localhost.""" | ||
| with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: | ||
| s.bind(("", 0)) | ||
| s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) |
There was a problem hiding this comment.
P3: SO_REUSEADDR is set after bind(), making it ineffective. Socket options must be set before binding. Since this function just finds a free port (binding to port 0), the SO_REUSEADDR line is unnecessary and can be removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/conftest.py, line 20:
<comment>`SO_REUSEADDR` is set after `bind()`, making it ineffective. Socket options must be set before binding. Since this function just finds a free port (binding to port 0), the `SO_REUSEADDR` line is unnecessary and can be removed.</comment>
<file context>
@@ -0,0 +1,116 @@
+ """Find a free port on localhost."""
+ with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
+ s.bind(("", 0))
+ s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+ return s.getsockname()[1]
+
</file context>
Summary
What Changed
vendor/stackone-ai-nodeprovides the MCP mock server implementationtests/conftest.pywith session-scopedmcp_mock_serverfixturetests/mocks/serve.tsimports from Node SDK and exposes/mcpand/actions/rpctest_toolset_mcp.py→test_fetch_tools.py, now uses real MCP serverWhy
The previous tests used monkeypatch to mock
_fetch_mcp_tools, which didn't test the actual MCP protocol communication. This change provides integration tests that exercise the full flow through the MCP SDK, improving confidence in the implementation.Coverage remains at 90% with more realistic test coverage of the MCP client code paths.
Testing
All 133 tests pass (22 in test_fetch_tools.py).
Summary by cubic
Added an MCP mock server and pytest fixtures to run integration tests for fetch_tools against a real MCP protocol. Replaced monkeypatch-based tests with end-to-end checks for headers, tool creation, and RPC execution.
New Features
Migration
Written for commit 48ad92b. Summary will update automatically on new commits.