Unit test infrastructure: Glo* stubs, init helpers, and smoke test#5482
Unit test infrastructure: Glo* stubs, init helpers, and smoke test#5482renecannao merged 79 commits intov3.0from
Conversation
Introduces the test infrastructure foundation for Phase 2.1 (#5473) of the unit testing framework (#5472). This enables unit test binaries to link against libproxysql.a without main.cpp, allowing individual components to be tested in isolation. New files: - test/tap/test_helpers/test_globals.h/.cpp: Stub definitions for all 25 Glo* global pointers and the PROXYSQL_EXTERN-based variable definitions (GloVars, MyHGM, PgHGM, __thread vars, tracked vars). Also stubs symbols from src/ (proxy_tls, SQLite3_Server, binary_sha1) and TAP noise testing functions. - test/tap/test_helpers/test_init.h/.cpp: Component initialization helpers (test_init_minimal, test_init_auth, test_init_query_cache, test_init_query_processor) with matching cleanup functions. All are idempotent and safe to call multiple times.
Adds the build system and initial smoke test for the unit test framework (Phase 2.1, #5473). - test/tap/tests/unit/Makefile: Compiles unit test binaries linked against libproxysql.a + test_globals.o. Compiles tap.o directly from tap.cpp to avoid the cpp-dotenv dependency chain, making unit tests buildable on both Linux and macOS. Handles platform-specific linker flags (Darwin vs Linux). - test/tap/tests/unit/smoke_test-t.cpp: Validates the test harness by exercising test_init_minimal() (GloVars setup), test_init_auth() (MySQL_Authentication add/lookup/exists cycle), and idempotency of all init/cleanup functions. Runs in <1 second with no Docker or network dependencies.
Adds unit_tests target to test/tap/Makefile so that unit tests are built alongside existing TAP tests via 'make build_tap_tests'. Also adds the unit test directory to clean/cleanall targets.
The 'ulong' type is not defined on macOS/Darwin. Add a conditional typedef to enable TAP test compilation on macOS without affecting Linux builds.
|
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:
📝 WalkthroughWalkthroughAdds a unit-test harness and build targets: new test globals and init helpers, multiple standalone unit tests and a tests/unit Makefile, integrates unit tests into the TAP Makefile, extracts connection-pool decision logic into a new header and uses it in pool code, and adds a macOS typedef for compilation. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Unit Test
participant Init as test_init helpers
participant Globals as test_globals
participant Auth as Auth subsystem
participant QC as Query_Cache subsystem
participant QP as Query_Processor
Test->>Init: test_init_minimal()
Init->>Globals: test_globals_init()
Globals-->>Init: globals configured
Test->>Init: test_init_auth()
Init->>Auth: allocate/assign GloMyAuth / GloPgAuth
Auth-->>Init: pointers set
Test->>Auth: add(user) / exists(user)
Auth-->>Test: responses
Test->>Init: test_init_query_cache()
Init->>QC: allocate GloMyQC / GloPgQC
QC-->>Test: set/get/flush/purgeHash/stats
Test->>Init: test_init_query_processor()
Init->>QP: allocate GloMyQPro / GloPgQPro
QP-->>Test: insert/sort/commit/get_rules/reset_all
Test->>Init: test_cleanup_*()
Init->>Globals: test_globals_cleanup()
Globals-->>Test: freed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a robust and isolated unit testing framework for ProxySQL components. It provides the necessary infrastructure to test individual modules without requiring a full daemon startup or external dependencies, significantly improving the ability to write reliable and fast component-level tests. The framework includes global variable stubs, component-specific initialization helpers, and a dedicated build system, culminating in a smoke test to validate its integrity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a solid foundation for unit testing in ProxySQL. The changes are well-structured, providing stubs for global variables, component initialization helpers, and a Makefile for building tests. The smoke test effectively validates the new infrastructure. I have a few suggestions to improve the robustness and coding style of the new test code.
| char datadir_buf[256]; | ||
| snprintf(datadir_buf, sizeof(datadir_buf), "%s/proxysql_unit_test_%d", | ||
| tmpdir, getpid()); | ||
|
|
||
| if (GloVars.datadir == nullptr) { | ||
| GloVars.datadir = strdup(datadir_buf); | ||
| } |
There was a problem hiding this comment.
The fixed-size buffer datadir_buf could be too small if the path in tmpdir is very long, leading to a truncated path. While snprintf prevents a buffer overflow, a truncated path can cause tests to fail in unexpected ways. Using std::string to construct the path dynamically would be more robust. You'll need to add #include <string> at the top of the file.
| char datadir_buf[256]; | |
| snprintf(datadir_buf, sizeof(datadir_buf), "%s/proxysql_unit_test_%d", | |
| tmpdir, getpid()); | |
| if (GloVars.datadir == nullptr) { | |
| GloVars.datadir = strdup(datadir_buf); | |
| } | |
| if (GloVars.datadir == nullptr) { | |
| std::string datadir_path = std::string(tmpdir) + "/proxysql_unit_test_" + std::to_string(getpid()); | |
| GloVars.datadir = strdup(datadir_path.c_str()); | |
| } |
| OPT := $(STDCPP) -O0 -ggdb $(PSQLCH) $(PSQLGA) $(PSQL31) $(PSQLFFTO) $(PSQLTSDB) \ | ||
| -DGITVERSION=\"$(GIT_VERSION)\" $(NOJEM) $(WGCOV) $(WASAN) \ | ||
| -Wl,--no-as-needed -Wl,-rpath,$(TAP_LDIR) |
There was a problem hiding this comment.
It's good practice to compile test code with a high warning level to catch potential issues early. Consider adding -Wall to the OPT compiler flags.
OPT := $(STDCPP) -O0 -ggdb -Wall $(PSQLCH) $(PSQLGA) $(PSQL31) $(PSQLFFTO) $(PSQLTSDB) \
-DGITVERSION=\"$(GIT_VERSION)\" $(NOJEM) $(WGCOV) $(WASAN) \
-Wl,--no-as-needed -Wl,-rpath,$(TAP_LDIR)
| bool added = GloMyAuth->add( | ||
| (char *)"testuser", // username | ||
| (char *)"testpass", // password | ||
| USERNAME_FRONTEND, // user type | ||
| false, // use_ssl | ||
| 0, // default_hostgroup | ||
| (char *)"", // default_schema | ||
| false, // schema_locked | ||
| false, // transaction_persistent | ||
| false, // fast_forward | ||
| 100, // max_connections | ||
| (char *)"", // attributes | ||
| (char *)"" // comment | ||
| ); |
There was a problem hiding this comment.
Using C-style casts like (char *) on string literals to cast away const is discouraged in C++ as it can lead to undefined behavior if the function modifies the string. It's safer and more idiomatic to use const_cast<char*>(). This applies to other similar calls in this file as well, for example in GloMyAuth->exists().
bool added = GloMyAuth->add(
const_cast<char *>("testuser"), // username
const_cast<char *>("testpass"), // password
USERNAME_FRONTEND, // user type
false, // use_ssl
0, // default_hostgroup
const_cast<char *>(""), // default_schema
false, // schema_locked
false, // transaction_persistent
false, // fast_forward
100, // max_connections
const_cast<char *>(""), // attributes
const_cast<char *>("") // comment
);There was a problem hiding this comment.
Pull request overview
Adds a new unit-test harness that allows building fast, component-level TAP unit tests by linking against libproxysql.a without main.cpp, using stubbed global singletons plus minimal init/cleanup helpers.
Changes:
- Introduces
test_globals.*andtest_init.*to provide Glo* stubs and idempotent component init/cleanup for unit tests. - Adds a new unit-test build system under
test/tap/tests/unit/and asmoke_test-tvalidating the harness. - Integrates unit tests into
test/tap/Makefileand adds a small macOSulongtypedef fix intap.cpp.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/tap/test_helpers/test_globals.h | Declares init/cleanup API for global stubs used by unit tests. |
| test/tap/test_helpers/test_globals.cpp | Defines stubbed Glo* globals, missing symbols, and minimal GloVars setup for linking unit tests without main.cpp. |
| test/tap/test_helpers/test_init.h | Declares component-level init/cleanup helpers (auth/query-cache/query-processor). |
| test/tap/test_helpers/test_init.cpp | Implements idempotent component init/cleanup helpers assigning real components to Glo* pointers. |
| test/tap/tests/unit/Makefile | Adds standalone unit-test build rules linking libproxysql.a + test stubs and compiling tap.o directly. |
| test/tap/tests/unit/smoke_test-t.cpp | Adds a TAP smoke test covering minimal init, auth add/exists, and idempotent cleanup. |
| test/tap/tap/tap.cpp | Adds ulong typedef for macOS compatibility. |
| test/tap/Makefile | Adds unit_tests target and hooks it into all/debug and clean. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/tap/Makefile
Outdated
| .PHONY: unit_tests | ||
| unit_tests: tap | ||
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) |
There was a problem hiding this comment.
This unit_tests target depends on tap, which triggers building libtap.so and the cpp-dotenv toolchain (cmake) in test/tap/tap/Makefile. That contradicts the stated goal of making unit tests build without the cpp-dotenv dependency chain, and can break environments where tap doesn't build. Consider making unit_tests depend on test_deps (or nothing) instead of tap, since the unit tests compile tap.cpp directly.
| bool added = GloMyAuth->add( | ||
| (char *)"testuser", // username | ||
| (char *)"testpass", // password | ||
| USERNAME_FRONTEND, // user type | ||
| false, // use_ssl | ||
| 0, // default_hostgroup | ||
| (char *)"", // default_schema | ||
| false, // schema_locked | ||
| false, // transaction_persistent | ||
| false, // fast_forward | ||
| 100, // max_connections | ||
| (char *)"", // attributes | ||
| (char *)"" // comment | ||
| ); |
There was a problem hiding this comment.
The test passes string literals to APIs expecting char* via C-style casts (e.g. (char *)"testuser"). If any callee writes to the buffer, this is undefined behavior and can crash. Prefer using mutable buffers (e.g., stack char[]) or strdup()/std::string backing storage so the char* points to writable memory.
| GloVars.global.gr_bootstrap_mode = 0; | ||
| GloVars.global.gr_bootstrap_uri = nullptr; | ||
| GloVars.global.data_packets_history_size = 0; | ||
|
|
There was a problem hiding this comment.
test_globals_init() sets various GloVars.global.* flags but never initializes glovars.has_debug like ProxySQL_Main_init() does. Several component constructors (e.g., MySQL_Authentication, Query_Cache) exit with "Incompatible debugging version" if glovars.has_debug doesn't match the build type, so DEBUG builds of the unit tests will fail immediately. Initialize glovars.has_debug in test_globals_init() (and any other required fields from ProxySQL_Main_init() that unit-tested components depend on).
| // Ensure the global debug flag matches the build type so that | |
| // components which validate debug compatibility in their constructors | |
| // (e.g., MySQL_Authentication, Query_Cache) behave as they do in | |
| // ProxySQL_Main_init(). | |
| #ifndef NDEBUG | |
| glovars.has_debug = 1; | |
| #else | |
| glovars.has_debug = 0; | |
| #endif |
test/tap/tests/unit/Makefile
Outdated
| UNIT_TESTS := smoke_test-t | ||
|
|
||
| .DEFAULT: default | ||
| .PHONY: default debug all | ||
|
|
||
| default: all | ||
| debug: OPT += -DDEBUG | ||
| debug: all | ||
|
|
||
| all: $(UNIT_TESTS) |
There was a problem hiding this comment.
This Makefile doesn't set a proper default goal: the first real target is $(ODIR), and .DEFAULT: default does not make default the default build target. As a result, running make in this directory (or invoking it from the parent Makefile with no explicit goal) may only create obj/ instead of building unit tests. Set .DEFAULT_GOAL := all (or make all/default the first target) so unit tests build by default.
test/tap/Makefile
Outdated
|
|
||
| .PHONY: unit_tests | ||
| unit_tests: tap | ||
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) |
There was a problem hiding this comment.
unit_tests invokes the sub-make with $(MAKECMDGOALS). If a developer runs make unit_tests here, the sub-make will be called as make unit_tests, but the unit-test Makefile doesn't define a unit_tests target, so the build will fail (or do the wrong thing depending on .DEFAULT). Consider invoking the sub-make with an explicit goal (e.g. all/clean) or adding a unit_tests: alias target in tests/unit/Makefile.
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) | |
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} |
Fixes from Copilot and Greptile review comments: - test_globals.cpp: Replace fixed-size datadir_buf[256] with std::string to avoid truncation on long TMPDIR paths - test_globals.cpp: Initialize glovars.has_debug to match build type (DEBUG vs release) so component constructors that validate debug compatibility do not abort - test/tap/Makefile: Remove 'tap' dependency from unit_tests target since unit tests compile tap.o directly and don't need the full libtap.so/cpp-dotenv build chain. Also remove $(MAKECMDGOALS) pass- through which would cause failures when invoked as 'make unit_tests' - tests/unit/Makefile: Fix default goal — make 'all' the first target so 'make' without arguments builds unit tests correctly
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/Makefile`:
- Around line 32-34: The unit_tests target currently forwards $(MAKECMDGOALS)
into the child make, which causes failures because the child Makefile only
defines default/all/debug/clean; change the unit_tests recipe so it invokes the
child make with a concrete child goal (for example "all" or "default") instead
of $(MAKECMDGOALS) — update the unit_tests target (which depends on tap) to run:
cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} all (or map parent goals to child
goals explicitly) so unit_tests no longer forwards unknown goals into the child
Makefile.
In `@test/tap/test_helpers/test_globals.cpp`:
- Around line 254-256: test_globals_init() currently strdup's into
GloVars.datadir only when it's null, but test_globals_cleanup() unconditionally
frees GloVars.datadir causing potential double/invalid frees if a test
pre-seeded or replaced the pointer; add ownership tracking (e.g., a bool
GloVars.datadir_owned) and set it true only when you allocate with strdup inside
test_globals_init() (where GloVars.datadir is set from strdup(datadir_buf)),
then in test_globals_cleanup() only free GloVars.datadir if datadir_owned is
true (and clear the pointer and owned flag afterward); apply the same
ownership-flag pattern for the other fields noted around lines 266-269.
- Around line 223-238: test_globals_init() currently doesn't set
GloVars.global.has_debug, causing MySQL_Authentication() to abort when the build
is compiled with DEBUG; set GloVars.global.has_debug to reflect the build mode
(true for debug builds, false otherwise) inside test_globals_init() so tests run
under both debug and non-debug builds; update the initialization alongside the
other GloVars.global fields (referencing GloVars.global.has_debug and
test_globals_init() and MySQL_Authentication()/test_init_auth() to locate the
relevant code).
In `@test/tap/test_helpers/test_init.cpp`:
- Around line 52-60: The init helper test_init_auth currently returns success if
either GloMyAuth or GloPgAuth is non-null, risking a half-initialized state;
change it so it guarantees both singletons are valid: if both are non-null
return success, if one is missing then either delete/cleanup the existing one
and recreate both, or construct the missing one so that after test_init_auth
returns both GloMyAuth and GloPgAuth are non-null; apply the identical
fix/pattern to test_init_query_cache to avoid symmetric half-initialization of
its globals.
- Around line 109-115: test_init_query_processor() currently constructs
MySQL_Query_Processor and PgSQL_Query_Processor while GloMTH/GloPTH may be
nullptr (their constructors call GloMTH->get_variable_int), causing crashes; fix
by ensuring the thread-handler globals are initialized before creating the
processors—either call test_init_minimal() at the start of
test_init_query_processor() if GloMTH/GloPTH are null, or explicitly allocate
and assign minimal/mock instances to GloMTH and GloPTH before invoking new
MySQL_Query_Processor() and new PgSQL_Query_Processor(), then proceed as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f72763c1-3206-4907-b529-c783d3a8c58f
📒 Files selected for processing (8)
test/tap/Makefiletest/tap/tap/tap.cpptest/tap/test_helpers/test_globals.cpptest/tap/test_helpers/test_globals.htest/tap/test_helpers/test_init.cpptest/tap/test_helpers/test_init.htest/tap/tests/unit/Makefiletest/tap/tests/unit/smoke_test-t.cpp
test/tap/Makefile
Outdated
| .PHONY: unit_tests | ||
| unit_tests: tap | ||
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) |
There was a problem hiding this comment.
Don't forward unit_tests into the child Makefile.
test/tap/tests/unit/Makefile only defines default, all, debug, and clean. If someone runs make unit_tests, this expands to make unit_tests inside tests/unit, which fails with “No rule to make target unit_tests”. Map the parent goal to a child goal instead of forwarding $(MAKECMDGOALS) verbatim.
🛠️ Suggested fix
.PHONY: unit_tests
unit_tests: tap
- cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS)
+ cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(if $(filter debug,$(MAKECMDGOALS)),debug,all)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: unit_tests | |
| unit_tests: tap | |
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(MAKECMDGOALS) | |
| .PHONY: unit_tests | |
| unit_tests: tap | |
| cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} $(if $(filter debug,$(MAKECMDGOALS)),debug,all) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/Makefile` around lines 32 - 34, The unit_tests target currently
forwards $(MAKECMDGOALS) into the child make, which causes failures because the
child Makefile only defines default/all/debug/clean; change the unit_tests
recipe so it invokes the child make with a concrete child goal (for example
"all" or "default") instead of $(MAKECMDGOALS) — update the unit_tests target
(which depends on tap) to run: cd tests/unit && CC=${CC} CXX=${CXX} ${MAKE} all
(or map parent goals to child goals explicitly) so unit_tests no longer forwards
unknown goals into the child Makefile.
| if (GloVars.datadir == nullptr) { | ||
| GloVars.datadir = strdup(datadir_buf); | ||
| } |
There was a problem hiding this comment.
Track ownership of GloVars.datadir before freeing it.
test_globals_init() only allocates datadir when it is null, but test_globals_cleanup() frees whatever pointer is there. If a test pre-seeds or replaces GloVars.datadir, cleanup turns into an invalid free or double free.
🛠️ Suggested fix
+static char *owned_test_datadir = nullptr;
+
int test_globals_init() {
// ...
- if (GloVars.datadir == nullptr) {
- GloVars.datadir = strdup(datadir_buf);
+ if (GloVars.datadir == nullptr && owned_test_datadir == nullptr) {
+ owned_test_datadir = strdup(datadir_buf);
+ GloVars.datadir = owned_test_datadir;
}
return 0;
}
void test_globals_cleanup() {
- if (GloVars.datadir != nullptr) {
- free(GloVars.datadir);
- GloVars.datadir = nullptr;
+ if (owned_test_datadir != nullptr) {
+ free(owned_test_datadir);
+ owned_test_datadir = nullptr;
}
+ GloVars.datadir = nullptr;
}Also applies to: 266-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/test_helpers/test_globals.cpp` around lines 254 - 256,
test_globals_init() currently strdup's into GloVars.datadir only when it's null,
but test_globals_cleanup() unconditionally frees GloVars.datadir causing
potential double/invalid frees if a test pre-seeded or replaced the pointer; add
ownership tracking (e.g., a bool GloVars.datadir_owned) and set it true only
when you allocate with strdup inside test_globals_init() (where GloVars.datadir
is set from strdup(datadir_buf)), then in test_globals_cleanup() only free
GloVars.datadir if datadir_owned is true (and clear the pointer and owned flag
afterward); apply the same ownership-flag pattern for the other fields noted
around lines 266-269.
| int test_init_auth() { | ||
| if (GloMyAuth != nullptr || GloPgAuth != nullptr) { | ||
| // Already initialized — idempotent | ||
| return 0; | ||
| } | ||
|
|
||
| GloMyAuth = new MySQL_Authentication(); | ||
| GloPgAuth = new PgSQL_Authentication(); | ||
|
|
There was a problem hiding this comment.
Don't report success from half-initialized auth/query-cache state.
These helpers return 0 as soon as either singleton is non-null. If one side exists and the other does not, callers get a “successful” init with a null dependency still left behind. Recreate the missing side, or tear down and rebuild both.
🛠️ Suggested fix
int test_init_auth() {
- if (GloMyAuth != nullptr || GloPgAuth != nullptr) {
- // Already initialized — idempotent
- return 0;
- }
-
- GloMyAuth = new MySQL_Authentication();
- GloPgAuth = new PgSQL_Authentication();
+ if (GloMyAuth == nullptr) {
+ GloMyAuth = new MySQL_Authentication();
+ }
+ if (GloPgAuth == nullptr) {
+ GloPgAuth = new PgSQL_Authentication();
+ }
return 0;
}Apply the same pattern to test_init_query_cache().
Also applies to: 79-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/test_helpers/test_init.cpp` around lines 52 - 60, The init helper
test_init_auth currently returns success if either GloMyAuth or GloPgAuth is
non-null, risking a half-initialized state; change it so it guarantees both
singletons are valid: if both are non-null return success, if one is missing
then either delete/cleanup the existing one and recreate both, or construct the
missing one so that after test_init_auth returns both GloMyAuth and GloPgAuth
are non-null; apply the identical fix/pattern to test_init_query_cache to avoid
symmetric half-initialization of its globals.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/tap/test_helpers/test_globals.cpp (1)
259-263:⚠️ Potential issue | 🟠 MajorAvoid freeing externally-owned
GloVars.datadirin cleanup.
test_globals_init()only allocatesGloVars.datadirwhen null, buttest_globals_cleanup()frees it unconditionally when non-null. If a test setsGloVars.datadiritself, cleanup can hit an invalid/double free.💡 Suggested fix
+static char* g_test_owned_datadir = nullptr; + int test_globals_init() { @@ - if (GloVars.datadir == nullptr) { + if (GloVars.datadir == nullptr && g_test_owned_datadir == nullptr) { std::string datadir_path = std::string(tmpdir) + "/proxysql_unit_test_" + std::to_string(getpid()); - GloVars.datadir = strdup(datadir_path.c_str()); + g_test_owned_datadir = strdup(datadir_path.c_str()); + GloVars.datadir = g_test_owned_datadir; } @@ void test_globals_cleanup() { - if (GloVars.datadir != nullptr) { - free(GloVars.datadir); - GloVars.datadir = nullptr; + if (g_test_owned_datadir != nullptr) { + free(g_test_owned_datadir); + g_test_owned_datadir = nullptr; } + GloVars.datadir = nullptr; }Also applies to: 273-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/test_helpers/test_globals.cpp` around lines 259 - 263, test_globals_cleanup() unconditionally frees GloVars.datadir even when it was set externally; change the logic so test_globals_init() records ownership (e.g., a bool flag like datadir_owned) when it allocates and only free GloVars.datadir in test_globals_cleanup() if that ownership flag is true; update test_globals_init() (where GloVars.datadir is strdup'd when null) to set datadir_owned = true and ensure test_globals_cleanup() checks datadir_owned before calling free and clears the flag after freeing; apply the same ownership pattern for the other similar allocation at the 273-277 site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/tests/unit/Makefile`:
- Line 12: The PROXYSQL_PATH assignment uses a shell while loop that can
infinite-loop if src/proxysql_global.cpp is missing (e.g., at filesystem root);
modify the logic that sets PROXYSQL_PATH to break/fail when the filesystem root
is reached: in the shell snippet that defines PROXYSQL_PATH (the line using
while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd), add a check each
iteration to compare current directory to parent (or test for "/") and exit with
a non-zero status or print an error if root is reached so the Makefile fails
fast instead of looping forever. Ensure the updated logic still returns the
discovered path when found and preserves existing behavior otherwise.
---
Duplicate comments:
In `@test/tap/test_helpers/test_globals.cpp`:
- Around line 259-263: test_globals_cleanup() unconditionally frees
GloVars.datadir even when it was set externally; change the logic so
test_globals_init() records ownership (e.g., a bool flag like datadir_owned)
when it allocates and only free GloVars.datadir in test_globals_cleanup() if
that ownership flag is true; update test_globals_init() (where GloVars.datadir
is strdup'd when null) to set datadir_owned = true and ensure
test_globals_cleanup() checks datadir_owned before calling free and clears the
flag after freeing; apply the same ownership pattern for the other similar
allocation at the 273-277 site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 758dc913-eb5d-4af3-b858-1df5e81b5845
📒 Files selected for processing (3)
test/tap/Makefiletest/tap/test_helpers/test_globals.cpptest/tap/tests/unit/Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tap/Makefile
| # See: GitHub issue #5473 (Phase 2.1: Test Infrastructure Foundation) | ||
|
|
||
|
|
||
| PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd) |
There was a problem hiding this comment.
Guard repo-root discovery to prevent infinite loop.
The current loop can hang forever when src/proxysql_global.cpp is not found (at /, cd .. is a no-op). Please fail fast when root is reached.
💡 Suggested fix
-PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd)
+PROXYSQL_PATH := $(shell \
+ d=$$(pwd); \
+ while [ ! -f "$$d/src/proxysql_global.cpp" ]; do \
+ if [ "$$d" = "/" ]; then \
+ echo "Could not locate repository root (missing src/proxysql_global.cpp)" >&2; \
+ exit 1; \
+ fi; \
+ d=$$(dirname "$$d"); \
+ done; \
+ echo "$$d")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PROXYSQL_PATH := $(shell while [ ! -f ./src/proxysql_global.cpp ]; do cd ..; done; pwd) | |
| PROXYSQL_PATH := $(shell \ | |
| d=$$(pwd); \ | |
| while [ ! -f "$$d/src/proxysql_global.cpp" ]; do \ | |
| if [ "$$d" = "/" ]; then \ | |
| echo "Could not locate repository root (missing src/proxysql_global.cpp)" >&2; \ | |
| exit 1; \ | |
| fi; \ | |
| d=$$(dirname "$$d"); \ | |
| done; \ | |
| echo "$$d") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/tap/tests/unit/Makefile` at line 12, The PROXYSQL_PATH assignment uses a
shell while loop that can infinite-loop if src/proxysql_global.cpp is missing
(e.g., at filesystem root); modify the logic that sets PROXYSQL_PATH to
break/fail when the filesystem root is reached: in the shell snippet that
defines PROXYSQL_PATH (the line using while [ ! -f ./src/proxysql_global.cpp ];
do cd ..; done; pwd), add a check each iteration to compare current directory to
parent (or test for "/") and exit with a non-zero status or print an error if
root is reached so the Makefile fails fast instead of looping forever. Ensure
the updated logic still returns the discovered path when found and preserves
existing behavior otherwise.
Comprehensive unit tests for MySQL_Query_Cache and PgSQL_Query_Cache covering 26 test cases across 11 test functions. Runs in <0.01s with no infrastructure dependencies. PgSQL_Query_Cache is used for most tests because its set()/get() API is simpler (no MySQL protocol parsing required). The underlying Query_Cache<T> template logic is identical for both protocols. Test coverage: - Basic set/get cycle with value verification - Cache miss on nonexistent key - User hash isolation (same key, different user_hash = miss) - Cache replacement (same key overwrites previous value) - Hard TTL expiration (expire_ms in the past) - Soft TTL expiration (cache_ttl parameter to get()) - flush() clears all entries and returns correct count - Set/flush cycle (entry retrievable then unretrievable) - Stats counters (SET and GET via SQL3_getStats()) - SQL3_getStats() result format (2 columns, has rows) - MySQL_Query_Cache construction, flush, and SQL3_getStats() - purgeHash() eviction of expired entries (live entries survive) - Bulk insert (100 entries) across 32 internal hash tables Key implementation note: Query_Cache constructor requires a valid Prometheus registry (GloVars.prometheus_registry). The test_init_ query_cache() helper creates one automatically.
- tests/unit/Makefile: Register query_cache_unit-t in the build system - test_init.cpp: Create a real Prometheus registry in test_init_query_ cache() since the Query_Cache constructor registers metrics via GloVars.prometheus_registry (nullptr would crash)
- test_pgsql_flush: flush() at start for clean state, assert == 10 - test_mysql_construction_and_flush: fix tautology (flushed == 0 || flushed >= 0 is always true for uint64_t), double-flush to ensure empty cache, then assert == 0 - test_pgsql_purge_expired: use max_memory_size=1 instead of 1GB to force the purge path to actually run (3% threshold wasn't reached) - test_pgsql_many_entries: fix misleading docstring claiming hash table distribution verification
Query Cache unit tests (Phase 2.3)
Comprehensive unit tests for MySQL_Query_Processor and PgSQL_Query_ Processor covering 42 test cases across 10 test functions. Runs in <0.01s with no infrastructure dependencies. Test coverage: - Rule creation via new_query_rule() with all 35 fields verified - Rule field storage: username, schemaname, match_digest, match_pattern, destination_hostgroup, cache_ttl, timeout, retries, sticky_conn, multiplex, apply, log, client_addr, comment - Regex modifier parsing: CASELESS, GLOBAL, combined (CASELESS,GLOBAL) - Rule insertion, sorting by rule_id, and commit - Rule retrieval via get_current_query_rules() SQLite3 result - Sort verification: rules inserted in reverse order, sorted ascending - Special fields: error_msg, OK_msg, replace_pattern - flagIN/flagOUT chaining setup - Username filter rules - reset_all() clears all rules - get_stats_commands_counters() returns valid result - PgSQL rule creation, insertion, and reset Note: Full process_query() testing requires a MySQL_Session with populated connection data, which is beyond isolated unit tests. The existing E2E TAP tests cover those scenarios.
- tests/unit/Makefile: Register query_processor_unit-t in build system - test_init.cpp: Create MySQL_Threads_Handler and PgSQL_Threads_Handler in test_init_query_processor() since QP constructors read variables from GloMTH/GloPTH. Trigger lazy initialization of VariablesPointers maps via get_variables_list() before constructing QP instances. Add extern declaration for GloPTH (not declared in any header).
- test_mysql_reset_all: remove 2 leaked mysql_simple_rule() calls whose return values were discarded (memory leak under ASAN) - test_mysql_rule_creation: fix misleading "Don't free" comment — rule is NOT inserted into QP and IS freed manually - test_init.cpp: free individual strings from get_variables_list() before freeing the array (was leaking all variable name strings) - test_init.cpp: fix misleading comment claiming GloMTH/GloPTH are cleaned up by test_cleanup_minimal() — they rely on process exit
Query Processor rule management unit tests (Phase 2.4)
Unit tests for standalone protocol functions and utility routines covering 43 test cases across 11 test functions. Runs in <0.01s with no infrastructure dependencies. Test coverage: - MySQL length-encoded integer decoding: 1-byte (0-250), 2-byte (0xFC prefix), 3-byte (0xFD prefix), 8-byte (0xFE prefix) - MySQL length-encoded integer encoding: boundary values at each encoding tier (251, 65535, 65536, 16777216) - Encode/decode roundtrip: 10 values across all encoding ranges survive write_encoded_length() → mysql_decode_length_ll() - mysql_hdr packet header: structure size (4 bytes), field packing, 24-bit max length - CPY3/CPY8 byte copy helpers: little-endian semantics, boundary values - MySQL query digest: normalization via mysql_query_digest_and_first_ comment_2(), whitespace collapsing, comment handling, empty query - PgSQL query digest: normalization via pgsql_query_digest_and_first_ comment_2(), whitespace collapsing - escape_string_single_quotes: no-op on clean strings, single quote doubling, multiple quotes - mywildcmp wildcard matching: exact match, % prefix/suffix/both, _ single char, non-match cases, empty string edge cases
- test_decode_length_3byte: pad buffer to 5 bytes (CPY3 reads 4 bytes via uint32_t* cast from buf+1, causing OOB read with 4-byte buffer) - test_cpy3: use 4-byte buffers with padding to avoid OOB read - test_encode_decode_roundtrip: initialize prefix[0]=0 each iteration to avoid UB when mysql_encode_length doesn't write for 1-byte values - test_mysql_query_digest: free first_comment between calls to prevent memory leak (malloc'd by digest function when comment present) - test_pgsql_query_digest: use strlen() instead of hardcoded q_len to avoid off-by-one, free first_comment after use - test_escape_single_quotes: fix misleading comment — function returns original pointer (not copy) when no quotes are present
Protocol encoding/decoding and utility unit tests (Phase 2.5)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/tap/tests/unit/Makefile (1)
248-256: Consider adding header dependencies for incremental builds.The unit test targets only depend on the
.cppsource and helper objects. Changes to headers (e.g.,test_init.h,test_globals.h, or ProxySQL headers) won't trigger rebuilds.♻️ Optional: Add header dependencies
+TEST_HELPERS_HDRS := $(TEST_HELPERS_DIR)/test_globals.h $(TEST_HELPERS_DIR)/test_init.h + -smoke_test-t: smoke_test-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) +smoke_test-t: smoke_test-t.cpp $(TEST_HELPERS_OBJ) $(TEST_HELPERS_HDRS) $(LIBPROXYSQLAR) $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ $(ALLOW_MULTI_DEF) -o $@ -query_cache_unit-t: query_cache_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) +query_cache_unit-t: query_cache_unit-t.cpp $(TEST_HELPERS_OBJ) $(TEST_HELPERS_HDRS) $(LIBPROXYSQLAR) $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \ $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \ $(ALLOW_MULTI_DEF) -o $@🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/Makefile` around lines 248 - 256, The smoke_test-t and query_cache_unit-t targets only list the .cpp and $(TEST_HELPERS_OBJ)/$(LIBPROXYSQLAR) which means header changes won't trigger rebuilds; update the Makefile so these targets depend on the relevant header files (e.g., test_init.h, test_globals.h and any ProxySQL headers) or wire in auto-generated dependency files (via -MMD/-MF during compilation and including the .d files) so that changes to headers cause incremental rebuilds; reference the targets smoke_test-t and query_cache_unit-t and the variables $(TEST_HELPERS_OBJ) and $(LIBPROXYSQLAR) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/tap/tests/unit/Makefile`:
- Around line 248-256: The smoke_test-t and query_cache_unit-t targets only list
the .cpp and $(TEST_HELPERS_OBJ)/$(LIBPROXYSQLAR) which means header changes
won't trigger rebuilds; update the Makefile so these targets depend on the
relevant header files (e.g., test_init.h, test_globals.h and any ProxySQL
headers) or wire in auto-generated dependency files (via -MMD/-MF during
compilation and including the .d files) so that changes to headers cause
incremental rebuilds; reference the targets smoke_test-t and query_cache_unit-t
and the variables $(TEST_HELPERS_OBJ) and $(LIBPROXYSQLAR) when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21d654ef-6ce5-48c4-9d4a-2910a50f3ce0
📒 Files selected for processing (3)
test/tap/test_helpers/test_init.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/query_cache_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
📚 Learning: 2026-01-20T09:34:27.165Z
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:27.165Z
Learning: In ProxySQL test files (test/tap/tests/), resource leaks (such as not calling `mysql_close()` on early return paths) are not typically fixed because test processes are short-lived and the OS frees resources on process exit. This is a common pattern across the test suite.
Applied to files:
test/tap/tests/unit/Makefiletest/tap/test_helpers/test_init.cpp
📚 Learning: 2026-01-20T07:40:34.938Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:24-28
Timestamp: 2026-01-20T07:40:34.938Z
Learning: In ProxySQL test files, calling `mysql_error(NULL)` after `mysql_init()` failure is safe because the MariaDB client library implementation returns an empty string for NULL handles (not undefined behavior).
Applied to files:
test/tap/test_helpers/test_init.cpp
📚 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/query_cache_unit-t.cpp
🪛 checkmake (0.2.2)
test/tap/tests/unit/Makefile
[warning] 263-263: Missing required phony target "test"
(minphony)
🪛 Clang (14.0.6)
test/tap/tests/unit/query_cache_unit-t.cpp
[error] 28-28: 'tap.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/test_helpers/test_init.cpp
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 84-84: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 85-85: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
test/tap/tests/unit/query_cache_unit-t.cpp
[error] 149-149: Common realloc mistake
(memleakOnRealloc)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: 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)
[warning] 66-66: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 112-112: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 140-140: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 144-144: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 175-175: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 197-197: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 228-228: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 276-276: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 309-309: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 398-398: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 409-409: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (18)
test/tap/test_helpers/test_init.cpp (5)
52-62: Half-initialized state still possible with||condition.The condition
GloMyAuth != nullptr || GloPgAuth != nullptrreturns success if either singleton exists, leaving the other potentially null. This was already flagged in a previous review.
79-99: Same half-initialized state issue applies here.Lines 80-82 use the same
||pattern. IfGloMyQCis non-null butGloPgQCis null (or vice versa), callers get a "successful" init with a partial state.The Prometheus registry initialization on lines 87-89 is a good defensive measure.
116-125: Potential null dereference in query processor constructors.This was flagged in a previous review. The
MySQL_Query_ProcessorandPgSQL_Query_Processorconstructors may accessGloMTH/GloPTHwhich remain null even aftertest_init_minimal(). The current code does not guard against this.
40-46: LGTM!Clean delegation to
test_globals_init()/test_globals_cleanup(). The wrapper functions provide a clear abstraction for minimal test setup.
64-73: LGTM!Cleanup functions correctly check for null before deleting and reset pointers to nullptr, preventing double-free and use-after-free issues.
Also applies to: 101-110, 127-136
test/tap/tests/unit/Makefile (4)
12-12: Infinite loop risk still present.The while loop can hang indefinitely if
src/proxysql_global.cppis not found (e.g., running make from an unrelated directory). This was flagged in a previous review.
22-42: LGTM!Include and library directories are well-organized with appropriate platform conditionals for Darwin. The ClickHouse integration follows the existing pattern.
Also applies to: 49-74, 81-87
100-115: LGTM!Static library selection correctly handles conditional features (PROXYSQLCLICKHOUSE, PROXYSQLGENAI) and platform-specific libraries (coredumper for Linux only).
208-227: LGTM!Compiling
tap.odirectly from source with-w(suppress warnings) is appropriate for avoiding the cpp-dotenv dependency chain while accepting that third-party code may have warnings. The test helper objects use-Wallas they should.test/tap/tests/unit/query_cache_unit-t.cpp (9)
1-47: LGTM!Clean file header with comprehensive documentation of test coverage. The
now_ms()helper correctly convertsmonotonic_time()from microseconds to milliseconds.
59-88: LGTM!Good test structure with proper handling of the null entry case to maintain TAP test count consistency. The memory ownership model (cache takes ownership of malloc'd buffer) matches the API contract from
PgSQL_Query_Cache::set().
93-124: LGTM!Good coverage of cache miss behavior and user_hash isolation - important for verifying that per-user caching works correctly.
133-155: LGTM!Cache replacement and TTL tests cover important edge cases. The expired entry test (line 179:
t - 1for expire_ms) correctly tests the boundary condition, and soft TTL test properly validates thecache_ttlparameter behavior.Also applies to: 167-209
218-243: LGTM!Good test hygiene - calling
flush()at line 220 before the test ensures clean state. The test correctly verifies both the return value and actual cache behavior.
253-265: LGTM!Clean helper function for extracting stats. Properly iterates through the SQLite3_result and cleans up with
delete result.
389-426: LGTM!Good purge test setup - creates 5 already-expired entries and 1 live entry, then verifies purgeHash() evicts expired entries while preserving the live one. The
purgeHash(1)call with minimal memory forces the eviction path.
476-503: LGTM!Main function has correct test plan count (26), properly sequences init/cleanup, and follows the TAP pattern with
exit_status()return. The test count calculation in comments (lines 496-497) is accurate: 4+1+2+2+1+1+2+2+2+3+3+1+2 = 26.
438-470: Potential flakiness: bulk insertion with same user_hash may cause key collisions.All 100 entries use
user_hash = 80000but unique key strings. While this should work correctly (keys are compared, not just hashes), verify that the cache implementation correctly handles many entries with the same user_hash.#!/bin/bash # Check how user_hash is used in the Query_Cache implementation rg -n "user_hash" --type=cpp lib/Query_Cache.cpp -A3 -B3 | head -60
Phase 3.8: Transaction state tracking extraction + unit tests
New files: - include/PgSQLCommandComplete.h: PgSQLCommandResult struct + parse_pgsql_command_complete() declaration - lib/PgSQLCommandComplete.cpp: implementation mirroring extract_pg_rows_affected() from PgSQLFFTO.cpp Parses PostgreSQL CommandComplete tags (INSERT/UPDATE/DELETE/SELECT/ FETCH/MOVE/COPY/MERGE) to extract row counts. Pure function with no FFTO or session dependencies.
17 test cases: DML commands (INSERT/UPDATE/DELETE/COPY/MERGE), SELECT commands (SELECT/FETCH/MOVE), no-row-count commands (CREATE TABLE/DROP INDEX/BEGIN), edge cases (null, empty, whitespace, INSERT with OID, large counts).
New: include/MySQLErrorClassifier.h, lib/MySQLErrorClassifier.cpp Functions: - classify_mysql_error(): classifies error codes as retryable (1047 WSREP, 1053 shutdown) or fatal, checking retry conditions - can_retry_on_new_connection(): checks if offline server retry is possible given connection state (reusable, no txn, no transfer)
19 test cases: retryable errors with/without conditions, non-retryable errors (access denied, syntax, table not found, gone away), offline retry with all blocking conditions tested individually.
New: include/BackendSyncDecision.h, lib/BackendSyncDecision.cpp determine_backend_sync_actions() returns bitmask of needed sync: - SYNC_USER: username mismatch → CHANGE USER required - SYNC_SCHEMA: schema mismatch → USE <db> required (skipped if SYNC_USER set, since CHANGE USER handles schema) - SYNC_AUTOCOMMIT: autocommit state mismatch Covers both MySQL (7 verify functions) and PgSQL (2 verify functions) at the decision level — the core comparisons are the same.
15 test cases: no sync needed, schema mismatch, user mismatch, user+schema (schema handled by user change), autocommit mismatch, multiple mismatches, null handling.
New: include/PgSQLMonitorDecision.h, lib/PgSQLMonitorDecision.cpp Functions: - pgsql_should_shun_on_ping_failure(): threshold-based shunning (simpler than MySQL — always aggressive with kill_all) - pgsql_should_offline_for_readonly(): read-only server in writer hostgroup should go OFFLINE_SOFT Unshun recovery is already covered by MonitorHealthDecision.h can_unshun_server() (same logic for both protocols).
11 test cases: ping shunning (threshold, boundary, disabled), read-only offline (writer/reader HG combinations).
New: include/PgSQLErrorClassifier.h, lib/PgSQLErrorClassifier.cpp Functions: - classify_pgsql_error(): classifies by SQLSTATE class — connection (08), transaction rollback (40), resources (53) = retryable; operator intervention (57), system error (58) = fatal; all others (syntax 42, constraints 23, data 22) = report to client - pgsql_can_retry_error(): checks retry conditions (retries left, not in transaction — PgSQL transactions are atomic)
25 test cases: connection errors (08xxx), transaction errors (40xxx), resource errors (53xxx), fatal errors (57xxx/58xxx), non-retryable (syntax/constraint/data), edge cases (null/empty), retry conditions (retries, in-transaction, non-retryable, fatal).
New: include/MySQLProtocolUtils.h, lib/MySQLProtocolUtils.cpp Functions: - mysql_read_lenenc_int(): reads MySQL length-encoded integers from raw buffers (mirrors read_lenenc_int in MySQLFFTO.cpp) - mysql_build_packet(): constructs MySQL wire-format packets (3-byte length + seq_id + payload) for crafting test data
36 test cases covering both MySQL and PgSQL FFTO protocol parsing: MySQL length-encoded integers (11 tests): - 1-byte, 2-byte, 3-byte, 8-byte values - Truncated buffers, empty input MySQL packet building (9 tests): - Normal, large (1000 bytes), empty packets - Header validation (length, seq_id, payload integrity) MySQL OK packet parsing (2 tests): - affected_rows extraction (1-byte and 2-byte lenenc) PgSQL CommandComplete extended (7 tests): - INSERT with OID, large row count, zero rows - All 8 command types in one sweep, 9 DDL commands - Null-terminated payload (real wire format) Fragmentation simulation (6 tests): - Truncated lenenc (partial data) - Multi-packet stream building and header verification
Signed-off-by: René Cannaò <rene@proxysql.com>
- BackendSyncDecision.cpp: treat asymmetric NULLs as mismatches (client=NULL + backend="bob" → SYNC_USER, not SYNC_NONE) - Tests: replace weak null assertions (a >= 0 always true) with specific checks for SYNC_USER/SYNC_SCHEMA on asymmetric NULLs, both-null → no sync, schema asymmetric null (+2 tests, plan 15→17)
Phase 3.4: Server selection algorithm extraction + unit tests
- Remove unused MYSQL_ERROR_CONTINUE enum value - Clarify retries_remaining parameter semantics in docstring
- Fix: 57014 (query_canceled) is now classified as REPORT_TO_CLIENT instead of FATAL (it's a normal cancellation, not a server crash) - Fix docstring: crash_shutdown is 57P02 (class 57), not class 58; class 58 is system/I/O errors - Add test for 57014 exception (+1 test, plan 25→26)
Phase 3.6: Backend variable sync decisions + unit tests
Signed-off-by: René Cannaò <rene@proxysql.com>
Phase 3.7: MySQL error classification + unit tests
Signed-off-by: René Cannaò <rene@proxysql.com>
Phase 3.9: PgSQL monitor health decisions + unit tests
Signed-off-by: René Cannaò <rene@proxysql.com>
Phase 3.10: PgSQL error classification + unit tests
Signed-off-by: René Cannaò <rene@proxysql.com>
FFTO: PgSQL CommandComplete tag parser extraction + unit tests
|
Summary
Implements Phase 2.1 of the Unit Testing Framework (#5472, #5473): the foundational test infrastructure that enables component-level unit tests for ProxySQL.
test/tap/test_helpers/): Stub definitions for all 25 Glo* global singletons and aPROXYSQL_EXTERN-based mechanism that providesGloVars,MyHGM,PgHGM, all__threadvariables, and tracked variable arrays — allowing test binaries to link againstlibproxysql.awithoutmain.cpptest_init_minimal(),test_init_auth(),test_init_query_cache(),test_init_query_processor()with matching cleanup functions — all idempotent and leak-freetest/tap/tests/unit/): Cross-platform build system (Linux + macOS) that compilestap.odirectly from source to avoid thecpp-dotenvdependency chainMySQL_Authenticationadd/lookup/exists cycle, and init/cleanup idempotency — runs in <0.01s with zero infrastructure dependenciesKey Design Decisions
PROXYSQL_EXTERNreuse: Rather than manually defining hundreds of global variables,test_globals.cppuses the same#define PROXYSQL_EXTERNmechanism assrc/proxysql_global.cppto emit all variable definitions through the existing header infrastructure.Stub functions for src/ symbols:
libproxysql.areferences symbols fromproxy_tls.o,SQLite3_Server.o, andmain.othat aren't in the library itself. These are stubbed as no-ops intest_globals.cpp(e.g.,ProxySQL_create_or_load_TLS,SQLite3_Server::get_variable,binary_sha1).Direct
tap.ocompilation: Unit tests compiletap.cppdirectly instead of depending onlibtap.so, avoiding thecpp-dotenv→cmakebuild chain that doesn't work on macOS. This makes unit tests self-contained.No production code changes: The only non-test file modified is
test/tap/tap/tap.cpp(a 4-lineulongtypedef for macOS compatibility).Commits
803eb27test_globals.h/.cpp+test_init.h/.cpp43c04fbMakefile+smoke_test-t.cpp77ae235unit_teststarget)a538532ulongtypedef intap.cppTest plan
smoke_test-tpasses all 15 tests on macOS (arm64)make build_tap_testsincludes unit testsNOJEMALLOC=1 WITHASAN=1) to verify no memory leaks in init/cleanup cycleSummary by CodeRabbit
New Features
Tests
Chores