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

[probelabs]

Problem / Task

Fix flaky tests in pumps/mcp_sql_aggregate_test.go caused by dirty database state from previous test runs.

Changes

• Explicitly clear the database table (DELETE FROM or DROP TABLE) right after pump.Init(...) in the tests to ensure a clean state before test logic runs.

Testing

• Ran go test ./pumps -run TestMCPSQLAggregatePump
• Verified build with go build ./...

[probelabs]

This PR fixes flaky tests in pumps/mcp_sql_aggregate_test.go by ensuring a clean database state for each test run. The changes introduce explicit database table clearing at the beginning of each test case, preventing state from one test from interfering with another and improving the reliability of the test suite.

Files Changed Analysis

• pumps/mcp_sql_aggregate_test.go: 6 lines were added across 6 different test functions (TestMCPSQLAggregatePump_WriteData, _Sharded, _EmptyData, _Upsert, _SmallBatchSize, _MultipleAPIs).
• The change consists of adding a pump.db.Exec(...) call to either DELETE FROM or DROP TABLE immediately after the pump is initialized, ensuring each test starts with a clean slate.

Architecture & Impact Assessment

• Accomplishment: Improves the stability and reliability of the CI pipeline by fixing flaky tests related to the MCP SQL aggregate pump.
• Key Technical Changes: The core change is the introduction of explicit database state cleanup at the start of each test, complementing the existing t.Cleanup teardown logic. This enforces test isolation.
• Affected System Components: The changes are confined to the test suite and have no impact on the production code or runtime behavior of the MCPSQLAggregatePump. The impact is limited to the development and testing environment.

Scope Discovery & Context Expansion

• The fix is localized to the mcp_sql_aggregate_test.go file. However, the underlying issue—flaky tests caused by shared database state—is a common pattern.
• A search for other database integration tests in the pumps/ directory reveals similar test files for other database backends (e.g., sql_test.go, mongo_test.go, graph_sql_test.go).
• This PR highlights a potential need to audit these other tests for similar state-related flakiness and apply the same cleanup-on-startup pattern to improve overall test suite robustness.

Metadata

• Review Effort: 1 / 5
• Primary Label: chore

---

Powered by Visor from Probelabs

Last updated: 2026-05-12T06:19:44.289Z | Triggered by: pr_opened | Commit: a92bc84

💡 TIP: You can chat with Visor using /visor ask <your question>

[probelabs]

Security Issues (1)

Severity	Location	Issue
🟡 Warning	pumps/mcp_sql_aggregate_test.go:207	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. 💡 Suggestion 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.

Architecture Issues (1)

Severity	Location	Issue
🟠 Error	pumps/mcp_sql_aggregate_test.go:245	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. 💡 Suggestion 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.

Security Issues (1)

Severity	Location	Issue
🟡 Warning	pumps/mcp_sql_aggregate_test.go:207	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. 💡 Suggestion 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.

\n\n ### Architecture Issues (1)

Severity	Location	Issue
🟠 Error	pumps/mcp_sql_aggregate_test.go:245	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. 💡 Suggestion 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.

\n\n ### Performance Issues (1)

Severity	Location	Issue
🟡 Warning	pumps/mcp_sql_aggregate_test.go:206	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). 💡 Suggestion 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`.

---

Powered by Visor from Probelabs

Last updated: 2026-05-12T06:19:37.291Z | Triggered by: pr_opened | Commit: a92bc84

💡 TIP: You can chat with Visor using /visor ask <your question>

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud