[test] Add tests for mcp.callSDKMethodWithReconnect and reconnectSDKTransport#6956
Merged
Merged
Conversation
Adds 18 comprehensive tests covering: - reconnectSDKTransport: unsupported transport, connection failures, session cleanup - callSDKMethodWithReconnect: happy path, non-session errors, session-not-found reconnect logic - listResources, listPrompts, readResource, getPrompt with real sessions - ServerInfo with and without session - NewHTTPConnection server info population Coverage improvements in internal/mcp: - reconnectSDKTransport: 0% → 100% - callSDKMethodWithReconnect: 33.3% → 100% - listResources: 28.6% → 85.7% - listPrompts: 28.6% → 85.7% - readResource: 50% → 100% - getPrompt: 50% → 100% - ServerInfo: 0% → 85.7% - Package overall: 84.5% → 92.2% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new internal/mcp test suite focused on exercising the SDK reconnect state machine and the SDK-backed method dispatch paths using a minimal streamable-protocol httptest.Server.
Changes:
- Introduces
reconnect_sdk_test.gowith a streamable MCP test server helper and initialize-response helper. - Adds coverage for
reconnectSDKTransportacross unsupported transport, connect failures, and reconnecting with an existing session. - Adds coverage for
callSDKMethodWithReconnectand SDK method dispatch for tools/resources/prompts +ServerInfo()population.
Show a summary per file
| File | Description |
|---|---|
internal/mcp/reconnect_sdk_test.go |
Adds streamable MCP server test harness and reconnect/method-dispatch coverage tests for internal/mcp connections. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 4
Comment on lines
+584
to
+586
| // TestServerInfo_NoSession verifies that ServerInfo returns nil when no SDK session | ||
| // is available (e.g. for a plain JSON-RPC connection that never established a session). | ||
| func TestServerInfo_NoSession(t *testing.T) { |
Comment on lines
+305
to
+307
| func TestCallSDKMethodWithReconnect_Success_NoReconnect(t *testing.T) { | ||
| var reconnectAttempts atomic.Int32 | ||
|
|
Comment on lines
+159
to
+187
| // TestCallSDKMethodWithReconnect_NoReconnectOnNonSessionError verifies that when | ||
| // callSDKMethod returns an error that is not a session-not-found error, | ||
| // callSDKMethodWithReconnect returns it directly without attempting reconnection. | ||
| func TestCallSDKMethodWithReconnect_NoReconnectOnNonSessionError(t *testing.T) { | ||
| // Start a simple server to create a real streamable connection. | ||
| srv := newStreamableMCPTestServer(t, func(w http.ResponseWriter, method string, req map[string]interface{}) { | ||
| if method == "initialize" { | ||
| writeInitializeResponse(w, "session-1", req) | ||
| return | ||
| } | ||
| if method == "notifications/initialized" { | ||
| w.WriteHeader(http.StatusAccepted) | ||
| return | ||
| } | ||
| // Return a server error for any real method call. | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(map[string]interface{}{ //nolint:errcheck | ||
| "jsonrpc": "2.0", | ||
| "id": req["id"], | ||
| "error": map[string]interface{}{"code": -32603, "message": "internal server error"}, | ||
| }) | ||
| }) | ||
| defer srv.Close() | ||
|
|
||
| conn := newStreamableConn(t, srv.URL) | ||
| require.Equal(t, HTTPTransportStreamable, conn.httpTransportType) | ||
|
|
||
| // callSDKMethod for an unsupported method returns a non-session-not-found error. | ||
| result, err := conn.callSDKMethodWithReconnect("unsupported/method", nil) |
Comment on lines
+438
to
+447
| // TestReadResource_WithSession verifies that readResource succeeds with a real SDK session. | ||
| func TestReadResource_WithSession(t *testing.T) { | ||
| srv := newStreamableMCPTestServer(t, func(w http.ResponseWriter, method string, req map[string]interface{}) { | ||
| switch method { | ||
| case "initialize": | ||
| writeInitializeResponse(w, "session-1", req) | ||
| case "notifications/initialized": | ||
| w.WriteHeader(http.StatusAccepted) | ||
| case "resources/read": | ||
| w.Header().Set("Content-Type", "application/json") |
Collaborator
|
@copilot address the review feedback |
Contributor
Addressed in commit Updates include:
|
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.
Test Coverage Improvement: mcp reconnect SDK functions
Function Analyzed
internal/mcpreconnectSDKTransport,callSDKMethodWithReconnect,listResources,listPrompts,readResource,getPrompt,ServerInforeconnectSDKTransport0%,callSDKMethodWithReconnect33.3%, package overall 84.5%reconnectSDKTransport100%,callSDKMethodWithReconnect100%, package overall 92.2%Why These Functions?
reconnectSDKTransportandcallSDKMethodWithReconnectimplement the session reconnect state machine — the most complex, branchy logic ininternal/mcp— yet had 0% and 33.3% coverage respectively.listResources,listPrompts, and related methods had only their error path covered (28.6%), missing the happy path through a real SDK session entirely.Tests Added
reconnectSDKTransport— unsupported transport type (default branch)reconnectSDKTransport— Streamable connect failure (bad URL)reconnectSDKTransport— SSE connect failurereconnectSDKTransport— existing session closed before reconnectcallSDKMethodWithReconnect— happy path (no error)callSDKMethodWithReconnect— non-session-not-found error (no reconnect triggered)callSDKMethodWithReconnect— session-not-found + reconnect failscallSDKMethodWithReconnect— session-not-found + reconnect succeeds + retry succeedslistResources— with real streamable session, empty resultlistPrompts— with real streamable session, empty resultreadResource— with real streamable session (error from server expected)getPrompt— with real streamable session (error from server expected)ServerInfo— with and without sessionNewHTTPConnection— server info population from initialize responseCoverage Report
Test Execution
All 18 tests pass:
Generated by Test Coverage Improver
Next run will target the next most complex under-tested function
Warning
Firewall blocked 8 domains
The following domains were blocked by the firewall during workflow execution:
172.30.0.50go.opentelemetry.iogo.yaml.ingolang.orggoogle.golang.orggopkg.inproxy.golang.orgreleaseassets.githubusercontent.comSee Network Configuration for more information.