HostGroups Manager unit tests (Phase 2.6)#5506
Conversation
Unit tests for MySQL_HostGroups_Manager and PgSQL_HostGroups_Manager covering 17 test cases. Runs in <0.01s with no infrastructure deps. Test coverage: - Server creation via create_new_server_in_hg() (single, multiple in same HG, different HGs) - Server removal via remove_server_in_hg() (existing, non-existent) - shun_and_killall() (existing server returns true, non-existent false) - set_server_current_latency_us() (latency tracking without crash) - Multiple hostgroups independence (shunning in one HG doesn't affect another) - Duplicate server handling - PgSQL: create, remove, and shun operations with PgSQL-specific types (PgSQL_srv_info_t, PgSQL_srv_opts_t) Infrastructure: - New test_init_hostgroups() / test_cleanup_hostgroups() helpers in test_init.h/.cpp - Creates real MySQL_HostGroups_Manager and PgSQL_HostGroups_Manager with internal SQLite3 databases - Requires Prometheus registry (auto-created if nullptr) Note: Methods like shun_and_killall() and set_server_current_latency_us() acquire their own write locks internally — callers must NOT hold the lock beforehand or a deadlock occurs.
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 significantly enhances the unit testing framework by introducing comprehensive tests for the MySQL_HostGroups_Manager and PgSQL_HostGroups_Manager. It ensures the robust and independent operation of server management functionalities, including server lifecycle, status changes, and performance metrics, all within an isolated testing environment that uses real manager implementations but without external network dependencies. 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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent 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). (3)
📝 WalkthroughWalkthroughAdds idempotent test init/cleanup helpers that allocate MyHGM/PgHGM and ensure GloVars.prometheus_registry, a new hostgroups unit test binary exercising MySQL/PgSQL HostGroups operations, and a Makefile target to build the test. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test runner
participant Init as test_init_hostgroups()
participant Glo as GloVars.prometheus_registry
participant MyHGM as MyHGM (MySQL_HostGroups_Manager)
participant PgHGM as PgHGM (PgSQL_HostGroups_Manager)
Test->>Init: call test_init_hostgroups()
Init->>Glo: ensure registry exists (create if null)
Init->>MyHGM: allocate MySQL_HostGroups_Manager -> assign to MyHGM
Init->>PgHGM: allocate PgSQL_HostGroups_Manager -> assign to PgHGM
Note right of Init: does NOT call managers' init() to avoid background threads
Test->>Init: run tests using MyHGM/PgHGM
Test->>Init: call test_cleanup_hostgroups()
Init->>MyHGM: delete and null MyHGM
Init->>PgHGM: delete and null PgHGM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 a comprehensive set of unit tests for the MySQL_HostGroups_Manager and PgSQL_HostGroups_Manager, which is a great step towards improving test coverage. The tests are well-structured and cover a good range of functionalities including server creation, removal, shunning, and latency tracking.
I've identified a few minor issues:
- A duplicate function declaration in a header file.
- Unsafe C-style casts in the new test file, which could be improved for safety.
- Some leftover developer comments that should be cleaned up.
Overall, this is a solid contribution. Addressing these points will further improve the code quality.
test/tap/test_helpers/test_init.h
Outdated
| void test_cleanup_query_processor(); | ||
|
|
There was a problem hiding this comment.
The function test_cleanup_query_processor() is already declared on line 110. This duplicate declaration should be removed to avoid confusion and maintain code cleanliness.
| bool shunned = MyHGM->shun_and_killall( | ||
| (char *)"192.168.1.1", 3306); |
There was a problem hiding this comment.
Casting a string literal to char* is unsafe because it casts away const. If shun_and_killall were to modify the string, it would result in undefined behavior. To ensure safety, please use a mutable character array. This applies to other similar calls in this file (e.g., lines 120-121, 137-138, 156, 161, and 234-235).
| bool shunned = MyHGM->shun_and_killall( | |
| (char *)"192.168.1.1", 3306); | |
| char addr[] = "192.168.1.1"; | |
| bool shunned = MyHGM->shun_and_killall(addr, 3306); |
| // Total: 1+1+3+2+2+1+2+1+3+1 = 17... let me recount | ||
| // init: 2, create: 3, remove: 2, status: 2, latency: 1, | ||
| // independence: 2, duplicate: 1, pgsql_create: 3, pgsql_shun: 1 | ||
| // = 17. Fix plan. |
There was a problem hiding this comment.
These comments appear to be developer notes for tracking the test plan. They should be removed to keep the code clean.
There was a problem hiding this comment.
Pull request overview
Adds Phase 2.6 unit coverage for HostGroups Managers, extending the TAP unit-test suite to validate basic MySQL/PgSQL hostgroup server lifecycle operations without external infrastructure.
Changes:
- Adds a new TAP unit test binary (
hostgroups_unit-t) with 17 assertions covering create/remove/shun/latency and basic PgSQL parity. - Introduces
test_init_hostgroups()/test_cleanup_hostgroups()to construct and tear down realMySQL_HostGroups_Manager/PgSQL_HostGroups_Managerinstances. - Registers the new unit test target in the unit-test
Makefile.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
test/tap/tests/unit/hostgroups_unit-t.cpp |
New HostGroups Manager unit tests (MySQL + PgSQL). |
test/tap/tests/unit/Makefile |
Adds hostgroups_unit-t to UNIT_TESTS and build rules. |
test/tap/test_helpers/test_init.h |
Declares new hostgroups init/cleanup APIs. |
test/tap/test_helpers/test_init.cpp |
Implements hostgroups init/cleanup (including Prometheus registry setup). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool shunned = MyHGM->shun_and_killall( | ||
| (char *)"192.168.1.1", 3306); | ||
| ok(shunned == true, | ||
| "MySQL HGM: shun_and_killall() returns true for existing server"); | ||
|
|
||
| // shun_and_killall on non-existent server | ||
| bool not_found = MyHGM->shun_and_killall( | ||
| (char *)"10.10.10.10", 9999); |
There was a problem hiding this comment.
These calls cast string literals to char* to satisfy the shun_and_killall(char*, int) signature. If the implementation ever writes to hostname, this becomes undefined behavior (string literals are not writable). Prefer passing a writable buffer (e.g., a local char[] or std::vector<char>), or add a small helper that builds a mutable copy before calling the API.
| bool shunned = MyHGM->shun_and_killall( | |
| (char *)"192.168.1.1", 3306); | |
| ok(shunned == true, | |
| "MySQL HGM: shun_and_killall() returns true for existing server"); | |
| // shun_and_killall on non-existent server | |
| bool not_found = MyHGM->shun_and_killall( | |
| (char *)"10.10.10.10", 9999); | |
| char hostname1[] = "192.168.1.1"; | |
| bool shunned = MyHGM->shun_and_killall( | |
| hostname1, 3306); | |
| ok(shunned == true, | |
| "MySQL HGM: shun_and_killall() returns true for existing server"); | |
| // shun_and_killall on non-existent server | |
| char hostname2[] = "10.10.10.10"; | |
| bool not_found = MyHGM->shun_and_killall( | |
| hostname2, 9999); |
| MyHGM->set_server_current_latency_us( | ||
| (char *)"172.16.0.1", 3306, 5000); // 5ms latency |
There was a problem hiding this comment.
set_server_current_latency_us(char*, ...) is being called by casting a string literal to char*. This is unsafe if the callee ever modifies the buffer. Use a writable hostname buffer (or a helper that makes a mutable copy) rather than casting away constness of a literal.
| MyHGM->set_server_current_latency_us( | |
| (char *)"172.16.0.1", 3306, 5000); // 5ms latency | |
| char hostname[] = "172.16.0.1"; | |
| MyHGM->set_server_current_latency_us( | |
| hostname, 3306, 5000); // 5ms latency |
| static void test_mysql_hostgroup_independence() { | ||
| add_mysql_server(60, "hg60-server", 3306, 1, 100); | ||
| add_mysql_server(70, "hg70-server", 3306, 1, 100); | ||
|
|
||
| // Shun server in HG 60 — should not affect HG 70 | ||
| bool s1 = MyHGM->shun_and_killall((char *)"hg60-server", 3306); | ||
| ok(s1 == true, | ||
| "MySQL HGM: shunned server in HG 60"); | ||
|
|
||
| // HG 70 server should still be accessible | ||
| bool s2 = MyHGM->shun_and_killall((char *)"hg70-server", 3306); | ||
| ok(s2 == true, | ||
| "MySQL HGM: HG 70 server independently operable"); | ||
| } |
There was a problem hiding this comment.
test_mysql_hostgroup_independence() doesn't currently assert that the HG70 server is unaffected by shunning the HG60 server; it only shuns the second server afterwards. To actually test independence, consider checking the HG70 server status after shunning HG60 (e.g., via find_server_in_hg() + get_status() / status), before performing any operation that changes HG70.
| bool s1 = MyHGM->shun_and_killall((char *)"hg60-server", 3306); | ||
| ok(s1 == true, | ||
| "MySQL HGM: shunned server in HG 60"); | ||
|
|
||
| // HG 70 server should still be accessible | ||
| bool s2 = MyHGM->shun_and_killall((char *)"hg70-server", 3306); | ||
| ok(s2 == true, |
There was a problem hiding this comment.
More occurrences of casting string literals to char* for shun_and_killall(). To avoid repeating unsafe casts, it may be worth introducing a small helper (e.g., shun(MyHGM, "host", port)) that creates a mutable copy of the hostname before calling the API.
| bool shunned = PgHGM->shun_and_killall( | ||
| (char *)"pg-shun-server", 5432); |
There was a problem hiding this comment.
PgHGM->shun_and_killall() is called by casting a string literal to char*. This relies on the function never writing to hostname; to keep the test robust against future changes, pass a writable buffer instead of casting away constness of a literal.
| bool shunned = PgHGM->shun_and_killall( | |
| (char *)"pg-shun-server", 5432); | |
| char hostname[] = "pg-shun-server"; | |
| bool shunned = PgHGM->shun_and_killall(hostname, 5432); |
test/tap/test_helpers/test_init.h
Outdated
| * Destroys MyHGM and PgHGM, setting them back to nullptr. | ||
| */ | ||
| void test_cleanup_hostgroups(); | ||
| void test_cleanup_query_processor(); |
There was a problem hiding this comment.
test_cleanup_query_processor() is declared twice in this header (once above and again after test_cleanup_hostgroups()). While duplicate prototypes compile, this looks accidental and can confuse readers; please remove the extra declaration at the end of the file.
| void test_cleanup_query_processor(); |
test/tap/test_helpers/test_init.cpp
Outdated
| int test_init_hostgroups() { | ||
| if (MyHGM != nullptr || PgHGM != nullptr) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
test_init_hostgroups() returns early if either MyHGM or PgHGM is already non-null. This can leave one manager uninitialized (e.g., MyHGM!=nullptr but PgHGM==nullptr) while still reporting success, which can lead to null dereferences in tests. Consider making the idempotency check require both managers to be initialized, or initialize whichever one is still null.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/tap/tests/unit/hostgroups_unit-t.cpp (1)
264-267: Remove leftover comment block.This appears to be debugging notes from test planning. The plan count is correct at 17, so these comments are no longer needed.
Proposed cleanup
test_pgsql_create_and_remove(); // 3 tests test_pgsql_shun(); // 1 test - // Total: 1+1+3+2+2+1+2+1+3+1 = 17... let me recount - // init: 2, create: 3, remove: 2, status: 2, latency: 1, - // independence: 2, duplicate: 1, pgsql_create: 3, pgsql_shun: 1 - // = 17. Fix plan. test_cleanup_hostgroups();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/hostgroups_unit-t.cpp` around lines 264 - 267, Remove the leftover test-planning comment block that starts with "// Total: 1+1+3+2+2+1+2+1+3+1 = 17... let me recount" and the subsequent lines listing init/create/remove/... and "= 17. Fix plan."; simply delete that entire comment block from the test file (hostgroups_unit-t.cpp) so only actual test code and relevant comments remain.
🤖 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/test_helpers/test_init.cpp`:
- Around line 194-202: test_cleanup_hostgroups currently deletes MyHGM while
background threads may still access it; call
MySQL_HostGroups_Manager::shutdown() on the MyHGM instance (or
MyHGM->shutdown()) before deleting and nulling it to signal and join the
HGCU_thread and GTID_syncer_thread, then proceed to delete MyHGM and set MyHGM =
nullptr; leave PgHGM deletion as-is. Ensure you reference MyHGM and
MySQL_HostGroups_Manager::shutdown() in the fix so the threads are stopped
before freeing the object.
In `@test/tap/test_helpers/test_init.h`:
- Line 130: Remove the duplicate declaration of the function
test_cleanup_query_processor() from the header so only a single prototype
remains; locate the second copy of the declaration (the one that duplicates the
existing prototype) and delete it, leaving the original
test_cleanup_query_processor() declaration untouched to avoid duplicate-symbol
warnings.
---
Nitpick comments:
In `@test/tap/tests/unit/hostgroups_unit-t.cpp`:
- Around line 264-267: Remove the leftover test-planning comment block that
starts with "// Total: 1+1+3+2+2+1+2+1+3+1 = 17... let me recount" and the
subsequent lines listing init/create/remove/... and "= 17. Fix plan."; simply
delete that entire comment block from the test file (hostgroups_unit-t.cpp) so
only actual test code and relevant comments remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 997d53ed-cb37-43ed-bfed-38dca92e1e9a
📒 Files selected for processing (4)
test/tap/test_helpers/test_init.cpptest/tap/test_helpers/test_init.htest/tap/tests/unit/Makefiletest/tap/tests/unit/hostgroups_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: Agent
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: run / trigger
🧰 Additional context used
🧠 Learnings (2)
📚 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.h
📚 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/hostgroups_unit-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/hostgroups_unit-t.cpp
[error] 18-18: 'tap.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/hostgroups_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)
🔇 Additional comments (6)
test/tap/test_helpers/test_init.h (1)
112-129: LGTM!The new
test_init_hostgroups()andtest_cleanup_hostgroups()declarations follow the established pattern of other init/cleanup pairs in this header.test/tap/test_helpers/test_init.cpp (1)
175-192: LGTM on the initialization logic.The idempotency check and Prometheus registry setup follow the established patterns from other
test_init_*functions.test/tap/tests/unit/Makefile (2)
234-234: LGTM!The new
hostgroups_unit-ttarget is correctly added to theUNIT_TESTSlist.
283-286: LGTM!The build target follows the established pattern from other unit test targets.
test/tap/tests/unit/hostgroups_unit-t.cpp (2)
1-28: Well-structured test file.The test organization is clear with logical groupings, good use of helper functions, and proper documentation of locking semantics (especially the notes about
shun_and_killall()andset_server_current_latency_us()acquiring their own locks). Using distinct hostgroup IDs for each test ensures isolation.
244-273: LGTM on test main function.The initialization order (
test_init_minimal()→test_init_hostgroups()) and cleanup order (reverse) follows the documented pattern. Test plan count of 17 matches the actual test assertions.
| void test_cleanup_hostgroups() { | ||
| if (MyHGM != nullptr) { | ||
| delete MyHGM; | ||
| MyHGM = nullptr; | ||
| } | ||
| if (PgHGM != nullptr) { | ||
| delete PgHGM; | ||
| PgHGM = nullptr; | ||
| } |
There was a problem hiding this comment.
Critical: Call shutdown() before deleting MyHGM to prevent use-after-free.
MySQL_HostGroups_Manager::init() spawns two background threads (HGCU_thread and GTID_syncer_thread) that continuously dereference the global MyHGM pointer. Deleting MyHGM without calling shutdown() first leaves these threads running and dereferencing freed memory.
From lib/MySQL_HostGroups_Manager.cpp:770-789, shutdown() signals the threads to exit and joins them before cleanup.
Proposed fix
void test_cleanup_hostgroups() {
if (MyHGM != nullptr) {
+ MyHGM->shutdown();
delete MyHGM;
MyHGM = nullptr;
}
if (PgHGM != nullptr) {
+ PgHGM->shutdown();
delete PgHGM;
PgHGM = nullptr;
}
}📝 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.
| void test_cleanup_hostgroups() { | |
| if (MyHGM != nullptr) { | |
| delete MyHGM; | |
| MyHGM = nullptr; | |
| } | |
| if (PgHGM != nullptr) { | |
| delete PgHGM; | |
| PgHGM = nullptr; | |
| } | |
| void test_cleanup_hostgroups() { | |
| if (MyHGM != nullptr) { | |
| MyHGM->shutdown(); | |
| delete MyHGM; | |
| MyHGM = nullptr; | |
| } | |
| if (PgHGM != nullptr) { | |
| PgHGM->shutdown(); | |
| delete PgHGM; | |
| PgHGM = nullptr; | |
| } | |
| } |
🤖 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 194 - 202,
test_cleanup_hostgroups currently deletes MyHGM while background threads may
still access it; call MySQL_HostGroups_Manager::shutdown() on the MyHGM instance
(or MyHGM->shutdown()) before deleting and nulling it to signal and join the
HGCU_thread and GTID_syncer_thread, then proceed to delete MyHGM and set MyHGM =
nullptr; leave PgHGM deletion as-is. Ensure you reference MyHGM and
MySQL_HostGroups_Manager::shutdown() in the fix so the threads are stopped
before freeing the object.
MySQL_HostGroups_Manager::init() starts two background threads (HGCU_thread and GTID_syncer_thread) that run forever in a blocking loop. The destructor does not call shutdown() to stop them, so the test process hangs indefinitely after main() returns — std::thread destructor calls std::terminate() on joinable threads. Fix: skip init() entirely. The constructor alone sets up the internal SQLite3 database and all data structures needed for unit testing. The background threads are only needed for real connection pool management (connection reuse, GTID sync), not for server add/remove/ shun operations.
- test_init.h: remove duplicate test_cleanup_query_processor() declaration (copy-paste artifact at line 130) - test_init.cpp: check MyHGM and PgHGM independently instead of using OR guard, so one can't be left null while the other exists
|
Summary
Implements Phase 2.6 (#5478) of the Unit Testing Framework: unit tests for
MySQL_HostGroups_ManagerandPgSQL_HostGroups_Manager.Test Coverage
Infrastructure Changes
test_init_hostgroups()/test_cleanup_hostgroups()in test_init.h/.cppMySQL_HostGroups_ManagerandPgSQL_HostGroups_Managerwith Prometheus registryNotable Finding
Methods like
shun_and_killall()andset_server_current_latency_us()acquire their own write locks internally. Tests must NOT wrap these calls with externalwrlock()/wrunlock()or a deadlock occurs. This is documented in the commit message and test comments.Depends On
Test plan
Summary by CodeRabbit