Skip to content

Commit 87c9b46

Browse files
authored
[Repo Assist] test(mcp): add connect-timeout default behaviour tests (#3947)
🤖 *This is a Repo Assist automated pull request.* ## Summary Adds four white-box tests in `package mcp` to cover the `connectTimeout` defaulting behaviour in `NewHTTPConnection`. These tests were motivated by the bug in #3933 (inconsistent `== 0` guard in `reconnectSDKTransport`) and serve as regression guards for both fixes. ## Tests Added | Test | What it checks | |------|---------------| | `TestNewHTTPConnection_DefaultConnectTimeout_ZeroInput` | `connectTimeout=0` → stored as `defaultConnectTimeout` (30 s) | | `TestNewHTTPConnection_DefaultConnectTimeout_NegativeInput` | `connectTimeout=-1s` → stored as `defaultConnectTimeout` (guards the `<= 0` fix) | | `TestNewHTTPConnection_DefaultConnectTimeout_CustomValue` | `connectTimeout=10s` → stored unchanged | | `TestDefaultConnectTimeout_Value` | `defaultConnectTimeout == 30*time.Second` — prevents silent drift from `config.DefaultConnectTimeout` | ## Implementation notes - Tests are in `package mcp` (white-box, same as existing `http_connection_test.go`) to allow direct access to the unexported `conn.connectTimeout` field. - A shared `newMinimalTestServer` helper constructs the httptest server needed for `NewHTTPConnection` to complete its handshake. ## Test Status ⚠️ **Infrastructure constraint**: `proxy.golang.org` is blocked in this environment, preventing `go test` from running locally. The tests follow the same pattern as the existing `http_connection_test.go` tests and are written to pass against the companion fix PR (`repo-assist/fix-issue-3933-connect-timeout`). > [!WARNING] > <details> > <summary><strong>⚠️ Firewall blocked 1 domain</strong></summary> > > The following domain was blocked by the firewall during workflow execution: > > - `proxy.golang.org` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "proxy.golang.org" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details> > Generated by [Repo Assist](https://github.com/github/gh-aw-mcpg/actions/runs/24510723428/agentic_workflow) · ● 6.1M · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests) > > To install this [agentic workflow](https://github.com/githubnext/agentics/blob/851905c06e905bf362a9f6cc54f912e3df747d55/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@851905c > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, model: auto, id: 24510723428, workflow_id: repo-assist, run: https://github.com/github/gh-aw-mcpg/actions/runs/24510723428 --> <!-- gh-aw-workflow-id: repo-assist -->
2 parents faabe08 + 9b7bfbc commit 87c9b46

File tree

1 file changed

+100
-0
lines changed

1 file changed

+100
-0
lines changed
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
package mcp
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
8+
"testing"
9+
"time"
10+
11+
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
13+
)
14+
15+
// newMinimalTestServer returns an httptest server that responds with the
16+
// minimal HTTP semantics needed for NewHTTPConnection to complete its SDK
17+
// transport handshake reliably.
18+
func newMinimalTestServer(t *testing.T) *httptest.Server {
19+
t.Helper()
20+
return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
21+
switch r.Method {
22+
case http.MethodGet:
23+
w.Header().Set("Content-Type", "text/event-stream")
24+
w.WriteHeader(http.StatusOK)
25+
return
26+
case http.MethodPost:
27+
resp := map[string]interface{}{
28+
"jsonrpc": "2.0",
29+
"id": 1,
30+
"result": map[string]interface{}{
31+
"protocolVersion": "2024-11-05",
32+
"serverInfo": map[string]interface{}{"name": "test"},
33+
},
34+
}
35+
w.Header().Set("Content-Type", "application/json")
36+
w.Header().Set("Mcp-Session-Id", "test-session")
37+
json.NewEncoder(w).Encode(resp) //nolint:errcheck
38+
return
39+
default:
40+
w.WriteHeader(http.StatusMethodNotAllowed)
41+
return
42+
}
43+
}))
44+
}
45+
46+
// TestNewHTTPConnection_DefaultConnectTimeout_ZeroInput verifies that a zero
47+
// connectTimeout is replaced with defaultConnectTimeout (30 s).
48+
func TestNewHTTPConnection_DefaultConnectTimeout_ZeroInput(t *testing.T) {
49+
srv := newMinimalTestServer(t)
50+
defer srv.Close()
51+
52+
conn, err := NewHTTPConnection(context.Background(), "test", srv.URL,
53+
map[string]string{"Authorization": "test"}, nil, "", 0, 0)
54+
require.NoError(t, err)
55+
require.NotNil(t, conn)
56+
defer conn.Close()
57+
58+
assert.Equal(t, defaultConnectTimeout, conn.connectTimeout,
59+
"zero connectTimeout should be replaced with defaultConnectTimeout")
60+
}
61+
62+
// TestNewHTTPConnection_DefaultConnectTimeout_NegativeInput verifies that a
63+
// negative connectTimeout is also replaced with defaultConnectTimeout.
64+
func TestNewHTTPConnection_DefaultConnectTimeout_NegativeInput(t *testing.T) {
65+
srv := newMinimalTestServer(t)
66+
defer srv.Close()
67+
68+
conn, err := NewHTTPConnection(context.Background(), "test", srv.URL,
69+
map[string]string{"Authorization": "test"}, nil, "", 0, -1*time.Second)
70+
require.NoError(t, err)
71+
require.NotNil(t, conn)
72+
defer conn.Close()
73+
74+
assert.Equal(t, defaultConnectTimeout, conn.connectTimeout,
75+
"negative connectTimeout should be replaced with defaultConnectTimeout")
76+
}
77+
78+
// TestNewHTTPConnection_DefaultConnectTimeout_CustomValue verifies that a
79+
// positive connectTimeout is stored as-is without being replaced.
80+
func TestNewHTTPConnection_DefaultConnectTimeout_CustomValue(t *testing.T) {
81+
srv := newMinimalTestServer(t)
82+
defer srv.Close()
83+
84+
custom := 10 * time.Second
85+
conn, err := NewHTTPConnection(context.Background(), "test", srv.URL,
86+
map[string]string{"Authorization": "test"}, nil, "", 0, custom)
87+
require.NoError(t, err)
88+
require.NotNil(t, conn)
89+
defer conn.Close()
90+
91+
assert.Equal(t, custom, conn.connectTimeout,
92+
"a positive connectTimeout should be stored unchanged")
93+
}
94+
95+
// TestDefaultConnectTimeout_Value guards against the constant value drifting
96+
// away from config.DefaultConnectTimeout (30 s) unintentionally.
97+
func TestDefaultConnectTimeout_Value(t *testing.T) {
98+
assert.Equal(t, 30*time.Second, defaultConnectTimeout,
99+
"defaultConnectTimeout must remain 30 s to stay in sync with config.DefaultConnectTimeout")
100+
}

0 commit comments

Comments
 (0)