test: add unit tests for low-coverage lib files - #5984
Conversation
Add unit tests targeting files with near-zero Codecov coverage: - parsersql_unit-t: Query_Processor_ParserSQL digests, command types, SET - pgsql_query_processor_unit-t: query_parser_command_type switch coverage - ffto_state_machine_unit-t: MySQLFFTO/PgSQLFFTO packet ingest paths - http_server_unit-t: ProxySQL_HTTP_Server lifecycle - restapi_server_unit-t: RESTAPI lifecycle and custom endpoint Note: some low Codecov numbers on these files were also inflated by a partial integration-tests upload on the latest v3.0 commit; these unit tests improve genuine coverage independent of that gap.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds TAP unit tests for SQL parsing, PostgreSQL query classification, FFTO state machines, HTTP server lifecycle, and REST API handling. It registers the tests, updates FFTO test setup for all users, and makes connector-pin validation portable. ChangesUnit test coverage and FFTO setup
Connector validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/tap/tests/unit/ffto_state_machine_unit-t.cpp (1)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse scoped FFTO objects.
Use stack allocation for both FFTO objects.
newnormally throws on allocation failure, so the null checks do not add useful coverage. A nested scope can retain the explicit destructor check.As per coding guidelines, use RAII for resource management.
Also applies to: 65-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/tap/tests/unit/ffto_state_machine_unit-t.cpp` around lines 17 - 21, Update the MySQLFFTO constructor/destructor tests to use stack-allocated objects within nested scopes instead of new/delete and remove the redundant null checks. Preserve the explicit destructor coverage by ending the inner scope before asserting the destructor behavior, and apply the same change to both FFTO test instances.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/unit/ffto_state_machine_unit-t.cpp`:
- Around line 122-129: Correct the PostgreSQL Parse message construction in the
shown test setup: use a 12-byte payload, encode the wire length as 16, and copy
query at msg[5 + name_len] so the statement-name terminator is not skipped
twice. Keep the resulting message valid for the expected SELECT 1 query.
In `@test/tap/tests/unit/Makefile`:
- Around line 428-432: Register parsersql_unit-t, pgsql_query_processor_unit-t,
http_server_unit-t, ffto_state_machine_unit-t, and restapi_server_unit-t in
test/tap/groups/groups.json, assigning each to an appropriate existing TAP group
so grouped execution includes all five binaries.
In `@test/tap/tests/unit/restapi_server_unit-t.cpp`:
- Around line 58-73: Replace the fixed usleep before the curl request with
bounded readiness polling in the REST API test. Repeatedly invoke the existing
GET command and parse its HTTP status until it returns 200 or a clear deadline
expires, preserving the final ok assertion and reporting the last observed
status.
- Around line 34-43: Update the endpoint callback in the REST API server test to
capture callback state through a shared_ptr held by value, rather than
referencing the stack-local hit variable. Add a pthread mutex protecting both
the callback’s write and the test thread’s read of that state, including the
related setup and assertion lines, while preserving the existing request
behavior.
---
Nitpick comments:
In `@test/tap/tests/unit/ffto_state_machine_unit-t.cpp`:
- Around line 17-21: Update the MySQLFFTO constructor/destructor tests to use
stack-allocated objects within nested scopes instead of new/delete and remove
the redundant null checks. Preserve the explicit destructor coverage by ending
the inner scope before asserting the destructor behavior, and apply the same
change to both FFTO test instances.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 393885df-2d5b-4aaf-b753-589e259fad6c
📒 Files selected for processing (6)
test/tap/tests/unit/Makefiletest/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/http_server_unit-t.cpptest/tap/tests/unit/parsersql_unit-t.cpptest/tap/tests/unit/pgsql_query_processor_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: run / trigger
- GitHub Check: build
⚠️ CI failures not shown inline (2)
GitHub Actions: CI-lint-groups-json / 0_lint.txt: test: add unit tests for low-coverage lib files
Conclusion: failure
##[group]Run python3 test/tap/groups/check_groups.py --source
�[36;1mpython3 test/tap/groups/check_groups.py --source�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
NOTE: 24 test(s) in groups.json have no matching source file on disk:
- fast_forward_grace_close_libmysql-t
- fast_forward_switch_replication_deprecate_eof_libmysql-t
- mysql-zstd_compression_level_libmysql-t
- mysql_reconnect_libmariadb-t
- mysql_reconnect_libmysql-t
- ok_packet_mixed_queries-t
- parsersql_digest_test-t
- prepare_statement_err3024_async-t
- prepare_statement_err3024_libmysql-t
- reg_test_mariadb_stmt_store_result_async-t
- reg_test_mariadb_stmt_store_result_libmysql-t
- reg_test_stmt_resultset_err_no_rows_libmysql-t
- setparser_parsersql_test-t
- setparser_test2-t
- setparser_test3-t
- test_clickhouse_server_libmysql-t
- test_match_eof_conn_cap_libmariadb-t
- test_match_eof_conn_cap_libmysql-t
- test_sqlite3_special_queries_libmariadb-t
- test_sqlite3_special_queries_libmysql-t
- test_ssl_fast_forward-2_libmariadb-t
- test_ssl_fast_forward-2_libmysql-t
- test_ssl_fast_forward-3_libmariadb-t
- test_ssl_fast_forward-3_libmysql-t
ERROR: 5 source test(s) missing from groups.json:
- ffto_state_machine_unit-t
- http_server_unit-t
- parsersql_unit-t
- pgsql_query_processor_unit-t
- restapi_server_unit-t
##[error]Process completed with exit code 1.
GitHub Actions: CI-lint-groups-json / lint: test: add unit tests for low-coverage lib files
Conclusion: failure
##[group]Run python3 test/tap/groups/check_groups.py --source
�[36;1mpython3 test/tap/groups/check_groups.py --source�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
NOTE: 24 test(s) in groups.json have no matching source file on disk:
- fast_forward_grace_close_libmysql-t
- fast_forward_switch_replication_deprecate_eof_libmysql-t
- mysql-zstd_compression_level_libmysql-t
- mysql_reconnect_libmariadb-t
- mysql_reconnect_libmysql-t
- ok_packet_mixed_queries-t
- parsersql_digest_test-t
- prepare_statement_err3024_async-t
- prepare_statement_err3024_libmysql-t
- reg_test_mariadb_stmt_store_result_async-t
- reg_test_mariadb_stmt_store_result_libmysql-t
- reg_test_stmt_resultset_err_no_rows_libmysql-t
- setparser_parsersql_test-t
- setparser_test2-t
- setparser_test3-t
- test_clickhouse_server_libmysql-t
- test_match_eof_conn_cap_libmariadb-t
- test_match_eof_conn_cap_libmysql-t
- test_sqlite3_special_queries_libmariadb-t
- test_sqlite3_special_queries_libmysql-t
- test_ssl_fast_forward-2_libmariadb-t
- test_ssl_fast_forward-2_libmysql-t
- test_ssl_fast_forward-3_libmariadb-t
- test_ssl_fast_forward-3_libmysql-t
ERROR: 5 source test(s) missing from groups.json:
- ffto_state_machine_unit-t
- http_server_unit-t
- parsersql_unit-t
- pgsql_query_processor_unit-t
- restapi_server_unit-t
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (3)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/parsersql_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpptest/tap/tests/unit/http_server_unit-t.cpptest/tap/tests/unit/pgsql_query_processor_unit-t.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/parsersql_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpptest/tap/tests/unit/http_server_unit-t.cpptest/tap/tests/unit/pgsql_query_processor_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/parsersql_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpptest/tap/tests/unit/http_server_unit-t.cpptest/tap/tests/unit/pgsql_query_processor_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/parsersql_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpptest/tap/tests/unit/http_server_unit-t.cpptest/tap/tests/unit/pgsql_query_processor_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/parsersql_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpptest/tap/tests/unit/http_server_unit-t.cpptest/tap/tests/unit/pgsql_query_processor_unit-t.cpp
🪛 Cppcheck (2.21.0)
test/tap/tests/unit/parsersql_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/restapi_server_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/http_server_unit-t.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/pgsql_query_processor_unit-t.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (5)
test/tap/tests/unit/ffto_state_machine_unit-t.cpp (1)
1-15: LGTM!Also applies to: 24-62, 72-113, 135-169
test/tap/tests/unit/pgsql_query_processor_unit-t.cpp (1)
1-448: LGTM!test/tap/tests/unit/parsersql_unit-t.cpp (1)
19-22: 🎯 Functional CorrectnessNo TAP plan mismatch here.
plan(53)is called after the other tests do not run, so these conditional assertions are not counted in the planned result count for this test sequence.> Likely an incorrect or invalid review comment.test/tap/tests/unit/http_server_unit-t.cpp (1)
1-65: LGTM!test/tap/tests/unit/restapi_server_unit-t.cpp (1)
1-33: LGTM!Also applies to: 77-86
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.0 #5984 +/- ##
==========================================
+ Coverage 52.41% 52.68% +0.26%
==========================================
Files 472 473 +1
Lines 143270 143367 +97
Branches 36196 36255 +59
==========================================
+ Hits 75102 75530 +428
+ Misses 51322 50936 -386
- Partials 16846 16901 +55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Register new unit tests in groups.json (unit-tests-g1) so check_groups.py --source passes - Fix PgSQL Parse message payload layout in ffto_state_machine_unit-t - Fix RESTAPI test: shared atomic hit flag, readiness polling instead of fixed usleep
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/unit/restapi_server_unit-t.cpp`:
- Line 50: Update the test setup around find_free_port and
ProxySQL_RESTAPI_Server::init so port allocation and server startup are
race-safe: either retry selecting a port when init fails, or reserve and pass
the bound socket through startup. Ensure the test does not proceed to wait on a
server that failed to listen.
- Line 51: After the port discovery assertion, stop the test when port <= 0 by
using the TAP harness bail or skip mechanism before constructing
ProxySQL_RESTAPI_Server. Preserve the existing success path for valid ports and
avoid initializing the server with an invalid value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 658bba9d-785f-4424-a859-15d3f97a5399
📒 Files selected for processing (3)
test/tap/groups/groups.jsontest/tap/tests/unit/ffto_state_machine_unit-t.cpptest/tap/tests/unit/restapi_server_unit-t.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tap/tests/unit/ffto_state_machine_unit-t.cpp
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/unit/restapi_server_unit-t.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/unit/restapi_server_unit-t.cpp
test/tap/tests/unit/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in
test/tap/tests/unit/must usetest_globals.handtest_init.hwith the custom unit-test harness.
Files:
test/tap/tests/unit/restapi_server_unit-t.cpp
🧠 Learnings (2)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/restapi_server_unit-t.cpp
📚 Learning: 2026-04-01T21:27:00.297Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:00.297Z
Learning: In ProxySQL unit tests under test/tap/tests/unit/, include test_globals.h and test_init.h only for tests that depend on ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). For “pure” data-structure/utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) that do not require runtime globals/initialization, it is correct to omit test_globals.h and test_init.h and instead include only tap.h plus the relevant project header(s).
Applied to files:
test/tap/tests/unit/restapi_server_unit-t.cpp
🪛 Cppcheck (2.21.0)
test/tap/tests/unit/restapi_server_unit-t.cpp
[warning] 46-46: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🪛 GitHub Actions: CI-lint-groups-json / 0_lint.txt
test/tap/groups/groups.json
[error] 1-1: groups.json format lint failed: key 'pgsql_query_logging_autodump-t' is not sorted before 'pgsql_query_processor_unit-t'. Run 'python3 test/tap/groups/lint_groups_json.py --fix' to auto-fix.
🪛 GitHub Actions: CI-lint-groups-json / lint
test/tap/groups/groups.json
[error] 1-1: groups.json format lint failed: key 'pgsql_query_logging_autodump-t' should come before 'pgsql_query_processor_unit-t'. Run 'python3 test/tap/groups/lint_groups_json.py --fix' to correct the ordering.
🔇 Additional comments (6)
test/tap/tests/unit/restapi_server_unit-t.cpp (5)
53-64: Use the repository-standard mutex for callback state.The current
shared_ptrcapture fixes callback lifetime, and the atomic operations avoid a data race. However,hitis a cross-thread flag, not a counter. Keep shared ownership, but protect the write at Line 59 and the read at Line 90 with one pthread mutex.As per coding guidelines, use pthread mutexes for synchronization and
std::atomic<>for counters.Also applies to: 90-90
Source: Coding guidelines
7-11: LGTM!
66-89: LGTM!
93-102: 🎯 Functional CorrectnessVerify the TAP plan after adding the port failure path.
The supplied snippet omits Lines 93-102. Confirm that the seven-assertion plan matches all
ok()calls and that the TAP bail or skip path forport <= 0does not leave the plan incomplete.
36-37: 🩺 Stability & AvailabilityNo change needed.
The CI test image installs
curl, socurl_http_code()has the executable it launches throughpopen().test/tap/groups/groups.json (1)
37-37: LGTM!Also applies to: 61-61, 152-152, 207-207, 231-231
| } | ||
|
|
||
| static void test_lifecycle_and_endpoint() { | ||
| int port = find_free_port(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid the free-port check/use race.
find_free_port() returns only an integer. It cannot reserve the socket until ProxySQL_RESTAPI_Server::init() binds the port. Another test or local process can claim the port after Line 50. The server then fails to listen, and the test waits until the deadline. Retry allocation and startup as one operation, or pass a reserved socket to the server.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/tap/tests/unit/restapi_server_unit-t.cpp` at line 50, Update the test
setup around find_free_port and ProxySQL_RESTAPI_Server::init so port allocation
and server startup are race-safe: either retry selecting a port when init fails,
or reserve and pass the bound socket through startup. Ensure the test does not
proceed to wait on a server that failed to listen.
pgsql_query_processor_unit-t was inserted before pgsql_query_logging_*. Run lint_groups_json.py --fix so alphabetical order is correct.
Soak jobs fail immediately with "rg: command not found" because test-connector-version-pins.bash required ripgrep. Switch to POSIX grep -F/-x/-q so the check runs on stock GitHub runners.
Partial INSERT OR REPLACE INTO mysql_users/pgsql_users only updated the backend PK row (username, backend) and left the frontend credential row at fast_forward=0. Auth uses the frontend row, so sessions never entered FAST_FORWARD and MySQLFFTO/PgSQLFFTO were never constructed — while digest assertions still passed via the normal query path. Use UPDATE ... SET fast_forward=1 on all user rows (matching mysql-fast_forward-t), verify runtime frontend credentials, and connect mysql errors test through ProxySQL (cl.port) not the backend port.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/tap/tests/test_ffto_pgsql-t.cpp`:
- Around line 120-130: Update the runtime-user validation block in
test_ffto_pgsql-t.cpp to treat a nonzero mysql_query() result as fatal: emit the
existing diagnostic style and return -1 before processing the result. Keep the
existing count validation for successful queries, ensuring the test only
proceeds when a frontend runtime user with fast_forward=1 is confirmed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e245ef17-2cf1-4016-93a8-b2d250c77d22
📒 Files selected for processing (18)
test/tap/tests/test_ffto_bypass-t.cpptest/tap/tests/test_ffto_mysql-t.cpptest/tap/tests/test_ffto_mysql_bypass_recovery-t.cpptest/tap/tests/test_ffto_mysql_concurrent-t.cpptest/tap/tests/test_ffto_mysql_errors-t.cpptest/tap/tests/test_ffto_mysql_large_queries-t.cpptest/tap/tests/test_ffto_mysql_large_resultsets-t.cpptest/tap/tests/test_ffto_mysql_mixed_protocol-t.cpptest/tap/tests/test_ffto_mysql_transactions-t.cpptest/tap/tests/test_ffto_pgsql-t.cpptest/tap/tests/test_ffto_pgsql_command_types-t.cpptest/tap/tests/test_ffto_pgsql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql_error_stats-t.cpptest/tap/tests/test_ffto_pgsql_errors-t.cpptest/tap/tests/test_ffto_pgsql_large_resultsets-t.cpptest/tap/tests/test_ffto_pgsql_mixed_protocol-t.cpptest/tap/tests/test_ffto_pgsql_pipeline-t.cpptest/tap/tests/test_ffto_pgsql_stmt_portal-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap-mysqlx)
- GitHub Check: run / trigger
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
test/tap/tests/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
test/tap/tests/**/*.cpp: Test files intest/tap/tests/must follow the naming patterntest_*.cppor*-t.cpp.
To add a new TAP test, add the<testname>-t.cppfile and register it intest/tap/tests/Makefile/groups.json; no special Makefile target is needed becausemake <testname>-tis generated by pattern rule.
Files:
test/tap/tests/test_ffto_pgsql_error_stats-t.cpptest/tap/tests/test_ffto_mysql_mixed_protocol-t.cpptest/tap/tests/test_ffto_pgsql_errors-t.cpptest/tap/tests/test_ffto_pgsql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql_stmt_portal-t.cpptest/tap/tests/test_ffto_pgsql_large_resultsets-t.cpptest/tap/tests/test_ffto_mysql_errors-t.cpptest/tap/tests/test_ffto_mysql_large_resultsets-t.cpptest/tap/tests/test_ffto_bypass-t.cpptest/tap/tests/test_ffto_mysql-t.cpptest/tap/tests/test_ffto_pgsql_mixed_protocol-t.cpptest/tap/tests/test_ffto_mysql_bypass_recovery-t.cpptest/tap/tests/test_ffto_mysql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql-t.cpptest/tap/tests/test_ffto_mysql_transactions-t.cpptest/tap/tests/test_ffto_mysql_large_queries-t.cpptest/tap/tests/test_ffto_pgsql_command_types-t.cpptest/tap/tests/test_ffto_pgsql_pipeline-t.cpp
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names must usePascalCasewith protocol prefixes such asMySQL_,PgSQL_, andProxySQL_.
Member variables must usesnake_case.
Constants and macros must useUPPER_SNAKE_CASE.
Use C++17, and gate conditional code with#ifdef PROXYSQL31,#ifdef PROXYSQL40,#ifdef PROXYSQLFFTO,#ifdef PROXYSQLTSDB, and#ifdef PROXYSQLCLICKHOUSE;PROXYSQLGENAImust not guard core code outsideplugins/genai/.
Consider performance implications when changing hot paths or other performance-critical code.
Use RAII for resource management and jemalloc for allocation.
Use pthread mutexes for synchronization andstd::atomic<>for counters.
Files:
test/tap/tests/test_ffto_pgsql_error_stats-t.cpptest/tap/tests/test_ffto_mysql_mixed_protocol-t.cpptest/tap/tests/test_ffto_pgsql_errors-t.cpptest/tap/tests/test_ffto_pgsql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql_stmt_portal-t.cpptest/tap/tests/test_ffto_pgsql_large_resultsets-t.cpptest/tap/tests/test_ffto_mysql_errors-t.cpptest/tap/tests/test_ffto_mysql_large_resultsets-t.cpptest/tap/tests/test_ffto_bypass-t.cpptest/tap/tests/test_ffto_mysql-t.cpptest/tap/tests/test_ffto_pgsql_mixed_protocol-t.cpptest/tap/tests/test_ffto_mysql_bypass_recovery-t.cpptest/tap/tests/test_ffto_mysql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql-t.cpptest/tap/tests/test_ffto_mysql_transactions-t.cpptest/tap/tests/test_ffto_mysql_large_queries-t.cpptest/tap/tests/test_ffto_pgsql_command_types-t.cpptest/tap/tests/test_ffto_pgsql_pipeline-t.cpp
🧠 Learnings (1)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/test_ffto_pgsql_error_stats-t.cpptest/tap/tests/test_ffto_mysql_mixed_protocol-t.cpptest/tap/tests/test_ffto_pgsql_errors-t.cpptest/tap/tests/test_ffto_pgsql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql_stmt_portal-t.cpptest/tap/tests/test_ffto_pgsql_large_resultsets-t.cpptest/tap/tests/test_ffto_mysql_errors-t.cpptest/tap/tests/test_ffto_mysql_large_resultsets-t.cpptest/tap/tests/test_ffto_bypass-t.cpptest/tap/tests/test_ffto_mysql-t.cpptest/tap/tests/test_ffto_pgsql_mixed_protocol-t.cpptest/tap/tests/test_ffto_mysql_bypass_recovery-t.cpptest/tap/tests/test_ffto_mysql_concurrent-t.cpptest/tap/tests/test_ffto_pgsql-t.cpptest/tap/tests/test_ffto_mysql_transactions-t.cpptest/tap/tests/test_ffto_mysql_large_queries-t.cpptest/tap/tests/test_ffto_pgsql_command_types-t.cpptest/tap/tests/test_ffto_pgsql_pipeline-t.cpp
🔇 Additional comments (18)
test/tap/tests/test_ffto_bypass-t.cpp (1)
37-41: LGTM!test/tap/tests/test_ffto_mysql-t.cpp (1)
113-132: LGTM!test/tap/tests/test_ffto_mysql_bypass_recovery-t.cpp (1)
143-149: LGTM!test/tap/tests/test_ffto_mysql_concurrent-t.cpp (1)
219-224: LGTM!test/tap/tests/test_ffto_pgsql_errors-t.cpp (1)
139-143: LGTM!test/tap/tests/test_ffto_pgsql_large_resultsets-t.cpp (1)
174-179: LGTM!test/tap/tests/test_ffto_pgsql_mixed_protocol-t.cpp (1)
140-144: LGTM!test/tap/tests/test_ffto_pgsql_pipeline-t.cpp (1)
127-131: LGTM!test/tap/tests/test_ffto_pgsql_stmt_portal-t.cpp (1)
126-130: LGTM!test/tap/tests/test_ffto_mysql_errors-t.cpp (1)
82-87: LGTM!Also applies to: 96-96
test/tap/tests/test_ffto_mysql_large_queries-t.cpp (1)
191-196: LGTM!test/tap/tests/test_ffto_mysql_large_resultsets-t.cpp (1)
214-219: LGTM!test/tap/tests/test_ffto_mysql_mixed_protocol-t.cpp (1)
230-235: LGTM!test/tap/tests/test_ffto_mysql_transactions-t.cpp (1)
169-174: LGTM!test/tap/tests/test_ffto_pgsql-t.cpp (1)
114-119: LGTM!test/tap/tests/test_ffto_pgsql_command_types-t.cpp (1)
178-183: LGTM!test/tap/tests/test_ffto_pgsql_concurrent-t.cpp (1)
140-145: LGTM!test/tap/tests/test_ffto_pgsql_error_stats-t.cpp (1)
77-81: LGTM!
Query errno (table column), not err_no (in-memory result label).
- Add ffto_mysql_helpers.h: SET mysql-ffto_enabled, require runtime frontend fast_forward=1 for the connecting user, verify session via processlist extended_info, and reset digests/errors correctly. - Rewrite test_ffto_mysql_errors-t to gate on real FF and use errno. - Fix test_ffto_mysql-t digest reset (DELETE mirror was a no-op). - Use SET mysql-ffto_* in remaining MySQL FFTO tests. - FFTO report_error/report_query_stats: do not drop stats when schemaname is unset.
- test_ffto_pgsql-t: treat runtime_pgsql_users query failure as fatal; use SET for ffto vars; reset digests via stats_*_reset - ffto_state_machine_unit: clarify valid PostgreSQL Parse wire layout - restapi_server_unit: bail out if no free port; keep readiness polling
|
1 participant
Summary
Adds unit tests for several
lib/files that show very low coverage on Codecov (v3.0):Query_Processor_ParserSQL.cppparsersql_unit-t(53 asserts)PgSQL_Query_Processor.cpppgsql_query_processor_unit-t(53 asserts)MySQLFFTO.cpp/PgSQLFFTO.cppffto_state_machine_unit-t(27 asserts)ProxySQL_HTTP_Server.cpphttp_server_unit-t(11 asserts)ProxySQL_RESTAPI_Server.cpprestapi_server_unit-t(7 asserts)Diagnosis note
Latest Codecov head (
89a29ec) reports 354 files / 43.6% vs prior commits ~471 files / ~52%. Withcarryforward: false, a missingintegration-testsflag upload makes daemon-only paths look uncovered even when TAP hits them. That is separate from this PR; these unit tests improve real unit-test coverage regardless.ProxySQL_Admin_Tests2.cppis DEBUG-onlyPROXYSQLTESThandlers already exercised by TAP (firewall_commands1-t, etc.) and is not practical to drive from the unit harness without a full admin/daemon.Test plan
make -C test/tap/tests/unit parsersql_unit-t pgsql_query_processor_unit-t http_server_unit-t ffto_state_machine_unit-t restapi_server_unit-tSummary by CodeRabbit