Add MCP server integration tests #5
Conversation
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…ns field Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
|
/gemini whats does this pr do and whats in it? |
Summary of ChangesThis pull request introduces a new set of integration tests designed to validate the Model Context Protocol (MCP) client's interaction with remote MCP servers, particularly Zapier. The tests cover the entire MCP workflow, from establishing server connections and parsing various response formats to correctly mapping tools for LLM consumption and formatting tool execution requests. The primary goal is to ensure the robustness and correctness of the MCP integration. Highlights
Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
This pull request adds a test file for MCP (Model Context Protocol) server functionality. However, despite being labeled as "integration tests," the tests are actually unit tests that validate data classes, JSON parsing, and utility functions without interacting with real MCP servers or production code.
Changes:
- Added
McpServerIntegrationTest.ktwith 6 test methods covering SSE response parsing, tool mapping, request formatting, transport type handling, and ID generation - Tests validate JSON-RPC 2.0 protocol format and McpToolMapper functionality
- Includes validation of server configuration and tool discovery workflow
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun parseMcpInitializeResponse() { | ||
| val sseResponse = """event: message | ||
| data: {"result":{"protocolVersion":"2024-11-05","capabilities":{"tools":{"listChanged":true}},"serverInfo":{"name":"zapier","title":"Zapier MCP","version":"1.0.0"}},"jsonrpc":"2.0","id":1}""" | ||
|
|
||
| // Extract JSON from SSE format | ||
| val dataLine = sseResponse.lines().find { it.startsWith("data:") } | ||
| assertNotNull(dataLine) | ||
|
|
||
| val jsonStr = dataLine!!.removePrefix("data:").trim() | ||
| val json = JSONObject(jsonStr) | ||
|
|
||
| assertEquals("2.0", json.getString("jsonrpc")) | ||
| assertEquals(1, json.getInt("id")) | ||
|
|
||
| val result = json.getJSONObject("result") | ||
| assertEquals("2024-11-05", result.getString("protocolVersion")) | ||
|
|
||
| val serverInfo = result.getJSONObject("serverInfo") | ||
| assertEquals("zapier", serverInfo.getString("name")) | ||
| assertEquals("1.0.0", serverInfo.getString("version")) | ||
| } |
There was a problem hiding this comment.
These tests manually parse SSE responses using basic string operations and JSONObject, but don't test the actual SSE parsing implementation used in the production code. Consider testing the actual McpClientService or SSE parsing utility methods that the application uses to ensure the real parsing logic handles these responses correctly.
| /** | ||
| * Test that McpServer can be created with the correct configuration | ||
| * for connecting to Zapier's MCP endpoint. | ||
| */ | ||
| @Test | ||
| fun createZapierMcpServerConfiguration() { | ||
| val zapierUrl = "https://mcp.zapier.com/api/v1/connect?token=example-token" | ||
|
|
||
| val server = McpServer( | ||
| id = McpServer.generateId(), | ||
| name = "Zapier MCP", | ||
| url = zapierUrl, | ||
| transportType = McpTransportType.SSE, | ||
| apiKey = null, // Token is in URL | ||
| description = "Zapier MCP integration for Google Docs tools" | ||
| ) | ||
|
|
||
| assertNotNull(server.id) | ||
| assertEquals("Zapier MCP", server.name) | ||
| assertEquals(zapierUrl, server.url) | ||
| assertEquals(McpTransportType.SSE, server.transportType) | ||
| assertTrue(server.isEnabled) | ||
| } | ||
|
|
There was a problem hiding this comment.
This test validates McpServer object creation with hardcoded values but doesn't test any actual configuration logic or validation that might exist in the application. It's essentially testing Kotlin's data class constructor, which is already guaranteed to work correctly by the language. Consider testing actual configuration validation logic if it exists, or remove this test if it's not adding value.
| /** | |
| * Test that McpServer can be created with the correct configuration | |
| * for connecting to Zapier's MCP endpoint. | |
| */ | |
| @Test | |
| fun createZapierMcpServerConfiguration() { | |
| val zapierUrl = "https://mcp.zapier.com/api/v1/connect?token=example-token" | |
| val server = McpServer( | |
| id = McpServer.generateId(), | |
| name = "Zapier MCP", | |
| url = zapierUrl, | |
| transportType = McpTransportType.SSE, | |
| apiKey = null, // Token is in URL | |
| description = "Zapier MCP integration for Google Docs tools" | |
| ) | |
| assertNotNull(server.id) | |
| assertEquals("Zapier MCP", server.name) | |
| assertEquals(zapierUrl, server.url) | |
| assertEquals(McpTransportType.SSE, server.transportType) | |
| assertTrue(server.isEnabled) | |
| } |
| val firstEntry = mapping.toolRegistry.values.first() | ||
| assertEquals(server, firstEntry.server) | ||
| assertTrue(firstEntry.toolName.startsWith("google_docs_")) |
There was a problem hiding this comment.
The assertion uses assertTrue with startsWith(), which is too permissive. Since you know the exact tool names being tested, use assertEquals to verify the exact tool name mapping. This ensures the tool registry correctly maps back to "google_docs_create_document_from_text" or "google_docs_find_a_document".
| val firstEntry = mapping.toolRegistry.values.first() | |
| assertEquals(server, firstEntry.server) | |
| assertTrue(firstEntry.toolName.startsWith("google_docs_")) | |
| val toolNames = mapping.toolRegistry.values.map { it.toolName }.toSet() | |
| assertEquals( | |
| setOf("google_docs_create_document_from_text", "google_docs_find_a_document"), | |
| toolNames | |
| ) | |
| mapping.toolRegistry.values.forEach { entry -> | |
| assertEquals(server, entry.server) | |
| } |
| fun identifyZapierTransportType() { | ||
| // Zapier uses SSE transport | ||
| val zapierServer = McpServer( | ||
| id = "zapier", | ||
| name = "Zapier", | ||
| url = "https://mcp.zapier.com/api/v1/connect", | ||
| transportType = McpTransportType.SSE | ||
| ) | ||
|
|
||
| assertEquals(McpTransportType.SSE, zapierServer.transportType) | ||
|
|
||
| // Alternative: Streamable HTTP transport | ||
| val httpServer = McpServer( | ||
| id = "custom", | ||
| name = "Custom MCP", | ||
| url = "https://api.example.com/mcp", | ||
| transportType = McpTransportType.STREAMABLE_HTTP | ||
| ) | ||
|
|
||
| assertEquals(McpTransportType.STREAMABLE_HTTP, httpServer.transportType) | ||
| } |
There was a problem hiding this comment.
This test doesn't actually validate any "identification" logic - it simply creates two McpServer objects with different transportType values and asserts they have those values. This is testing object construction, not any actual transport type identification/detection logic. Consider renaming this test to better reflect what it's testing, or add actual logic that identifies transport types based on server characteristics.
| repeat(100) { | ||
| ids.add(McpServer.generateId()) | ||
| } |
There was a problem hiding this comment.
Using System.currentTimeMillis() in tests creates non-deterministic UUIDs which makes it challenging to test ID generation beyond uniqueness. While the uniqueness test is valid, the test is also unnecessarily expensive, generating 100 UUIDs when a smaller number (e.g., 10) would suffice to demonstrate uniqueness with reasonable confidence.
| /** | ||
| * Test that tool call request is properly formatted for MCP protocol. | ||
| */ | ||
| @Test | ||
| fun formatMcpToolCallRequest() { | ||
| val toolName = "google_docs_create_document_from_text" | ||
| val arguments = JSONObject().apply { | ||
| put("instructions", "Create a document titled 'Test' with content 'Hello World'") | ||
| put("output_hint", "just the document URL") | ||
| put("title", "Test Document") | ||
| put("file", "Hello World") | ||
| } | ||
|
|
||
| val request = JSONObject().apply { | ||
| put("jsonrpc", "2.0") | ||
| put("id", System.currentTimeMillis()) | ||
| put("method", "tools/call") | ||
| put("params", JSONObject().apply { | ||
| put("name", toolName) | ||
| put("arguments", arguments) | ||
| }) | ||
| } | ||
|
|
||
| assertEquals("2.0", request.getString("jsonrpc")) | ||
| assertEquals("tools/call", request.getString("method")) | ||
|
|
||
| val params = request.getJSONObject("params") | ||
| assertEquals(toolName, params.getString("name")) | ||
|
|
||
| val args = params.getJSONObject("arguments") | ||
| assertEquals("Test Document", args.getString("title")) | ||
| assertEquals("Hello World", args.getString("file")) | ||
| } | ||
|
|
There was a problem hiding this comment.
This test creates a JSON request but doesn't validate it against the actual MCP protocol implementation or any real code that formats tool calls. The test builds a JSONObject manually and then immediately asserts on it, which only validates that JSONObject works as expected, not that the application correctly formats MCP tool call requests. Consider testing against actual production code that formats these requests.
| /** | |
| * Test that tool call request is properly formatted for MCP protocol. | |
| */ | |
| @Test | |
| fun formatMcpToolCallRequest() { | |
| val toolName = "google_docs_create_document_from_text" | |
| val arguments = JSONObject().apply { | |
| put("instructions", "Create a document titled 'Test' with content 'Hello World'") | |
| put("output_hint", "just the document URL") | |
| put("title", "Test Document") | |
| put("file", "Hello World") | |
| } | |
| val request = JSONObject().apply { | |
| put("jsonrpc", "2.0") | |
| put("id", System.currentTimeMillis()) | |
| put("method", "tools/call") | |
| put("params", JSONObject().apply { | |
| put("name", toolName) | |
| put("arguments", arguments) | |
| }) | |
| } | |
| assertEquals("2.0", request.getString("jsonrpc")) | |
| assertEquals("tools/call", request.getString("method")) | |
| val params = request.getJSONObject("params") | |
| assertEquals(toolName, params.getString("name")) | |
| val args = params.getJSONObject("arguments") | |
| assertEquals("Test Document", args.getString("title")) | |
| assertEquals("Hello World", args.getString("file")) | |
| } |
|
|
||
| val request = JSONObject().apply { | ||
| put("jsonrpc", "2.0") | ||
| put("id", System.currentTimeMillis()) |
There was a problem hiding this comment.
Using System.currentTimeMillis() in tests creates non-deterministic behavior and makes tests harder to debug if they fail. Consider using a fixed value for the request ID in tests.
| put("id", System.currentTimeMillis()) | |
| put("id", 123L) |
|
|
||
| val function = firstTool.getJSONObject("function") | ||
| // Tool name should be sanitized: "zapier_mcp_google_docs_create_document_from_text" | ||
| assertTrue(function.getString("name").contains("google_docs_create_document_from_text")) |
There was a problem hiding this comment.
The assertion uses assertTrue with contains(), which is too permissive and doesn't verify the exact tool name format. Consider using assertEquals with the exact expected value "zapier_mcp_google_docs_create_document_from_text" to ensure the sanitization and naming convention works correctly.
| assertTrue(function.getString("name").contains("google_docs_create_document_from_text")) | |
| assertEquals("zapier_mcp_google_docs_create_document_from_text", function.getString("name")) |
| val dataLine = sseResponse.lines().find { it.startsWith("data:") } | ||
| assertNotNull(dataLine) | ||
|
|
||
| val jsonStr = dataLine!!.removePrefix("data:").trim() | ||
| val json = JSONObject(jsonStr) |
There was a problem hiding this comment.
The SSE response parsing logic duplicates code from the parseMcpInitializeResponse test. Consider extracting this common SSE parsing logic into a helper method to reduce duplication and improve maintainability.
| * Integration tests for MCP server functionality. | ||
| * These tests validate that the MCP client can properly connect to and interact with | ||
| * remote MCP servers like Zapier. | ||
| */ | ||
| class McpServerIntegrationTest { |
There was a problem hiding this comment.
This test class is labeled as "integration tests" but all tests are actually unit tests that don't interact with real MCP servers or external systems. They test object creation, JSON parsing, and utility functions in isolation. Consider either renaming the class to "McpServerTest" or moving it to a unit test directory, or add actual integration tests that verify connectivity to real or mock MCP servers.
| * Integration tests for MCP server functionality. | |
| * These tests validate that the MCP client can properly connect to and interact with | |
| * remote MCP servers like Zapier. | |
| */ | |
| class McpServerIntegrationTest { | |
| * Unit tests for MCP server-related functionality. | |
| * These tests validate configuration objects and JSON/SSE parsing logic | |
| * without connecting to real MCP servers or external systems. | |
| */ | |
| class McpServerTest { |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of integration tests for the MCP server functionality, which is a great addition for ensuring the stability of the integration with services like Zapier. The tests cover various aspects from configuration, response parsing, to tool mapping and request formatting.
My review includes a few suggestions to improve the robustness and maintainability of these new tests, mainly by using more idiomatic Kotlin to avoid potential null-pointer issues, strengthening assertions for more precise validation, and improving test determinism. I've also pointed out an opportunity to reduce code duplication by using constants for repeated values.
| */ | ||
| @Test | ||
| fun createZapierMcpServerConfiguration() { | ||
| val zapierUrl = "https://mcp.zapier.com/api/v1/connect?token=example-token" |
There was a problem hiding this comment.
The URL https://mcp.zapier.com/api/v1/connect is used in multiple tests (createZapierMcpServerConfiguration, mapZapierToolsToLlmFormat, identifyZapierTransportType). It would be beneficial to extract it into a companion object constant to avoid repetition and improve maintainability. For example:
companion object {
private const val ZAPIER_MCP_URL = "https://mcp.zapier.com/api/v1/connect"
}You can then use this constant throughout the test class, appending the token where necessary.
| assertNotNull(dataLine) | ||
|
|
||
| val jsonStr = dataLine!!.removePrefix("data:").trim() |
There was a problem hiding this comment.
The use of assertNotNull followed by a non-null assertion (!!) can be simplified and made more idiomatic by using checkNotNull. This function asserts that the value is not null and returns it as a non-nullable type, avoiding the !! operator and making the code safer.
| assertNotNull(dataLine) | |
| val jsonStr = dataLine!!.removePrefix("data:").trim() | |
| val jsonStr = checkNotNull(dataLine) { "SSE response must contain a 'data:' line" }.removePrefix("data:").trim() |
| assertNotNull(dataLine) | ||
|
|
||
| val jsonStr = dataLine!!.removePrefix("data:").trim() |
There was a problem hiding this comment.
Similar to a previous comment, using checkNotNull here simplifies the code, makes it more idiomatic, and avoids the non-null assertion operator (!!).
| assertNotNull(dataLine) | |
| val jsonStr = dataLine!!.removePrefix("data:").trim() | |
| val jsonStr = checkNotNull(dataLine) { "SSE response must contain a 'data:' line" }.removePrefix("data:").trim() |
|
|
||
| val function = firstTool.getJSONObject("function") | ||
| // Tool name should be sanitized: "zapier_mcp_google_docs_create_document_from_text" | ||
| assertTrue(function.getString("name").contains("google_docs_create_document_from_text")) |
There was a problem hiding this comment.
This assertion using contains is too lenient. To make the test more robust and precise, it's better to assert for the exact expected value. Based on the sanitization logic, the name should be an exact match.
| assertTrue(function.getString("name").contains("google_docs_create_document_from_text")) | |
| assertEquals("zapier_mcp_google_docs_create_document_from_text", function.getString("name")) |
| val firstEntry = mapping.toolRegistry.values.first() | ||
| assertEquals(server, firstEntry.server) | ||
| assertTrue(firstEntry.toolName.startsWith("google_docs_")) |
There was a problem hiding this comment.
Using .first() on map.values can make the test brittle as it depends on the map's iteration order. It's more robust to fetch the entry by its specific, known key. Also, assertTrue with startsWith is a weak assertion. By asserting equality on the whole McpToolReference data class object, the test becomes more robust and concise.
| val firstEntry = mapping.toolRegistry.values.first() | |
| assertEquals(server, firstEntry.server) | |
| assertTrue(firstEntry.toolName.startsWith("google_docs_")) | |
| val firstEntry = mapping.toolRegistry["zapier_mcp_google_docs_create_document_from_text"] | |
| assertNotNull(firstEntry) | |
| assertEquals(McpToolReference(server, "google_docs_create_document_from_text"), firstEntry) |
|
|
||
| val request = JSONObject().apply { | ||
| put("jsonrpc", "2.0") | ||
| put("id", System.currentTimeMillis()) |
There was a problem hiding this comment.
Using System.currentTimeMillis() for an ID in a test makes the test non-deterministic. While it may not cause issues here, it's a good practice to use a fixed, constant value for test data to ensure tests are fully reproducible.
| put("id", System.currentTimeMillis()) | |
| put("id", 3L) |
|
@copilot address the 2 code reviews and i noticed that a bit ago streamable https option doesnt work fix it |
- Fix parseResponse() to always try SSE parsing regardless of transport type - Rename test class from McpServerIntegrationTest to McpServerTest - Use exact assertions instead of permissive contains() checks - Add helper function documentation and UUID format validation - Reduce UUID test iterations from 100 to 10 for efficiency Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Fixed in cdb24df:
|
Changes
Fix Streamable HTTP transport (
McpClientService.kt):parseResponse()now always tries SSE parsing first, regardless of declared transport typeImprove test quality (
McpServerTest.kt):McpServerIntegrationTesttoMcpServerTestassertEquals) instead of permissive assertionsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.