test(fetch_tools): add MCP mock server for integration testing#68
Merged
test(fetch_tools): add MCP mock server for integration testing#68
Conversation
Contributor
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'.
Suggested change
| typescript-go | |
| typescript |
Comment on lines
+112
to
+116
|
|
||
| @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.
Suggested change
| @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>
Suggested change
| # 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>
ryoppippi
force-pushed
the
feature/mcp-mock-server-tests
branch
from
48ad92b to
006432d
Compare
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.
- Add automatic pnpm install for vendor/stackone-ai-node in shellHook - Update submodule to latest commit - Fix pnpm version reference (pnpm -> pnpm_10)
ryoppippi
force-pushed
the
feature/mcp-mock-server-tests
branch
from
006432d to
a4367cf
Compare
- Add submodules: true to CI checkout step - Fix shellHook to check for package.json instead of directory - Format tests/mocks/serve.ts with oxfmt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What Changed
vendor/stackone-ai-nodeprovides the MCP mock server implementationpnpm installfor the submodule when neededtests/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 97% with more realistic test coverage of the MCP client code paths.
Testing
All 208 tests pass (26 in test_fetch_tools.py).
Commits
chore: add stackone-ai-node as git submodulebuild(nix): add bun, pnpm, and typescript-go to devShelltest: add MCP mock server infrastructuretest(fetch_tools): rewrite tests to use real MCP mock serverbuild(nix): add shellHook for MCP mock server dependencies