test: refactor integration test framework for full MCP validation#2711
test: refactor integration test framework for full MCP validation#2711anubhav756 wants to merge 6 commits intomainfrom
Conversation
Link Resolution NoteLocal links and directory changes work differently on GitHub than on the docsite. You must ensure fixes pass the GitHub check and also work with Summary
Errors per inputErrors in DEVELOPER.md
|
eaeb55c to
c33e4b4
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the integration test framework to provide more robust and specific validation for Multi-Cloud Platform (MCP) tool invocation endpoints. The changes involve a significant refactoring of test configurations and functions, moving towards a standardized approach for testing MCP tool interactions. By centralizing error handling and introducing dedicated test helpers, the PR aims to improve the clarity, efficiency, and maintainability of the test suite for MCP components. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request migrates tool invocation tests from a direct HTTP API to a new JSON-RPC 2.0 based /mcp endpoint, standardizing error handling and test setup. Documentation in DEVELOPER.md was updated to reflect the new RunMCPToolInvokeTest for tool execution. New helper functions and mock data were introduced in internal/server/mcp_test.go to support the JSON-RPC format, and tests/auth.go now sets default client IDs for test reliability. Existing HTTP integration tests and various tool invocation test functions in tests/tool.go and tests/option.go were refactored to use the new MCP endpoint and JSON-RPC request structure. Review comments highlight that several tests still use the old request body format for the /mcp endpoint, which will cause failures, and point out a typo in requiresClientAuthrorization in mcp_test.go. Additionally, a resource management improvement is suggested to move defer resp.Body.Close() immediately after http.DefaultClient.Do(req) in runRequest for robustness.
I am having trouble creating individual review comments. Click here to see my feedback.
tests/tool.go (1447-1448)
This test and several others below it (e.g., RunPostgresListViewsTest, RunPostgresListSchemasTest) have been updated to use the /mcp endpoint, but the requestBody is still in the old format. The /mcp endpoint expects a JSON-RPC 2.0 request, so these tests will fail.
The request body needs to be wrapped in a proper JSON-RPC structure. For example:
{
"jsonrpc": "2.0",
"id": "some-id",
"method": "tools/call",
"params": {
"name": "list_tables",
"arguments": {
"table_names": ""
}
}
}DEVELOPER.md (184)
This line seems to be a leftover from the previous description and is now inaccurate for RunMCPToolInvokeTest. This test executes a tool, it does not return the tool's manifest.
internal/server/mcp_test.go (1231)
There is a typo in the field name requiresClientAuthrorization. It should be requiresClientAuthorization.
requiresClientAuthorization: true,
internal/server/mcp_test.go (1365-1374)
The defer resp.Body.Close() should be placed immediately after the successful http.DefaultClient.Do(req) call. This ensures that the response body is closed even if subsequent operations like io.ReadAll fail or panic. It's a standard Go idiom for resource management.
resp, err := http.DefaultClient.Do(req)
if err != nil {
return nil, nil, fmt.Errorf("unable to send request: %w", err)
}
defer resp.Body.Close()
respBody, err := io.ReadAll(resp.Body)
if err != nil {
return nil, nil, fmt.Errorf("unable to read request body: %w", err)
}
ea98ec8 to
3240483
Compare
3240483 to
71ff839
Compare
2ef806a to
d3ce2ce
Compare
9ebf274 to
64bffaa
Compare
Yuan325
left a comment
There was a problem hiding this comment.
Why do we want to rename everything with a MCP? Since the entire Toolbox is being updated to MCP anyway, I'm wondering if keeping the existing names might keep things cleaner. I'm slightly concerned that adding 'MCP' to everything might end up being a bit redundant~
| } | ||
|
|
||
| // RunMCPToolInvoke runs the tool invoke endpoint | ||
| func RunMCPToolInvokeTest(t *testing.T, select1Want string, options ...MCPToolInvokeTestOption) { |
There was a problem hiding this comment.
is this specific towards the http source?
e842e1c to
63d049f
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
63d049f to
3ab1865
Compare
3ab1865 to
1e38f7e
Compare
1e38f7e to
33d4249
Compare
Overview
This PR refactors the underlying test methodology across MCP Toolbox to natively support integration testing using MCP JSON-RPC requests.
Changes
RunMCPToolInvokeTest,RunMCPExecuteSqlToolInvokeTest,RunMCPToolInvokeSimpleTest) intests/tool.go.jsonrpc.JSONRPCRequest) to simulate robust MCP payloads./apiHTTP implementation straight to the core/mcppathway.Note
This PR is Part 1 of 5 to fully migrate MCP Toolbox onto MCP endpoints and remove the legacy
/apiREST endpoints.