mcp: add tests for UnmarshalJSON handling of nil Content pointers #275
Conversation
- Introduces a new test file `content_nil_test.go` which verifies that `UnmarshalJSON` methods for various `Content` types do not panic when unmarshaling onto `nil` pointers. - Adds a `nil` check in `contentFromWire` function to guard against a `nil` `wire.Content` parameter. - Tests cover different scenarios, including valid and invalid content types, as well as cases with empty or missing content fields. For [modelcontextprotocol#205](modelcontextprotocol#205)
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
Thanks! Appreciate the tests (and fix?).
Just think we should update this to use cmp.Diff. I can merge and then do that myself if you prefer.
mcp/content_nil_test.go
Outdated
| } | ||
|
|
||
| // Verify that the Content field was properly populated | ||
| switch v := tt.content.(type) { |
There was a problem hiding this comment.
I think you can simplify this significantly using cmp.Diff: just compare the expected and actual value.
https://pkg.go.dev/github.com/google/go-cmp/cmp is already required by this module.
| } | ||
|
|
||
| func contentFromWire(wire *wireContent, allow map[string]bool) (Content, error) { | ||
| if wire == nil { |
There was a problem hiding this comment.
The test failed without this change, right?
There was a problem hiding this comment.
That's correct. One of the tests that I added failed without this change.
The test is unmarshaling {"model":"test","role":"user"} (missing "content"), which causes the *wireContent parameter here to be nil.
Below is the test output without this change:
$ go test ./mcp
--- FAIL: TestContentUnmarshalNilWithEmptyContent (0.00s)
--- FAIL: TestContentUnmarshalNilWithEmptyContent/Missing_Content_field (0.00s)
content_nil_test.go:170: UnmarshalJSON panicked: runtime error: invalid memory address or nil pointer dereference
FAIL
FAIL github.com/modelcontextprotocol/go-sdk/mcp 7.532s
FAIL
- Added expected output values for various `Content` types in `TestContentUnmarshalNil`. - Changed to use the `cmp` package to simplify assertions.
2 participants
Check that UnmarshalJSON methods for Content don't panic on nil.
content_nil_test.gowhich verifies thatUnmarshalJSONmethods for variousContenttypes do not panic when unmarshaling ontonilpointers.nilcheck incontentFromWirefunction to guard against anilwire.Contentparameter.For modelcontextprotocol#205