fix(test): adapt MCP integration test to rmcp Streamable HTTP protocol - #587
Conversation
The /api/mcp endpoint migrated from plain JSON-RPC to rmcp's StreamableHttpService, which wraps responses in SSE framing and requires a proper session handshake (initialize → capture session ID → notifications/initialized). The old test was sending bare JSON-RPC POSTs without a session ID, causing panics on non-JSON response bodies. Added mcp_request() helper that handles SSE parsing, session ID extraction, and header injection. Updated all 15 /api/mcp test calls (Tests 20-46) to use the new protocol-aware helper. Co-authored-by: Crush (GLM 5.2) <crush@local>
📝 WalkthroughWalkthroughChangesMCP API integration tests
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
apps/desktop/src-tauri/tests/api_test.rs (1)
437-993: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a small helper for repeated
tools/callboilerplate.Every migrated tool-call site repeats the same
jsonrpc/id/method: "tools/call"/params: {name, arguments}envelope plusSome(&mcp_session_id). A thin wrapper would remove this duplication across all ~15 sites and make future protocol changes (e.g., another header) a one-line edit.♻️ Example helper
async fn mcp_tool_call( app: &axum::Router, session_id: &str, id: i64, name: &str, arguments: Value, ) -> (StatusCode, Value) { let (status, _, body) = mcp_request( app, serde_json::json!({ "jsonrpc": "2.0", "id": id, "method": "tools/call", "params": { "name": name, "arguments": arguments } }), Some(session_id), ) .await; (status, body) }This is purely cosmetic/maintainability; functionally the migration is correct.
🤖 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 `@apps/desktop/src-tauri/tests/api_test.rs` around lines 437 - 993, The repeated MCP tools/call request envelope should be centralized in a small helper. Add an async mcp_tool_call helper near the existing mcp_request utility, accepting app, session ID, request ID, tool name, and arguments, then update the migrated tools/call sites to use it while preserving their existing assertions and response handling.
🤖 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.
Nitpick comments:
In `@apps/desktop/src-tauri/tests/api_test.rs`:
- Around line 437-993: The repeated MCP tools/call request envelope should be
centralized in a small helper. Add an async mcp_tool_call helper near the
existing mcp_request utility, accepting app, session ID, request ID, tool name,
and arguments, then update the migrated tools/call sites to use it while
preserving their existing assertions and response handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c70cb3f3-cf11-4acd-9dca-d4d552c84842
📒 Files selected for processing (1)
apps/desktop/src-tauri/tests/api_test.rs
Summary
The
/api/mcpendpoint migrated from plain JSON-RPC to rmcp'sStreamableHttpService, which wraps responses in SSE framing and requires a proper session handshake. The integration test (test_rust_backend_api) was still sending bare JSON-RPC POSTs without a session ID, causing panics whenserde_json::from_slicehit non-JSON response bodies.Changes
mcp_request()helper that:initializewithprotocolVersion,capabilities, andclientInfomcp-session-idfrom the response headernotifications/initializedto complete the handshaketext/event-stream) and plain JSON responses/api/mcptest calls (Tests 20–46) to use the protocol-aware helperTest plan
cargo test --test api_testpasses (all 46 sub-tests)Summary by CodeRabbit