[test-improver] Improve tests for internal/server (registerToolsFromBackendContext) - #11107
Conversation
Add TestRegisterToolsFromBackend_HandlerInvocation which invokes the registered tool.Handler closure directly to exercise argument parsing, session auto-initialization, and both the success and malformed-JSON error paths of the closure created in registerToolsFromBackendContext. Also covers the nil Params case (ParseToolArguments defaults to empty arguments). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds direct test coverage for registered backend tool handlers.
Changes:
- Tests valid, malformed, and nil tool arguments.
- Exercises backend invocation and session initialization paths.
Show a summary per file
| File | Description |
|---|---|
internal/server/register_tools_from_backend_test.go |
Adds handler invocation tests using an HTTP mock backend. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
internal/server/register_tools_from_backend_test.go:931
- This does not verify the advertised nil-params default: the backend returns the same constant response regardless of the request, so the test also passes if nil params are translated incorrectly. Record the
tools/callpayload and assert that itsargumentsvalue is the expected empty object.
result, data, err := tool.Handler(context.Background(), req, nil)
require.NoError(err)
assert.NotNil(result)
assert.NotNil(data)
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
| require := require.New(t) | ||
| assert := assert.New(t) |
| result, _, err := tool.Handler(context.Background(), req, nil) | ||
| require.Error(err, "malformed JSON arguments should produce an error") | ||
| assert.NotNil(result, "an error CallToolResult should still be returned") |
| result, data, err := tool.Handler(context.Background(), req, nil) | ||
| require.NoError(err) | ||
| assert.NotNil(result) | ||
| assert.NotNil(data) |
🔒 mcpg Read-Only Stress — gVisorSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS References: §31653312791
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address review feedback |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in commit |
🔒 mcpg Read-Only Stress — default AWFSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS Part B refusals are gateway-enforced ( References: §31717560202
|
🔒 mcpg Read-Only Stress — docker-sbxSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS Run: §31717560169
|
File analyzed
internal/server/register_tools_from_backend_test.goWhy this file
internal/serveris large and central; this test file already used testify (require/assert) consistently, but the tool-registration handler closure created insideregisterToolsFromBackendContext(ininternal/server/tool_registry.go) was never invoked directly by any test — it was only reachable indirectly through full MCP transport/integration tests, leaving its argument-parsing, session-auto-init, and error-handling branches uncovered.[test-improver]PR existed.Improvements made
Added
TestRegisterToolsFromBackend_HandlerInvocation, which registers a real tool from a mock HTTP backend and then calls the returnedtool.Handlerclosure directly with three sub-cases (t.Run):requireSession), and a full round-trip call to the backend viacallBackendTool, asserting a non-nil result/data.mcp.ParseToolArgumentserror path, verifying the handler returns an errorCallToolResultwithout ever reaching the backend.Params— exercises the "no arguments" default path inParseToolArguments.Existing tests and assertions were left untouched (all still use manual
if err != nilchecks only withinhttptestHTTP handler funcs, which run on separate goroutines whererequire/assertcalls are unsafe — this pattern is preserved and called out with a clarifying comment on the new test's handler).Coverage before/after (
internal/serverpackage)registerToolsFromBackendContext: 72.5% → 91.3%Test output