Skip to content

Commit 750b010

Browse files
authored
[test-improver] Improve tests for server/http_server (#5495)
# Test Improvements: `http_server_test.go` ## File Analyzed - **Test File**: `internal/server/http_server_test.go` - **Package**: `internal/server` ## Improvements Made ### 1. Better Testing Patterns - ✅ Converted `TestNewHTTPServer` from a single flat test to a table-driven test covering four distinct address formats (`host:port`, `:port`, `127.0.0.1:0`, empty string) - ✅ Added descriptive subtest names for each scenario - ✅ Used `t.Setenv` for environment variable control in timeout test ### 2. Increased Coverage — `buildMCPHTTPServer` contract coverage - ✅ `TestBuildMCPHTTPServer_ReturnsServerWithCorrectAddr` — verifies the returned server is bound to the requested address - ✅ `TestBuildMCPHTTPServer_RouteBuilderIsCalled` — verifies the routeBuilder callback is actually invoked - ✅ `TestBuildMCPHTTPServer_RouteBuilderReceivesSessionTimeout` — verifies the session timeout from `MCP_GATEWAY_SESSION_TIMEOUT` env var is passed through to the routeBuilder - ✅ `TestBuildMCPHTTPServer_CustomRouteFromBuilder` — verifies routes registered inside the routeBuilder callback are accessible through the built server's handler ### 3. Follow-up review feedback addressed - ✅ Removed redundant endpoint tests that duplicated existing `/health` and OAuth discovery coverage in other server test files - ✅ Kept only `buildMCPHTTPServer`-specific contract assertions to reduce duplicated maintenance surface ### 4. Cleaner &amp; More Stable Tests - ✅ Each test is fully isolated with its own `UnifiedServer` and `t.Cleanup` for teardown - ✅ Uses `httptest.NewRecorder` / `httptest.NewRequest` — no real ports opened - ✅ No timing dependencies ## Test Execution Targeted and full validation pass: - `go test ./internal/server -run 'TestNewHTTPServer|TestBuildMCPHTTPServer'` - `make agent-finished` ## Why These Changes? `http_server_test.go` originally had minimal coverage. This PR improves direct coverage of `newHTTPServer` and `buildMCPHTTPServer`-specific contracts (address propagation, routeBuilder invocation, session-timeout forwarding, and custom route wiring) while removing duplicate endpoint assertions already covered elsewhere. --- *Generated by Test Improver Workflow* *Focuses on better patterns, increased coverage, and more stable tests* &gt; Generated by <a href="https://github.com/github/gh-aw-mcpg/actions/runs/25704663772/agentic_workflow">Test Improver</a> · ● 3.9M · <a href="https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-improver%22&amp;type=pullrequests">◷</a>
2 parents 93a0095 + 8e493e8 commit 750b010

1 file changed

Lines changed: 104 additions & 4 deletions

File tree

Lines changed: 104 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,118 @@
11
package server
22

33
import (
4+
"context"
45
"net/http"
6+
"net/http/httptest"
57
"testing"
8+
"time"
69

710
"github.com/stretchr/testify/assert"
811
"github.com/stretchr/testify/require"
12+
13+
"github.com/github/gh-aw-mcpg/internal/config"
914
)
1015

1116
func TestNewHTTPServer(t *testing.T) {
12-
handler := http.NewServeMux()
17+
tests := []struct {
18+
name string
19+
addr string
20+
handler http.Handler
21+
}{
22+
{
23+
name: "host:port address",
24+
addr: "127.0.0.1:1234",
25+
handler: http.NewServeMux(),
26+
},
27+
{
28+
name: "port-only address",
29+
addr: ":8080",
30+
handler: http.NewServeMux(),
31+
},
32+
{
33+
name: "zero port",
34+
addr: "127.0.0.1:0",
35+
handler: http.NewServeMux(),
36+
},
37+
{
38+
name: "empty address",
39+
addr: "",
40+
handler: http.NewServeMux(),
41+
},
42+
}
43+
44+
for _, tt := range tests {
45+
t.Run(tt.name, func(t *testing.T) {
46+
server := newHTTPServer(tt.addr, tt.handler)
47+
require.NotNil(t, server)
48+
assert.Equal(t, tt.addr, server.Addr)
49+
assert.Same(t, tt.handler, server.Handler)
50+
})
51+
}
52+
}
53+
54+
// TestBuildMCPHTTPServer_ReturnsServerWithCorrectAddr verifies that buildMCPHTTPServer
55+
// returns an http.Server bound to the requested address.
56+
func TestBuildMCPHTTPServer_ReturnsServerWithCorrectAddr(t *testing.T) {
57+
us, err := NewUnified(context.Background(), &config.Config{})
58+
require.NoError(t, err)
59+
t.Cleanup(func() { us.Close() })
60+
61+
const addr = "127.0.0.1:0"
62+
server := buildMCPHTTPServer(addr, us, "", "", func(_ *http.ServeMux, _ time.Duration) {})
1363

14-
server := newHTTPServer("127.0.0.1:1234", handler)
1564
require.NotNil(t, server)
16-
assert.Equal(t, "127.0.0.1:1234", server.Addr)
17-
assert.Same(t, handler, server.Handler)
65+
assert.Equal(t, addr, server.Addr)
66+
}
67+
68+
// TestBuildMCPHTTPServer_RouteBuilderIsCalled verifies that buildMCPHTTPServer
69+
// invokes the supplied routeBuilder callback.
70+
func TestBuildMCPHTTPServer_RouteBuilderIsCalled(t *testing.T) {
71+
us, err := NewUnified(context.Background(), &config.Config{})
72+
require.NoError(t, err)
73+
t.Cleanup(func() { us.Close() })
74+
75+
called := false
76+
buildMCPHTTPServer("127.0.0.1:0", us, "", "", func(_ *http.ServeMux, _ time.Duration) {
77+
called = true
78+
})
79+
80+
assert.True(t, called, "routeBuilder should be called by buildMCPHTTPServer")
81+
}
82+
83+
// TestBuildMCPHTTPServer_RouteBuilderReceivesSessionTimeout verifies that
84+
// the session timeout passed to routeBuilder reflects the environment variable.
85+
func TestBuildMCPHTTPServer_RouteBuilderReceivesSessionTimeout(t *testing.T) {
86+
us, err := NewUnified(context.Background(), &config.Config{})
87+
require.NoError(t, err)
88+
t.Cleanup(func() { us.Close() })
89+
90+
t.Setenv("MCP_GATEWAY_SESSION_TIMEOUT", "15m")
91+
92+
var capturedTimeout time.Duration
93+
buildMCPHTTPServer("127.0.0.1:0", us, "", "", func(_ *http.ServeMux, sessionTimeout time.Duration) {
94+
capturedTimeout = sessionTimeout
95+
})
96+
97+
assert.Equal(t, 15*time.Minute, capturedTimeout)
98+
}
99+
100+
// TestBuildMCPHTTPServer_CustomRouteFromBuilder verifies that routes registered
101+
// inside the routeBuilder callback are accessible via the returned server's handler.
102+
func TestBuildMCPHTTPServer_CustomRouteFromBuilder(t *testing.T) {
103+
us, err := NewUnified(context.Background(), &config.Config{})
104+
require.NoError(t, err)
105+
t.Cleanup(func() { us.Close() })
106+
107+
server := buildMCPHTTPServer("127.0.0.1:0", us, "", "", func(mux *http.ServeMux, _ time.Duration) {
108+
mux.HandleFunc("/custom-test-route", func(w http.ResponseWriter, _ *http.Request) {
109+
w.WriteHeader(http.StatusTeapot)
110+
})
111+
})
112+
113+
req := httptest.NewRequest(http.MethodGet, "/custom-test-route", nil)
114+
rr := httptest.NewRecorder()
115+
server.Handler.ServeHTTP(rr, req)
116+
117+
assert.Equal(t, http.StatusTeapot, rr.Code)
18118
}

0 commit comments

Comments
 (0)