Conversation
We also set the defaults in the client.go before sending them to the server. However, we might be talking to a client that is not doing this so also set the defaults within the server.
mcp/mcp_test.go
Outdated
| name string | ||
| schema *jsonschema.Schema | ||
| expectedKey string | ||
| expectedValue any |
There was a problem hiding this comment.
Maybe just have want map[string]any, and use cmp.Diff to compare with the content we got?
That allows for more complex tests, and (I think) simplifies the test body.
There was a problem hiding this comment.
Great idea! I liked that this simplified things and even handled the float comparison out of the box
mcp/mcp_test.go
Outdated
| expectedKey: "key", | ||
| expectedValue: "one", | ||
| }, | ||
| // TODO: should we have a test with number (float) types? |
mcp/mcp_test.go
Outdated
| } | ||
| defer cs.Close() | ||
|
|
||
| schemas := []struct { |
There was a problem hiding this comment.
'tests'? More than just schemas.
There was a problem hiding this comment.
good point, renamed to testcases
|
PTAL |
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
LGTM, pending perhaps a couple more test cases.
We are adding support for defaults for non-boolean types, this adds tests for invalid values for these new default types.
|
PTAL, I updated the server side to both apply defaults and do validation. I also removed the use of reflect as discussed offline (thanks for the help!) |
| if err := remarshal(wireSchema, &schema); err != nil { | ||
| return nil, err | ||
| } | ||
| if schema == nil { |
There was a problem hiding this comment.
This is possible? wireShema is non-nil, so wouldn't a nil schema be an error here?
There was a problem hiding this comment.
TL;DR: I think we either need a reflect.IsNil() call on wireSchema to determine if it is really nil, or we need to check if the result of remarshall is nil when err is != nil (which feels gross).
In more detail:
In the server, as I understand, the wireschema is **jsonschema. we pass in a nil value for this which gets wrapped in another pointer. so, in the server, wireschema is non-nil but *wireschema is nil. This means we end up passing a non-nil wireschema here, but it is unmarshalling a nil jsonschema.
This means that when we get back here since we passed &schema into remarshall now schema is nil.
It might make remarshall more robust to have a reflect isNil call and return an err, but then we'd need to add in the reflect call.
There was a problem hiding this comment.
On the server the schema should be a *jsonschema.Schema, not a **jsonschema.Schema. Where are you seeing this?
Let's merge this as-is, and I will take a look.
There was a problem hiding this comment.
BTW, I looked into it: the test uses a typed nil (one of the downsides of having any for the schema).
I think the code, as written, is fine.
|
|
||
| resolved, err := schema.Resolve(nil) | ||
| if err != nil { | ||
| fmt.Printf(" resolve err: %s", err) |
3 participants
#617