-
Notifications
You must be signed in to change notification settings - Fork 875
discuss: how to surface ClientOptions on convenience constructors #891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package client | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/mark3labs/mcp-go/client/transport" | ||
| ) | ||
|
|
||
| // TestNewStdioClient_AppliesClientOptions exercises the new constructor's | ||
| // promise: every ClientOption passed in the variadic slot is applied to the | ||
| // returned client. WithSession is convenient because it sets a single bool | ||
| // observable from the test. | ||
| func TestNewStdioClient_AppliesClientOptions(t *testing.T) { | ||
| // Use a no-op command for the underlying subprocess; the test only | ||
| // cares that NewStdioClient applies the options before returning. | ||
| c, err := NewStdioClient("true", nil, nil, WithSession()) | ||
| if err != nil { | ||
| t.Fatalf("NewStdioClient: %v", err) | ||
| } | ||
| t.Cleanup(func() { _ = c.Close() }) | ||
| if !c.initialized { | ||
| t.Fatalf("WithSession not applied; initialized=false") | ||
| } | ||
| } | ||
|
|
||
| // TestNewSSEClient_AppliesClientOptions verifies that NewSSEClient routes | ||
| // the variadic ClientOption arguments through to the constructed client | ||
| // while keeping transportOpts separate. | ||
| func TestNewSSEClient_AppliesClientOptions(t *testing.T) { | ||
| c, err := NewSSEClient( | ||
| "http://example.invalid/sse", | ||
| []transport.ClientOption{transport.WithHeaders(map[string]string{"x-test": "1"})}, | ||
| WithSession(), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("NewSSEClient: %v", err) | ||
| } | ||
| if !c.initialized { | ||
| t.Fatalf("WithSession not applied; initialized=false") | ||
| } | ||
| } | ||
|
|
||
| // TestNewStreamableHTTPClient_AppliesClientOptions verifies the same routing | ||
| // for the streamable-http convenience constructor. | ||
| func TestNewStreamableHTTPClient_AppliesClientOptions(t *testing.T) { | ||
| c, err := NewStreamableHTTPClient( | ||
| "http://example.invalid/mcp", | ||
| nil, | ||
| WithSession(), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("NewStreamableHTTPClient: %v", err) | ||
| } | ||
| if !c.initialized { | ||
| t.Fatalf("WithSession not applied; initialized=false") | ||
| } | ||
| } | ||
|
|
||
| // TestNewStreamableHTTPClient_PreservesAutoSession asserts that the | ||
| // existing "transport reports an active session ID → set WithSession" | ||
| // behaviour from NewStreamableHttpClient is preserved on the new | ||
| // constructor. The fixture transport here has no session, so initialized | ||
| // reflects only the caller-supplied opts. | ||
| func TestNewStreamableHTTPClient_PreservesAutoSession(t *testing.T) { | ||
| c, err := NewStreamableHTTPClient("http://example.invalid/mcp", nil) | ||
| if err != nil { | ||
| t.Fatalf("NewStreamableHTTPClient: %v", err) | ||
| } | ||
| if c.initialized { | ||
| t.Fatalf("initialized=true without an active session; auto-session must not fire") | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,3 +20,29 @@ func NewStreamableHttpClient(baseURL string, options ...transport.StreamableHTTP | |||||||||||||||||
| } | ||||||||||||||||||
| return NewClient(trans, clientOptions...), nil | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // NewStreamableHTTPClient creates a new streamable-http-based MCP client | ||||||||||||||||||
| // with the given base URL, applying the provided transport-level options | ||||||||||||||||||
| // when constructing the transport and the provided client-level options | ||||||||||||||||||
| // to the returned client. | ||||||||||||||||||
| // | ||||||||||||||||||
| // Pass transport options (e.g. transport.WithContinuousListening) as a | ||||||||||||||||||
| // slice in transportOpts, and client options (e.g. WithTracer, | ||||||||||||||||||
| // WithPropagator) as the variadic opts. When the transport reports an | ||||||||||||||||||
| // active session ID at construction time, WithSession is appended | ||||||||||||||||||
| // automatically so the returned client skips re-initialisation. | ||||||||||||||||||
| func NewStreamableHTTPClient( | ||||||||||||||||||
| baseURL string, | ||||||||||||||||||
| transportOpts []transport.StreamableHTTPCOption, | ||||||||||||||||||
| opts ...ClientOption, | ||||||||||||||||||
| ) (*Client, error) { | ||||||||||||||||||
| trans, err := transport.NewStreamableHTTP(baseURL, transportOpts...) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return nil, fmt.Errorf("failed to create streamable-http transport: %w", err) | ||||||||||||||||||
| } | ||||||||||||||||||
| clientOpts := opts | ||||||||||||||||||
| if trans.GetSessionId() != "" { | ||||||||||||||||||
| clientOpts = append(clientOpts, WithSession()) | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+43
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid mutating caller-provided option slices. Line 43 aliases Suggested fix- clientOpts := opts
+ clientOpts := append([]ClientOption(nil), opts...)
if trans.GetSessionId() != "" {
clientOpts = append(clientOpts, WithSession())
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| return NewClient(trans, clientOpts...), nil | ||||||||||||||||||
| } | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Align these tests with repository testing conventions.
These tests use
t.Fatalfand duplicated per-case functions; the repo guideline requirestestify/assert+testify/requireand table-driven tests.As per coding guidelines, "Use
testify/assertandtestify/requirefor testing; implement table-driven tests withtests := []struct{ name, ... }."🤖 Prompt for AI Agents