chore: migrate custom testing framework to GoogleTest#2
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe pull request migrates the entire test infrastructure from a custom test harness to GoogleTest. Changes include updating CMakeLists.txt to fetch googletest and define a macro for creating test targets, refactoring 8 test files to use GoogleTest's TEST() macros and assertions, and removing custom test utilities while introducing GoogleTest-based alternatives. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_utils.hpp (1)
15-26:⚠️ Potential issue | 🟡 MinorRemove unused
to_string_safehelper function.The
detail::to_string_safetemplate is dead code—it is not used anywhere in the codebase and appears to be a remnant from the old custom test framework. Since GoogleTest provides its own value-to-string conversion for assertion messages, this helper can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_utils.hpp` around lines 15 - 26, Remove the dead helper template function to_string_safe: delete its full template definition (the to_string_safe function and any forward declaration) from tests/test_utils.hpp and remove any now-unused includes or using directives that were only required by that helper (e.g., <string>, <type_traits>) if they become unused after removal; ensure no other code references to_string_safe remain before committing.tests/server_tests.cpp (1)
187-200:⚠️ Potential issue | 🟠 MajorFail fast when client setup/connect fails in protocol tests.
At Line 187+ / 218+ / 257+, these tests can exit without any assertion if socket creation or
connectfails, which can produce false positives.💡 Suggested fix (apply this pattern in all three tests)
- const int sock = socket(AF_INET, SOCK_STREAM, 0); - if (sock >= 0) { - set_sock_timeout(sock); - if (connect(sock, reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)) == 0) { // NOLINT + const int sock = socket(AF_INET, SOCK_STREAM, 0); + ASSERT_GE(sock, 0) << "socket() failed"; + set_sock_timeout(sock); + ASSERT_EQ(connect(sock, reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)), 0) + << "connect() failed"; + { ... - } - static_cast<void>(close(sock)); } + static_cast<void>(close(sock));Also applies to: 218-239, 257-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/server_tests.cpp` around lines 187 - 200, The test can silently pass when socket creation or connect fails; replace the silent conditionals with explicit test assertions: assert the socket descriptor is valid (e.g., ASSERT_GE(sock, 0)) immediately after calling socket(AF_INET, SOCK_STREAM, 0), and assert that connect(...) returns 0 (e.g., ASSERT_EQ(connect(...), 0)) before proceeding to send/recv; keep the close(sock) in a cleanup path (or use a scoped RAII closer) so the socket is always closed after the assertions in the test functions that use socket, connect, STARTUP_PKT_LEN, and EXPECT_LE.
🧹 Nitpick comments (2)
tests/transaction_manager_tests.cpp (1)
42-56: Consider expanding the Isolation test.The current test only verifies that transaction IDs are sequential. For a test named "Isolation", consider adding assertions that validate actual isolation properties (e.g., that uncommitted changes from one transaction are not visible to another).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transaction_manager_tests.cpp` around lines 42 - 56, The Isolation test only checks ID ordering; expand it to assert actual isolation: in TransactionManagerTests::Isolation use TransactionManager (tm.begin/tm.commit) with Catalog/storage to create or pin a test record/key, have txn1 perform a write/update (uncommitted) via the same storage API used elsewhere, then in txn2 attempt to read that record and assert it does not observe txn1's uncommitted change, then commit txn1 and either read again from a new transaction (tm.begin) or from txn2 after proper refresh and assert the committed value is visible; use Transaction, TransactionManager::begin/commit, LockManager, Catalog and BufferPoolManager instances already created to coordinate and add EXPECT_* assertions for both pre-commit and post-commit visibility.tests/lock_manager_tests.cpp (1)
94-120: Deadlock test acknowledges current limitations.The comment on lines 109-111 appropriately documents that the implementation may not detect deadlocks. Consider creating a tracking issue if deadlock detection is a planned feature.
Would you like me to open an issue to track implementing deadlock detection in the LockManager?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/lock_manager_tests.cpp` around lines 94 - 120, The Deadlock test in tests/lock_manager_tests.cpp documents that LockManager currently does not detect deadlocks; if deadlock detection is a planned feature, open a tracking issue describing the desired behavior for LockManager (e.g., cycle detection in wait-for graph, victim selection/abort policy, or timeout semantics) and reference the Deadlock test plus the relevant APIs (LockManager, acquire_exclusive, unlock, Transaction) so implementers can link the test to the feature work. Ensure the issue states acceptance criteria (how the Deadlock test should change once detection is implemented) and any performance/compatibility constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 125-131: Replace the mutable main branch URL in the
FetchContent_Declare for googletest with the stable release tag (use URL
pointing to v1.17.0, e.g.
https://github.com/google/googletest/archive/refs/tags/v1.17.0.zip) and add
URL_HASH SHA256 with the official checksum to enable integrity verification;
update the FetchContent_Declare block (the lines around FetchContent_Declare,
URL and FetchContent_MakeAvailable and keep set(gtest_force_shared_crt ON CACHE
BOOL "" FORCE) unchanged) so the project fetches the pinned v1.17.0 archive with
the SHA256 check.
In `@tests/server_tests.cpp`:
- Around line 124-125: The tests currently drop recv() return values for
multi-byte frames (AUTH_OK_LEN, READY_LEN and the other two spots) and also
assign then discard n_d for a single-byte read; replace these fragile reads with
a small helper (e.g., recv_all or read_full) that loops calling recv() until the
requested length is accumulated or an error/EOF occurs, returning total bytes
read or -1 on error; use this helper everywhere the code now calls recv(sock,
buffer.data(), AUTH_OK_LEN, 0) / recv(..., READY_LEN, 0) and where n_d is
assigned so you actually check the return, and ensure failures cause the test to
fail (or abort) instead of silently proceeding.
---
Outside diff comments:
In `@tests/server_tests.cpp`:
- Around line 187-200: The test can silently pass when socket creation or
connect fails; replace the silent conditionals with explicit test assertions:
assert the socket descriptor is valid (e.g., ASSERT_GE(sock, 0)) immediately
after calling socket(AF_INET, SOCK_STREAM, 0), and assert that connect(...)
returns 0 (e.g., ASSERT_EQ(connect(...), 0)) before proceeding to send/recv;
keep the close(sock) in a cleanup path (or use a scoped RAII closer) so the
socket is always closed after the assertions in the test functions that use
socket, connect, STARTUP_PKT_LEN, and EXPECT_LE.
In `@tests/test_utils.hpp`:
- Around line 15-26: Remove the dead helper template function to_string_safe:
delete its full template definition (the to_string_safe function and any forward
declaration) from tests/test_utils.hpp and remove any now-unused includes or
using directives that were only required by that helper (e.g., <string>,
<type_traits>) if they become unused after removal; ensure no other code
references to_string_safe remain before committing.
---
Nitpick comments:
In `@tests/lock_manager_tests.cpp`:
- Around line 94-120: The Deadlock test in tests/lock_manager_tests.cpp
documents that LockManager currently does not detect deadlocks; if deadlock
detection is a planned feature, open a tracking issue describing the desired
behavior for LockManager (e.g., cycle detection in wait-for graph, victim
selection/abort policy, or timeout semantics) and reference the Deadlock test
plus the relevant APIs (LockManager, acquire_exclusive, unlock, Transaction) so
implementers can link the test to the feature work. Ensure the issue states
acceptance criteria (how the Deadlock test should change once detection is
implemented) and any performance/compatibility constraints.
In `@tests/transaction_manager_tests.cpp`:
- Around line 42-56: The Isolation test only checks ID ordering; expand it to
assert actual isolation: in TransactionManagerTests::Isolation use
TransactionManager (tm.begin/tm.commit) with Catalog/storage to create or pin a
test record/key, have txn1 perform a write/update (uncommitted) via the same
storage API used elsewhere, then in txn2 attempt to read that record and assert
it does not observe txn1's uncommitted change, then commit txn1 and either read
again from a new transaction (tm.begin) or from txn2 after proper refresh and
assert the committed value is visible; use Transaction,
TransactionManager::begin/commit, LockManager, Catalog and BufferPoolManager
instances already created to coordinate and add EXPECT_* assertions for both
pre-commit and post-commit visibility.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txttests/buffer_pool_tests.cpptests/cloudSQL_tests.cpptests/lock_manager_tests.cpptests/recovery_manager_tests.cpptests/recovery_tests.cpptests/server_tests.cpptests/statement_tests.cpptests/test_utils.hpptests/transaction_manager_tests.cpp
| FetchContent_Declare( | ||
| googletest | ||
| URL https://github.com/google/googletest/archive/refs/heads/main.zip | ||
| ) | ||
| # For Windows: prevent overriding the parent project's compiler/linker settings | ||
| set(gtest_force_shared_crt ON CACHE BOOL "" FORCE) | ||
| FetchContent_MakeAvailable(googletest) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest stable google/googletest release tag 2025 2026
💡 Result:
The latest stable google/googletest release tag (covering 2025–2026, as of Feb 27, 2026) is:
v1.17.0— released April 30, 2025 (commit52eb8108c5bdec04579160ae17225d66034bd723). [1][2]
No newer stable release tag than v1.17.0 is shown on the upstream releases page. [1]
🌐 Web query:
GoogleTest v1.14.0 SHA256 checksum source archive zip GitHub
💡 Result:
SHA256 for the GitHub source archive zip v1.14.0.zip:
- URL:
https://github.com/google/googletest/archive/refs/tags/v1.14.0.zip - SHA256:
1f357c27ca988c3f7c6b4bf68a9395005ac6761f034046e9dde0896e3aba00e4[1]
🌐 Web query:
GoogleTest v1.17.0 SHA256 checksum github source archive zip
💡 Result:
SHA256 (GitHub source archive ZIP) for GoogleTest v1.17.0:
- Archive:
https://github.com/google/googletest/archive/v1.17.0.zip(redirects to the tag ZIP) - SHA256:
40d4ec942217dcc84a9ebe2a68584ada7d4a33a8ee958755763278ea1c5e18ff[1]
Pin GoogleTest to a stable release with checksum verification.
Using refs/heads/main.zip at line 127 is mutable and non-reproducible; builds may change unexpectedly and integrity is not verified. Use the latest stable release v1.17.0 with SHA256 checksum validation:
Suggested fix
FetchContent_Declare(
googletest
- URL https://github.com/google/googletest/archive/refs/heads/main.zip
+ URL https://github.com/google/googletest/archive/refs/tags/v1.17.0.zip
+ URL_HASH SHA256=40d4ec942217dcc84a9ebe2a68584ada7d4a33a8ee958755763278ea1c5e18ff
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 125 - 131, Replace the mutable main branch URL
in the FetchContent_Declare for googletest with the stable release tag (use URL
pointing to v1.17.0, e.g.
https://github.com/google/googletest/archive/refs/tags/v1.17.0.zip) and add
URL_HASH SHA256 with the official checksum to enable integrity verification;
update the FetchContent_Declare block (the lines around FetchContent_Declare,
URL and FetchContent_MakeAvailable and keep set(gtest_force_shared_crt ON CACHE
BOOL "" FORCE) unchanged) so the project fetches the pinned v1.17.0 archive with
the SHA256 check.
| static_cast<void>(recv(sock, buffer.data(), AUTH_OK_LEN, 0)); | ||
| static_cast<void>(recv(sock, buffer.data(), READY_LEN, 0)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l tests/server_tests.cppRepository: poyrazK/cloudSQL
Length of output: 86
🏁 Script executed:
# Extract the specific lines mentioned in the review
echo "=== Lines 124-125 ==="
sed -n '120,130p' tests/server_tests.cpp
echo -e "\n=== Lines 145-146 ==="
sed -n '140,150p' tests/server_tests.cpp
echo -e "\n=== Lines 227-228 ==="
sed -n '220,235p' tests/server_tests.cppRepository: poyrazK/cloudSQL
Length of output: 1967
🏁 Script executed:
# Search for all recv/send calls with discarded results in this file
echo "=== static_cast<void>(recv/send) calls ==="
rg -n 'static_cast<void>\((recv|send)\(' tests/server_tests.cpp
echo -e "\n=== ssize_t recv calls ==="
rg -n 'const ssize_t.*recv\(' tests/server_tests.cpp
echo -e "\n=== Other recv/send usage patterns ==="
rg -n -B2 -A2 'recv\(|send\(' tests/server_tests.cpp | head -100Repository: poyrazK/cloudSQL
Length of output: 6857
Add validation for all recv() calls to handle partial reads in protocol parsing.
Lines 124, 125, 227, and 228 discard recv() return values without validating that the full frame was read in a single call. TCP can return partial reads; a multi-byte message may require multiple recv() calls. This causes these protocol tests to silently mis-parse responses under congested network conditions.
Additionally, line 144 assigns the result to a variable then discards it without checking (line 145: (void)n_d;), creating inconsistency with other single-byte reads that do validate (e.g., lines 134, 153).
Implement a loop-based helper that accumulates bytes until the full message length is received, and use it for all frame reads:
Suggested implementation
+bool recv_exact(int sock, void* buf, size_t len) {
+ auto* p = static_cast<char*>(buf);
+ size_t total = 0;
+ while (total < len) {
+ const ssize_t n = recv(sock, p + total, len - total, 0);
+ if (n <= 0) return false;
+ total += static_cast<size_t>(n);
+ }
+ return true;
+}
...
- static_cast<void>(recv(sock, buffer.data(), AUTH_OK_LEN, 0));
- static_cast<void>(recv(sock, buffer.data(), READY_LEN, 0));
+ ASSERT_TRUE(recv_exact(sock, buffer.data(), AUTH_OK_LEN));
+ ASSERT_TRUE(recv_exact(sock, buffer.data(), READY_LEN));📝 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.
| static_cast<void>(recv(sock, buffer.data(), AUTH_OK_LEN, 0)); | |
| static_cast<void>(recv(sock, buffer.data(), READY_LEN, 0)); | |
| bool recv_exact(int sock, void* buf, size_t len) { | |
| auto* p = static_cast<char*>(buf); | |
| size_t total = 0; | |
| while (total < len) { | |
| const ssize_t n = recv(sock, p + total, len - total, 0); | |
| if (n <= 0) return false; | |
| total += static_cast<size_t>(n); | |
| } | |
| return true; | |
| } | |
| ASSERT_TRUE(recv_exact(sock, buffer.data(), AUTH_OK_LEN)); | |
| ASSERT_TRUE(recv_exact(sock, buffer.data(), READY_LEN)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/server_tests.cpp` around lines 124 - 125, The tests currently drop
recv() return values for multi-byte frames (AUTH_OK_LEN, READY_LEN and the other
two spots) and also assign then discard n_d for a single-byte read; replace
these fragile reads with a small helper (e.g., recv_all or read_full) that loops
calling recv() until the requested length is accumulated or an error/EOF occurs,
returning total bytes read or -1 on error; use this helper everywhere the code
now calls recv(sock, buffer.data(), AUTH_OK_LEN, 0) / recv(..., READY_LEN, 0)
and where n_d is assigned so you actually check the return, and ensure failures
cause the test to fail (or abort) instead of silently proceeding.
1 participant
This PR migrates the existing custom testing framework to GoogleTest (GTest). Key changes: - Updated CMakeLists.txt to fetch and configure GTest using FetchContent. - Converted all 8 test suites to standard GTest syntax and macros. - Refactored test_utils.hpp to remove redundant macros. - Addressed various linting and compilation issues across tests.
Summary by CodeRabbit
Tests
Chores