Add @proxysql_min_version tag for version-aware TAP test execution#5562
Add @proxysql_min_version tag for version-aware TAP test execution#5562renecannao merged 2 commits intov3.0from
Conversation
Skip tests that require a newer ProxySQL version than the one being tested, preventing false-positive CI failures across v3.0.x/v3.1.x/4.0.x branches. Tagged 19 FFTO/TSDB tests (>=3.1) and 46 GenAI/MCP/AI tests (>=4.0) in groups.json. Tests without a tag run on all versions.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughImplemented version-aware test filtering by detecting ProxySQL binary version at runtime and skipping tests annotated with Changes
Sequence DiagramsequenceDiagram
participant Runner as Test Runner
participant Detector as Version Detector
participant Binary as ProxySQL Binary
participant Discovery as Test Discovery
participant Executor as Test Executor
Runner->>Detector: Start test run
Detector->>Binary: Execute --version
Binary-->>Detector: Return version string
Detector->>Detector: Parse semantic version (e.g., 4.0.0)
Detector-->>Runner: Detected version
Runner->>Discovery: Load test groups & metadata
Discovery->>Discovery: Parse groups.json & `@proxysql_min_version` tags
Discovery-->>Runner: Tests with min_version requirements
Runner->>Runner: For each test: compare detected_version >= min_version?
alt Version Match
Runner->>Executor: Queue test for execution
Executor-->>Runner: Test runs
else Version Mismatch
Runner->>Runner: Log skip (requires vX.Y+)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 adds support for MySQL 8.4 to the CI infrastructure, introducing a new infra-mysql84-binlog environment and a standalone binlog reader. It implements version-based test filtering via a new @proxysql_min_version tag and updates existing TAP tests to accommodate MySQL 8.4 changes, such as the utf8mb3 charset and revised error codes. Review feedback identifies a missing Python dependency (packaging.version) in some environments, security risks with eval in shell scripts, and several bugs in test_binlog_reader-t.cpp, including a mismatched argument in a diagnostic message and redundant semicolons.
I am having trouble creating individual review comments. Click here to see my feedback.
test/infra/control/run-tests-isolated.bash (188)
The script imports packaging.version, which is not part of the Python standard library. This introduces a dependency that might not be present in all CI environments, potentially causing the test runner to fail. Consider using distutils.version.LooseVersion (though deprecated) or a simple custom version parsing helper to avoid external dependencies.
test/infra/infra-mysql84-binlog/bin/docker-proxy-post.bash (12)
Using eval to perform variable substitution in the SQL file is brittle and potentially dangerous. If the SQL content contains characters like backticks or dollar signs that are not intended for shell expansion, the command will fail or produce incorrect SQL. A safer and more robust approach would be to use envsubst if available, or a simple sed command for specific variables.
References
- Avoid using eval on untrusted or complex strings as it can lead to unexpected shell execution or mangling of data.
test/tap/tests/test_binlog_reader-t.cpp (201)
Redundant double semicolon at the end of the line.
uint32_t hg_whg_sync_queries = hg_stats.at(WHG).second;
test/tap/tests/test_binlog_reader-t.cpp (203)
Redundant double semicolon at the end of the line.
uint32_t hg_rhg_sync_queries = hg_stats.at(RHG).second;
test/tap/tests/test_binlog_reader-t.cpp (221)
There is a logic error in the diagnostic message arguments. hg_rhg_queries is passed twice at the end, but the format string expects act_sync_queries as the last placeholder, which should be hg_rhg_sync_queries.
WHG, hg_whg_exp_queries, hg_whg_queries, hg_whg_exp_sync_queries, hg_whg_sync_queries,
RHG, NUM_CHECKS, hg_rhg_queries, NUM_CHECKS, hg_rhg_sync_queriestest/tap/tests/test_binlog_reader-t.cpp (242)
Redundant double semicolon at the end of the line.
hg_whg_sync_queries = hg_stats.at(WHG).second;
test/tap/tests/test_binlog_reader-t.cpp (244)
Redundant double semicolon at the end of the line.
hg_rhg_sync_queries = hg_stats.at(RHG).second;
test/tap/tests/test_read_only_actions_offline_hard_servers-t.cpp (41)
Hardcoding RHG = WHG + 1 is risky as it assumes a specific hostgroup numbering convention that may not hold true for all infrastructures. It is better to explicitly read RHG from an environment variable (e.g., TAP_MYSQL8_READER_HG) or derive it more reliably from the infrastructure configuration.
Signed-off-by: René Cannaò <rene@proxysql.com>
|
2 participants
Summary
@proxysql_min_version:X.Ymetadata tag ingroups.jsonto skip tests that require a newer ProxySQL version than the one being tested@proxysql_min_version:3.1and 46 GenAI/MCP/AI tests with@proxysql_min_version:4.0Closes #5561
Summary by CodeRabbit