Skip to content

[TT-17199] test: fix flaky mcp sql aggregate tests #1004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
probelabs wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[TT-17199] test: fix flaky mcp sql aggregate tests #1004

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pumps/mcp_sql_aggregate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@
t.Run(tc.name, func(t *testing.T) {
pump := MCPSQLAggregatePump{}
require.NoError(t, pump.Init(conf))
pump.db.Exec(fmt.Sprintf("DELETE FROM %q", tableName))

Check warning on line 206 in pumps/mcp_sql_aggregate_test.go

View check run for this annotation

probelabs / Visor: performance

performance Issue 

Using `DELETE FROM` to clear all rows from a table is less efficient than `TRUNCATE TABLE`. `DELETE` performs a scan and logs each row deletion, which can be slow and resource-intensive. `TRUNCATE` is a faster DDL operation that deallocates data pages with minimal logging. This performance recommendation applies to all similar `DELETE FROM` statements added in this file (lines 366, 381, 417, 620).
Raw output 
For clearing all data from a table in a test setup, prefer using `TRUNCATE TABLE` for better performance. This will make the test setup faster and use fewer resources, which can be noticeable when running large test suites repeatedly. The `DROP TABLE` on line 241 is appropriate for its context (sharded tables). The other `DELETE` statements should be changed to `TRUNCATE`.

Check warning on line 206 in pumps/mcp_sql_aggregate_test.go

View check run for this annotation

probelabs / Visor: quality

logic Issue 

The error returned by `pump.db.Exec` is not checked. If the database cleanup operation fails, the error will be ignored, and the test will proceed with a potentially dirty database state. This could lead to continued test flakiness. This issue applies to all newly added `pump.db.Exec` calls in this file (lines 206, 240, 366, 381, 417, 623).
Raw output 
Check the error and fail the test if the cleanup fails. For example, change line 206 to:
```go
_, err := pump.db.Exec(fmt.Sprintf("DELETE FROM %q", tableName))
require.NoError(t, err)
```
t.Cleanup(func() {

Check warning on line 207 in pumps/mcp_sql_aggregate_test.go

View check run for this annotation

probelabs / Visor: security

security Issue 

SQL queries are constructed using `fmt.Sprintf` on lines 207, 242, 367, 382, 418, and 623. This can lead to SQL injection if the table name variable is derived from user input. In this test file, the table names are hardcoded or constants, so there is no immediate vulnerability. However, this is a dangerous pattern that should be avoided to prevent accidental introduction of vulnerabilities if copied elsewhere.
Raw output 
To promote secure coding practices, avoid using `fmt.Sprintf` to build queries. Since table names cannot be parameterized in SQL, ensure they are from a trusted source (like constants, as is the case here). Consider adding a comment to these lines explaining that this is only safe because the table name is a hardcoded value, to warn other developers.
pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName))
})

Expand Down Expand Up @@ -236,11 +237,12 @@
Type: "postgres",
ConnectionString: getTestPostgresConnectionString(),
TableSharding: true,
},

Check failure on line 240 in pumps/mcp_sql_aggregate_test.go

View check run for this annotation

probelabs / Visor: quality

logic Issue 

The cleanup logic for the sharded test `TestMCPSQLAggregatePump_WriteData_Sharded` appears incorrect. Dropping the base table name (`tableName`) does not remove sharded tables (e.g., `..._20250101`) from previous runs. Since the test's correctness depends on the state of these sharded tables, leaving them in a dirty state will cause the test to remain flaky. The fix is therefore incomplete for this scenario.
Raw output 
Instead of dropping the base table, explicitly drop the specific sharded tables that this test creates and queries. This requires defining the timestamps used in the test before the cleanup logic.
```go
// Define timestamps to be used in the test
ts1 := time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)
ts2 := time.Date(2025, 1, 2, 0, 0, 0, 0, time.UTC)

// Clean up sharded tables from previous runs
tableName1 := fmt.Sprintf("%s_%s", tableName, ts1.Format("20060102"))
tableName2 := fmt.Sprintf("%s_%s", tableName, ts2.Format("20060102"))

_, err := pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName1))
require.NoError(t, err)
_, err = pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName2))
require.NoError(t, err)
```
This logic should replace the `DROP TABLE` call on line 240. You will also need to update the `record1` and `record2` initializations to use `ts1` and `ts2` respectively.
}))
pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName))

record1 := analytics.AnalyticsRecord{
TimeStamp: time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC),

Check failure on line 245 in pumps/mcp_sql_aggregate_test.go

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The pre-test cleanup for the sharded test case appears to be incorrect. It attempts to `DROP` the base table name, but the test creates and uses date-sharded tables (e.g., `..._20250101`). This `DROP` statement will not remove leftover sharded tables from previous runs, failing to fix the potential test flakiness. The cleanup should target the specific sharded tables this test interacts with.
Raw output 
The cleanup logic should explicitly drop the sharded tables that are used in this test (e.g., `tyk_mcp_analytics_20250101` and `tyk_mcp_analytics_20250102`). Also, consider using `t.Cleanup` to ensure tables are dropped even if the test fails mid-execution, which would make the test suite more robust.
APIID: "test-api", APIName: "test-api", OrgID: "test-org",
ResponseCode: 200, Day: 1, Month: 1, Year: 2025,
MCPStats: analytics.MCPStats{IsMCP: true, JSONRPCMethod: "tools/call", PrimitiveType: "tool", PrimitiveName: "t1"},
Expand Down Expand Up @@ -363,6 +365,7 @@
ConnectionString: getTestPostgresConnectionString(),
},
}))
pump.db.Exec(fmt.Sprintf("DELETE FROM %q", analytics.AggregateMCPSQLTable))
err := pump.WriteData(context.Background(), []interface{}{})
assert.NoError(t, err)
}
Expand All @@ -378,6 +381,7 @@
ConnectionString: getTestPostgresConnectionString(),
},
}))
pump.db.Exec(fmt.Sprintf("DELETE FROM %q", tableName))
t.Cleanup(func() {
pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName))
})
Expand Down Expand Up @@ -414,6 +418,7 @@
BatchSize: 1, // force 1-record batches to exercise batch loop
},
}))
pump.db.Exec(fmt.Sprintf("DELETE FROM %q", tableName))
t.Cleanup(func() {
pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName))
})
Expand Down Expand Up @@ -617,6 +622,7 @@
ConnectionString: getTestPostgresConnectionString(),
},
}))
pump.db.Exec(fmt.Sprintf("DELETE FROM %q", tableName))
t.Cleanup(func() {
pump.db.Exec(fmt.Sprintf("DROP TABLE IF EXISTS %q", tableName))
})
Expand Down
Loading