Repository navigation
test(source/cassandra): add MCP integration tests - #4194
arijitbakshi15-stack wants to merge 2 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests for Cassandra MCP tools in tests/cassandra/cassandra_mcp_test.go, covering tool listing and invocation. Feedback on the changes highlights that the generated table name tableNameTemplateParam exceeds Cassandra's 48-character limit due to its long prefix and UUID, and suggests shortening the prefix to avoid table creation failures.
|
|
||
| paramTableName := "param_table_" + strings.ReplaceAll(uuid.New().String(), "-", "") | ||
| tableNameAuth := "auth_table_" + strings.ReplaceAll(uuid.New().String(), "-", "") | ||
| tableNameTemplateParam := "template_param_table_" + strings.ReplaceAll(uuid.New().String(), "-", "") |
There was a problem hiding this comment.
In Cassandra, table names have a maximum length limit of 48 characters. The current table name tableNameTemplateParam is constructed as "template_param_table_" (21 chars) plus a 32-character UUID, resulting in a 53-character string. This will cause table creation to fail on a real Cassandra instance.
Consider shortening the prefix to "tmpl_param_" (11 characters) to keep the total length at 43 characters, which is safely within the 48-character limit.
| tableNameTemplateParam := "template_param_table_" + strings.ReplaceAll(uuid.New().String(), "-", "") | |
| tableNameTemplateParam := "tmpl_param_" + strings.ReplaceAll(uuid.New().String(), "-", "") |
|
/gcbrun |
Add Cassandra integration tests for the /mcp endpoint in a new file, following the convention in googleapis#2922. - tests/cassandra/cassandra_mcp_test.go adds TestCassandraMCPListTools and TestCassandraMCPCallTool, which start the server without --enable-api. - List tools verifies the full tools/list manifest using GetBaseMCPExpectedTools and GetTemplateParamMCPExpectedTools. Cassandra registers no execute-sql tool, so the execute-sql entries are not part of its manifest. - Call tool reuses the existing helpers with WithMCP() and WithMCPTemplate(), so the same cases run over both endpoints. The Cassandra-specific options are carried over unchanged, including the disabled select-1 cases, since CQL has no SELECT 1. - Teardown is registered with t.Cleanup so nothing leaks when setup fails partway through, and the session is closed last. The legacy /api test file is unchanged. Part of googleapis#3706
d9a7b9b to
4672f4f
Compare
|
/gcbrun |
Cassandra limits table names to 48 characters. The legacy prefix "template_param_table_" (21 chars) plus a 32-character UUID is 53 characters, so use a shorter prefix for the template parameter table. The cassandra shard passed with the longer name, so this is defensive rather than a fix for an observed failure. Part of googleapis#3706
This branch has not been deployed
4 participants
Description
Adds Cassandra integration tests for the
/mcpendpoint in a new file, following the convention in #2922. Part of #3706.tests/cassandra/cassandra_mcp_test.goaddsTestCassandraMCPListToolsandTestCassandraMCPCallTool. Both start the Toolbox server without--enable-api.TestCassandraMCPListToolsverifies the completetools/listmanifest, built fromGetBaseMCPExpectedTools()andGetTemplateParamMCPExpectedTools()(15 tools). Cassandra registers no execute-sql tool, so unlike the other sources in this series its manifest does not includeGetExecuteSQLMCPExpectedTools().TestCassandraMCPCallToolreuses the existing helpers with the options method, so the same cases run over both endpoints:RunToolInvokeTestwithWithMCP(),RunToolInvokeWithTemplateParameterswithWithMCPTemplate(), andRunMCPToolCallMethod, which already ran over/mcp.setupCassandraMCPServerhelper that opens the session, creates the three test tables and starts the server, reusing the existing package-local helpers (initCassandraSession,initTable,dropTable,createParamToolInfo,getCassandraAuthToolInfo,getCassandraTmplToolInfo,getCassandraWants,getCassandraTmplWants).The legacy
/apitest file is unchanged, sotests/cassandra/cassandra_integration_test.gokeeps--enable-apiuntil the cleanup phase of #3706.Cassandra specifics carried over
All of the legacy option overrides are carried over verbatim rather than normalized:
DisableSelect1Test(),DisableSelect1AuthTest()andDisableMcpSelect1AuthTest(), since CQL has noSELECT 1.RunToolInvokeTestis therefore called with an emptyselect1Want.DisableOptionalNullParamTest(), and the row expectationsWithMyToolId3NameAliceWant,WithMyToolById4Want(Cassandra returns an empty string rather than NULL) andWithMyArrayToolWant.DisableSelectFilterTest(),DisableDdlTest(),DisableInsertTest(), plusWithSelectAllWant,WithTmplSelectId1WantandWithTmplSelectNameWant. The DDL and insert tools are still registered byAddTemplateParamConfig, so they remain part of the asserted manifest even though they are not invoked.Teardown
The review findings from earlier PRs in this series are applied from the start:
t.Cleanuprather than a returned teardown closure (per the review on test(source/cockroachdb): add MCP integration tests #4113).CmdExec.Close()is called before the config-file cleanup (per the reviews on test(source/trino): add MCP integration tests #4149, test(source/oracle): add MCP integration tests #4165 and test(source/snowflake): add MCP integration tests #4166).StartCmd's returnedcleanuponly removes the temporary config file;Closeis what cancels the command context and closes its pipes.Cleanups run LIFO, so the effective order is: stop the server, drop the three tables, then close the session.
Review guidelines from #4046
/mcptests, per test(source/cloud-sql-mysql): create MCP integration tests #2922 — the legacy file is untouched.Test<>ListToolsandTest<>CallTool— matching the rest of the series.RunToolGetTest, which targets the/api-only tool-get endpoint. Cassandra has no source-specific invoke helpers, sotests/is unchanged by this PR.Validation
cassandra-cqlbuilds its manifest through the sharedparameters.ProcessParameters, so thetools/listentries match the shared expected-tool helpers.my-secure-tool, whichtools/listfilters out under the protocol version the harness negotiates.gofmt,go vet ./tests/cassandra/andgolangci-lint run ./tests/cassandra/...are clean..ci/integration.cloudbuild.yamlchange is needed: thecassandrashard runs the compiled package binary, so the new file is picked up automatically.Not yet verified
The Cassandra source connects to the cluster at startup and I have no Cassandra cluster locally, so the new tests have not run. The auth cases additionally need a Google ID token. They need a maintainer CI run.
PR Checklist
!if this involves a breaking changePart of #3706 🦕