[test-improver] Improve tests for mcp HTTP transport - #9667
Conversation
Add three targeted tests for previously uncovered error paths in the mcp package's HTTP transport layer: - TestExecuteHTTPRequest_ConnectionError: verifies that when the backend server is unreachable, executeHTTPRequest returns an error with the expected 'cannot connect to HTTP backend' message (exercises the isHTTPConnectionError true path). - TestExecuteHTTPRequest_BodyReadError: verifies that when the server closes the connection mid-body, executeHTTPRequest returns an error wrapping the io.ReadAll failure. - TestSendHTTPRequest_ReconnectSucceedsButRetryFails: verifies that when a session-not-found response triggers a successful reconnect but the subsequent retry request fails (server closes after reconnect), the error from the retry is returned to the caller. Also adds a newPlainJSONInitServer test helper for bootstrapping plain JSON connections in tests that need to redirect httpURL independently. Coverage improvements: - executeHTTPRequest: 90.5% → 95.2% - sendHTTPRequest: 90.9% → 95.5% - internal/mcp package total: 92.7% → 93.0% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This failure isn’t coming from the PR changes. The |
I investigated job |
There was a problem hiding this comment.
Pull request overview
Adds targeted coverage for HTTP transport error and reconnect paths.
Changes:
- Tests connection and truncated-body failures.
- Tests retry failure after reconnection.
- Adds a plain JSON initialization helper.
Show a summary per file
| File | Description |
|---|---|
internal/mcp/http_transport_test.go |
Expands HTTP transport error-path coverage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| case <-closeAfterReconnect: | ||
| default: | ||
| close(closeAfterReconnect) | ||
| go srv.Close() |
## Summary Addresses the review feedback on #9667 (which was merged before the feedback could be applied). The Copilot reviewer flagged `TestSendHTTPRequest_ReconnectSucceedsButRetryFails` in `internal/mcp/http_transport_test.go` as racy: > This shutdown is racy: starting `srv.Close()` in a goroutine does not guarantee that the listener/current keep-alive connection is closed before `sendHTTPRequest` issues its retry. The retry can therefore reach the handler, receive another 404, and make `require.Error` fail intermittently. ## Fix Following the reviewer's suggestion, the teardown is now deterministic. On the reconnect (second) `initialize` response the handler: - Sets `Connection: close` so the client cannot reuse the keep-alive connection for the retry. - Synchronously closes `srv.Listener` before returning so the retry `tools/list` request cannot reconnect. `srv.Close()` itself can't be called synchronously inside the handler (it blocks on the in-flight connection), so only the listener is closed inline and full server cleanup is deferred. The now-unused `closeAfterReconnect` channel and the `go srv.Close()` goroutine are removed. ## Verification - Target test passes 50× (`-count=50`) and 20× under `-race`. - `make agent-finished` passes (format, build, lint, all Go tests, Rust guard tests). - Both SDK canary tests still pass.
Address review feedback on github#9667. The previous shutdown was racy: starting srv.Close() in a goroutine did not guarantee the listener or current keep-alive connection was closed before sendHTTPRequest issued its retry, so the retry could reach the handler, get another 404, and make require.Error fail intermittently. Make the transition deterministic: on the reconnect initialize, set Connection: close on the response and synchronously close srv.Listener before the handler returns so the retry cannot reuse or reconnect to the server. Normal server cleanup is deferred. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 16dda201-99df-4ddf-8139-e4ee11ef18be
Summary
Adds three targeted tests for previously uncovered error paths in the
internal/mcpHTTP transport layer.File analyzed
internal/mcp/http_transport_test.goImprovements made
New tests
TestExecuteHTTPRequest_ConnectionError— verifies that when the backend server is unreachable,executeHTTPRequestreturns an error with the expected"cannot connect to HTTP backend"message. Exercises theisHTTPConnectionErrortrue path.TestExecuteHTTPRequest_BodyReadError— verifies that when the server closes the connection mid-body (hijacked TCP connection that sends headers then drops),executeHTTPRequestreturns an error wrapping theio.ReadAllfailure.TestSendHTTPRequest_ReconnectSucceedsButRetryFails— verifies that when a session-not-found response triggers a successful reconnect (new session issued), but the subsequent retry request fails because the server closes after the reconnect initialize, the retry error is returned to the caller.New helper
newPlainJSONInitServer— test helper that starts a server handling only the MCPinitializemethod, used to bootstrap plain JSON connections whosehttpURLwill be redirected.Coverage before/after
executeHTTPRequestsendHTTPRequestinternal/mcptotalTest output
All existing tests continue to pass.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
awmgmcpgindex.crates.ioSee Network Configuration for more information.