test(source/alloydb): create MCP integration tests#2843
test(source/alloydb): create MCP integration tests#2843anubhav756 wants to merge 1 commit intomainfrom
Conversation
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 refactors the AlloyDB integration tests to leverage a newly introduced native MCP JSON-RPC harness. This change standardizes how tools are invoked and their responses are processed within the test suite, leading to a more robust and maintainable testing framework. The update ensures that all AlloyDB-related tests interact with the tool endpoints through a consistent and unified interface. Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors several AlloyDB integration tests to use a new tests.InvokeMCPTool helper function and a standardized mcpResp structure for handling tool invocation responses. This includes updating runAlloyDBToolGetTest, runAlloyDBListClustersTest, runAlloyDBListInstancesTest, runAlloyDBGetClusterTest, runAlloyDBGetInstanceTest, runAlloyDBGetUserTest, and TestWaitToolEndpoints. However, several tests (runAlloyDBListUsersTest, runAlloyDBGetClusterTest, runAlloyDBGetInstanceTest, runAlloyDBGetUserTest) were only partially refactored, resulting in compilation errors due to undefined resultStr variables and incorrect usage of the old body.Result field. Additionally, error handling for io.ReadAll is missing in runAlloyDBListClustersTest and runAlloyDBListInstancesTest, which should be addressed for robustness.
ffeb175 to
2bbbe7a
Compare
c4ee2d5 to
feb8ced
Compare
13ad9d5 to
3465675
Compare
78ea17e to
1633f13
Compare
5edc923 to
3510b3a
Compare
c1dc605 to
004eb32
Compare
3510b3a to
c7d0b80
Compare
948e1d9 to
1dd5ab3
Compare
c7d0b80 to
19e1bb2
Compare
1dd5ab3 to
df8a1e1
Compare
19e1bb2 to
67debd9
Compare
df8a1e1 to
5ed2bd1
Compare
67debd9 to
4d26ecd
Compare
5ed2bd1 to
815da44
Compare
c9f38b7 to
31ac3eb
Compare
815da44 to
f2c1e86
Compare
31ac3eb to
f090f91
Compare
f2c1e86 to
62d2c4f
Compare
f090f91 to
5d14d97
Compare
62d2c4f to
d994aa8
Compare
5d14d97 to
8bae0e7
Compare
d994aa8 to
5749645
Compare
| }) | ||
| } | ||
|
|
||
| func TestAlloyDBMCPReadEndpoints(t *testing.T) { |
There was a problem hiding this comment.
why are there TestAlloyDBCallTool vs TestAlloyDBMCPReadEndpoints?
Yuan325
left a comment
There was a problem hiding this comment.
Could you double-check the comments across all test cases? Just want to make sure the updates carried over to all of them. Thank you!
| for _, want := range tc.want { | ||
| if !regexp.MustCompile(want).MatchString(got) { | ||
| t.Errorf("Expected substring not found: %q", want) | ||
| } | ||
| } |
There was a problem hiding this comment.
Are we expecting that all the want will be within got?
Can we keep the original check?: I think in here, we want to make sure that got doesn't return more cluster than it originally should.
var got []string
for _, cluster := range clustersData.Clusters {
got = append(got, cluster.Name)
}
sort.Strings(got)
sort.Strings(tc.want)
if !reflect.DeepEqual(got, tc.want) {
t.Errorf("cluster list mismatch:\n got: %v\nwant: %v", got, tc.want)
}
| name string | ||
| args map[string]any | ||
| want []string | ||
| wantContains string |
There was a problem hiding this comment.
| wantContains string | |
| wantContentErr string |
want vs wantContains feels very confusing.
If you can update for the other tests as well that'll be great, ty!
| name string | ||
| args map[string]any | ||
| want []string | ||
| wantContains string |
There was a problem hiding this comment.
| wantContains string | |
| wantContentErr string |
| if mcpResp.Result.IsError { | ||
| t.Fatalf("returned error result: %v", mcpResp.Result) | ||
| } | ||
| if len(tc.want) > 0 { |
There was a problem hiding this comment.
we might be able to just remove this check since tc.want will always be >0.
| if mcpResp.Result.IsError { | ||
| t.Fatalf("returned error result: %v", mcpResp.Result) | ||
| } | ||
| if len(tc.want) > 0 { |
There was a problem hiding this comment.
we should be able to remove this check since tc.want is always >0
| name string | ||
| args map[string]any | ||
| want map[string]any | ||
| wantContains string |
There was a problem hiding this comment.
| wantContains string | |
| wantContentErr string |
| if mcpResp.Result.IsError { | ||
| t.Fatalf("returned error result: %v", mcpResp.Result) | ||
| } | ||
| if tc.want != nil { |
There was a problem hiding this comment.
might not need to check this based on the test cases that we have.
| name string | ||
| args map[string]any | ||
| want map[string]any | ||
| wantContains string |
There was a problem hiding this comment.
| wantContains string | |
| wantContentErr string |
| if mcpResp.Result.IsError { | ||
| t.Fatalf("returned error result: %v", mcpResp.Result) | ||
| } | ||
| if tc.want != nil { |
There was a problem hiding this comment.
won't need to check this.
| if tc.want != nil { |
| if mcpResp.Result.IsError { | ||
| t.Fatalf("returned error result: %v", mcpResp.Result) | ||
| } | ||
| if tc.want != nil { |
Overview
This PR adds the mapped integration tests for AlloyDB tools using the native MCP harness.
Changes
tests/alloydb/alloydb_mcp_test.goto test database listing and cluster operations over the native/mcppathway.alloydb_integration_test.go.Checklist