Skip to content

Commit cebccbe

Browse files
authored
[test-improver] Improve tests for mcp tool_result package (#4254)
# Test Improvements: `tool_result_test.go` ## File Analyzed - **Test File**: `internal/mcp/tool_result_test.go` - **Package**: `internal/mcp` - **Lines Added**: +38 ## Improvements Made ### 1. Increased Coverage - ✅ Added test for `ConvertToCallToolResult` when the `content` field is a non-array value (e.g. a JSON string) — triggers the `json.Unmarshal` error fallback that wraps raw bytes as a text content item - ✅ Added test documenting `ParseToolArguments` behavior with `null` JSON input — shows it returns a `nil` map (not empty map) without error - **`ConvertToCallToolResult` coverage**: 93.1% → **100%** - **`ParseToolArguments` coverage**: maintained at 100% - **`NewErrorCallToolResult` coverage**: maintained at 100% ### 2. Cleaner & More Stable Tests - ✅ New tests follow the existing `t.Run()` subtest style in the file (not table-driven, consistent with surrounding code) - ✅ Use `require`/`assert` from testify for precise, idiomatic assertions ## Test Execution All `internal/mcp` tests pass: ``` ok github.com/github/gh-aw-mcpg/internal/mcp 0.744s coverage: 82.0% of statements github.com/github/gh-aw-mcpg/internal/mcp/tool_result.go:15: ConvertToCallToolResult 100.0% github.com/github/gh-aw-mcpg/internal/mcp/tool_result.go:131: ParseToolArguments 100.0% github.com/github/gh-aw-mcpg/internal/mcp/tool_result.go:148: NewErrorCallToolResult 100.0% ``` *Note: `TestFetchAndFixSchema_NetworkError` in `internal/config` is a pre-existing failure unrelated to this PR (network test making a real HTTP request that returns 403 in CI).* ## Why These Changes? `ConvertToCallToolResult` had an untested fallback path: when the response JSON has a `content` field that is not an array (e.g. `{"content": "some string"}`), the typed struct unmarshal fails and the function falls back to wrapping the raw bytes as a text content item. This path was completely untested. The `ParseToolArguments` null-JSON test documents a subtle but important behavior: passing `null` JSON yields a `nil` map (not an empty map), which callers should be aware of. --- *Generated by Test Improver Workflow* *Focuses on better patterns, increased coverage, and more stable tests* > [!WARNING] > <details> > <summary><strong>⚠️ Firewall blocked 1 domain</strong></summary> > > The following domain was blocked by the firewall during workflow execution: > > - `invalidhostthatdoesnotexist12345.com` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "invalidhostthatdoesnotexist12345.com" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details> > Generated by [Test Improver](https://github.com/github/gh-aw-mcpg/actions/runs/24720404496/agentic_workflow) · ● 7.6M · [◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-improver%22&type=pullrequests) <!-- gh-aw-agentic-workflow: Test Improver, engine: copilot, model: auto, id: 24720404496, workflow_id: test-improver, run: https://github.com/github/gh-aw-mcpg/actions/runs/24720404496 --> <!-- gh-aw-workflow-id: test-improver -->
2 parents d61f10e + d4e0ba8 commit cebccbe

1 file changed

Lines changed: 38 additions & 0 deletions

File tree

internal/mcp/tool_result_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,29 @@ func TestConvertToCallToolResult(t *testing.T) {
221221
require.NoError(t, err)
222222
require.NotNil(t, result)
223223
})
224+
225+
// This test exercises the fallback path in ConvertToCallToolResult where
226+
// json.Unmarshal into the typed backendResult struct fails. This happens
227+
// when the "content" field exists but holds a non-array JSON value (e.g. a
228+
// string). The function should fall back to wrapping the raw bytes as text.
229+
t.Run("content field is non-array value falls back to raw text wrap", func(t *testing.T) {
230+
// Use a raw JSON map so we can provide a string-typed "content" field
231+
// that passes the hasContentField check but fails the typed unmarshal.
232+
input := map[string]interface{}{
233+
"content": "this is a string, not an array",
234+
}
235+
236+
result, err := ConvertToCallToolResult(input)
237+
238+
require.NoError(t, err)
239+
require.NotNil(t, result)
240+
assert.Len(t, result.Content, 1)
241+
assert.False(t, result.IsError)
242+
243+
text, ok := result.Content[0].(*sdk.TextContent)
244+
require.True(t, ok, "Expected TextContent fallback")
245+
assert.Contains(t, text.Text, "this is a string, not an array")
246+
})
224247
}
225248

226249
// TestParseToolArguments tests extraction and unmarshaling of tool arguments.
@@ -317,6 +340,21 @@ func TestParseToolArguments(t *testing.T) {
317340
require.NoError(t, err)
318341
assert.Empty(t, args)
319342
})
343+
344+
t.Run("null json arguments returns nil map without error", func(t *testing.T) {
345+
// json.Unmarshal("null", &map) is valid and yields a nil map.
346+
// The function returns (nil, nil) in this case.
347+
req := &sdk.CallToolRequest{
348+
Params: &sdk.CallToolParamsRaw{
349+
Arguments: json.RawMessage(`null`),
350+
},
351+
}
352+
353+
args, err := ParseToolArguments(req)
354+
355+
require.NoError(t, err)
356+
assert.Nil(t, args, "null JSON arguments should yield a nil map")
357+
})
320358
}
321359

322360
// TestNewErrorCallToolResult tests construction of error CallToolResult values.

0 commit comments

Comments
 (0)