test(source/cloudsqlpg): create MCP integration tests#2994
test(source/cloudsqlpg): create MCP integration tests#2994anubhav756 wants to merge 6 commits intoanubhav-mcp-alloydbpgfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds integration tests for the Cloud SQL Postgres data source, including tests for listing and calling tools. The review identifies several improvement opportunities: refactoring the connection pool setup to eliminate code duplication and use production logic, enhancing environment variable validation to report all missing variables simultaneously, and renaming validation functions to better reflect their purpose while ensuring consistent application across test cases.
| switch "" { | ||
| case CloudSQLPostgresProject: | ||
| t.Fatal("'CLOUD_SQL_POSTGRES_PROJECT' not set") | ||
| case CloudSQLPostgresRegion: | ||
| t.Fatal("'CLOUD_SQL_POSTGRES_REGION' not set") | ||
| case CloudSQLPostgresInstance: | ||
| t.Fatal("'CLOUD_SQL_POSTGRES_INSTANCE' not set") | ||
| case CloudSQLPostgresDatabase: | ||
| t.Fatal("'CLOUD_SQL_POSTGRES_DATABASE' not set") | ||
| case CloudSQLPostgresUser: | ||
| t.Fatal("'CLOUD_SQL_POSTGRES_USER' not set") | ||
| case CloudSQLPostgresPass: | ||
| t.Fatal("'CLOUD_SQL_POSTGRES_PASS' not set") | ||
| } |
There was a problem hiding this comment.
This switch "" pattern for checking environment variables is clever, but it has a downside: it fails the test on the first missing variable. This can be frustrating for developers who then have to fix one variable and rerun tests, only to find another one is missing.
A more developer-friendly approach is to check for all required variables and report all missing ones at once. This gives a complete picture of the required setup.
vars := map[string]string{
"CLOUD_SQL_POSTGRES_PROJECT": CloudSQLPostgresProject,
"CLOUD_SQL_POSTGRES_REGION": CloudSQLPostgresRegion,
"CLOUD_SQL_POSTGRES_INSTANCE": CloudSQLPostgresInstance,
"CLOUD_SQL_POSTGRES_DATABASE": CloudSQLPostgresDatabase,
"CLOUD_SQL_POSTGRES_USER": CloudSQLPostgresUser,
"CLOUD_SQL_POSTGRES_PASS": CloudSQLPostgresPass,
}
var missing []string
for key, val := range vars {
if val == "" {
missing = append(missing, key)
}
}
if len(missing) > 0 {
t.Fatalf("Missing required environment variables: %s", strings.Join(missing, ", "))
}| } | ||
|
|
||
| func TestCloudSQLPgCallTool(t *testing.T) { | ||
| getCloudSQLPgVars(t) |
There was a problem hiding this comment.
The getCloudSQLPgVars function is called here, but its return value is ignored. This is confusing because the function name suggests it gets something, but it's being used only for the side-effect of validating environment variables.
To improve clarity, consider renaming the function to requireCloudSQLPgVars to make its purpose as a validator explicit.
Also, TestCloudSQLPgListTools would benefit from this check at the beginning, as it also depends on these environment variables to start the server with the prebuilt configuration.
75f859a to
c4a9912
Compare
59c2b6a to
2be51dc
Compare
5849570 to
274ee95
Compare
2be51dc to
5561185
Compare
4323012 to
4b94c00
Compare
e120bc5 to
e74fd80
Compare
4b94c00 to
2f96090
Compare
7011296 to
29a4b31
Compare
7b71b7d to
9a3ebe7
Compare
8fb70d9 to
4edad57
Compare
4edad57 to
aa1537c
Compare
This PR adds the mapped integration tests for CloudSQL PG tools using the native MCP harness.