Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds three new unit-test suites and registers them in the build: catalog_coverage_tests, transaction_coverage_tests, and utils_coverage_tests; tests exercise Catalog operations, transaction/lock-manager concurrency and rollback, and Value/RpcClient edge cases. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (2)
tests/utils_coverage_tests.cpp (1)
86-96: Test covers connection failure paths well, but consider potential flakiness.The test uses port 1 which is typically privileged and closed, but network behavior can vary across environments. If CI runs in containers with different network configurations, this test might behave unexpectedly. Consider adding a comment documenting this assumption, or using a mock/stub approach for more deterministic testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils_coverage_tests.cpp` around lines 86 - 96, The test relies on reaching an unreachable service on localhost (RpcClient constructor, connect/is_connected/call/send_only) which can be flaky; replace the hardcoded localhost+port assumption by using a non-routable TEST-NET address (e.g., 192.0.2.1) or, preferably, refactor to a deterministic stub/mock of RpcClient and call the same methods (connect, is_connected, call, send_only) to simulate failure; also add a short comment explaining the chosen approach and why it avoids CI network variability so the intent is clear.tests/transaction_coverage_tests.cpp (1)
78-135: Good rollback coverage, but cleanup could be more robust.The test correctly exercises the undo log paths for INSERT, UPDATE (with old_rid), and DELETE operations, verifying that
tm.abort()properly reverses all changes.Consider using a test fixture with
SetUp/TearDownto ensure cleanup runs even if assertions fail:♻️ Suggested improvement for robust cleanup
+class TransactionCoverageTestFixture : public ::testing::Test { +protected: + void TearDown() override { + std::remove("./test_data/rollback_stress.heap"); + // Consider also cleaning up the directory if needed + } +}; -TEST(TransactionCoverageTests, DeepRollback) { +TEST_F(TransactionCoverageTestFixture, DeepRollback) { // ... test body ... - static_cast<void>(std::remove("./test_data/rollback_stress.heap")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transaction_coverage_tests.cpp` around lines 78 - 135, Replace the standalone TEST( TransactionCoverageTests, DeepRollback ) with a test fixture so cleanup always runs: create a class TransactionCoverageTests : public ::testing::Test and implement SetUp to initialize Catalog::create(), StorageManager disk_manager, BufferPoolManager bpm, LockManager lm, TransactionManager tm, and create the HeapTable used by the test; implement TearDown to abort any live Transaction* txn (or call tm.abort(txn) if not nullptr), close/remove the backing file "./test_data/rollback_stress.heap" (and any other test_data artifacts), and release/destroy bpm and disk_manager resources; change the test to TEST_F(TransactionCoverageTests, DeepRollback) and remove the inline static_cast<void>(std::remove(...)) cleanup line so the TearDown handles it. Ensure the fixture exposes the symbols used in the test (tm, txn, table, disk_manager, bpm) so the existing test body can be reused unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/transaction_coverage_tests.cpp`:
- Around line 78-135: Replace the standalone TEST( TransactionCoverageTests,
DeepRollback ) with a test fixture so cleanup always runs: create a class
TransactionCoverageTests : public ::testing::Test and implement SetUp to
initialize Catalog::create(), StorageManager disk_manager, BufferPoolManager
bpm, LockManager lm, TransactionManager tm, and create the HeapTable used by the
test; implement TearDown to abort any live Transaction* txn (or call
tm.abort(txn) if not nullptr), close/remove the backing file
"./test_data/rollback_stress.heap" (and any other test_data artifacts), and
release/destroy bpm and disk_manager resources; change the test to
TEST_F(TransactionCoverageTests, DeepRollback) and remove the inline
static_cast<void>(std::remove(...)) cleanup line so the TearDown handles it.
Ensure the fixture exposes the symbols used in the test (tm, txn, table,
disk_manager, bpm) so the existing test body can be reused unchanged.
In `@tests/utils_coverage_tests.cpp`:
- Around line 86-96: The test relies on reaching an unreachable service on
localhost (RpcClient constructor, connect/is_connected/call/send_only) which can
be flaky; replace the hardcoded localhost+port assumption by using a
non-routable TEST-NET address (e.g., 192.0.2.1) or, preferably, refactor to a
deterministic stub/mock of RpcClient and call the same methods (connect,
is_connected, call, send_only) to simulate failure; also add a short comment
explaining the chosen approach and why it avoids CI network variability so the
intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cee9e2c4-18cb-4b3f-a5ee-d83382d0efe3
📒 Files selected for processing (4)
CMakeLists.txttests/catalog_coverage_tests.cpptests/transaction_coverage_tests.cpptests/utils_coverage_tests.cpp
…rministic network failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/utils_coverage_tests.cpp (1)
42-42: Consider usingEXPECT_EQforstd::stringcomparison.While
EXPECT_STREQwith.c_str()works,EXPECT_EQis more idiomatic forstd::stringcomparisons in gtest and provides clearer error messages.📝 Suggested refactor
- EXPECT_STREQ(v_null.to_string().c_str(), "NULL"); + EXPECT_EQ(v_null.to_string(), "NULL");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils_coverage_tests.cpp` at line 42, The test currently uses EXPECT_STREQ(v_null.to_string().c_str(), "NULL"); — replace this with the idiomatic gtest string comparison using EXPECT_EQ by calling EXPECT_EQ(v_null.to_string(), "NULL") (i.e., reference the v_null.to_string() method and remove the .c_str() + EXPECT_STREQ usage so failures show clearer std::string diffs).tests/transaction_coverage_tests.cpp (2)
37-38: Isolate the on-disk state per test instance.Hard-coding
./test_dataand deleting only one heap file makes this fixture sensitive to stale artifacts. A per-test temp directory with recursive cleanup will keep reruns and parallel execution from bleeding state intoDeepRollback.Also applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transaction_coverage_tests.cpp` around lines 37 - 38, The test fixture currently hardcodes "./test_data" when creating StorageManager (disk_manager) and only deletes a single heap file, which allows stale on-disk state to leak between tests; change the test setup to create a unique per-test temporary directory (e.g., using std::filesystem::temp_directory_path() combined with a unique suffix like pid/timestamp/UUID) and construct StorageManager with that path (where disk_manager is created), and in the teardown remove the entire directory recursively (not just one heap file) to ensure full cleanup; apply the same change for the other occurrence around line 68 so all tests (including DeepRollback) use isolated temp dirs and recursive cleanup.
95-123: Use explicit synchronization instead of sleep/poll/yield.Line 110 only proves that no reader incremented within 200 ms; it does not guarantee all readers were actually blocked on the writer. Replacing the timing windows and spin loop with a condition variable/latch will make this test deterministic and much less CPU-hungry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transaction_coverage_tests.cpp` around lines 95 - 123, The test uses sleeps and busy-wait (std::this_thread::sleep_for / yield) to assert readers are blocked, which is flaky and CPU-heavy; replace that with explicit synchronization: have the reader thread lambda signal a condition_variable or count down a std::latch (or increment an atomic and notify on a condition_variable) after successfully acquiring the shared lock, and have the main thread wait on that condition_variable or latch to assert no readers have progressed while the writer holds the lock and to wait deterministically for all readers after writer_txn is unlocked; update uses of shared_granted, stop, readers, lm.acquire_shared, lm.unlock and EXPECT_EQ to coordinate via the chosen synchronization primitive instead of sleep/poll/yield so the test becomes deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/transaction_coverage_tests.cpp`:
- Around line 133-168: The test currently creates all rows inside the same
transaction that is later aborted, so restore semantics for UPDATE.old_rid and
DELETE undo aren’t exercised; modify the test to seed one or more committed rows
first (use a separate transaction that calls table->insert and commits via the
transaction manager before starting the txn used for modifications), then in the
transaction under test perform table->remove, table->insert and
txn->add_undo_log (including UndoLog::Type::UPDATE with the old_rid and
UndoLog::Type::DELETE), call tm->abort(txn), and finally assert via
table->tuple_count() and table->scan() that the original committed rows are
visible again; alternatively split into two tests where one seeds committed rows
and the other aborts modifications to them.
---
Nitpick comments:
In `@tests/transaction_coverage_tests.cpp`:
- Around line 37-38: The test fixture currently hardcodes "./test_data" when
creating StorageManager (disk_manager) and only deletes a single heap file,
which allows stale on-disk state to leak between tests; change the test setup to
create a unique per-test temporary directory (e.g., using
std::filesystem::temp_directory_path() combined with a unique suffix like
pid/timestamp/UUID) and construct StorageManager with that path (where
disk_manager is created), and in the teardown remove the entire directory
recursively (not just one heap file) to ensure full cleanup; apply the same
change for the other occurrence around line 68 so all tests (including
DeepRollback) use isolated temp dirs and recursive cleanup.
- Around line 95-123: The test uses sleeps and busy-wait
(std::this_thread::sleep_for / yield) to assert readers are blocked, which is
flaky and CPU-heavy; replace that with explicit synchronization: have the reader
thread lambda signal a condition_variable or count down a std::latch (or
increment an atomic and notify on a condition_variable) after successfully
acquiring the shared lock, and have the main thread wait on that
condition_variable or latch to assert no readers have progressed while the
writer holds the lock and to wait deterministically for all readers after
writer_txn is unlocked; update uses of shared_granted, stop, readers,
lm.acquire_shared, lm.unlock and EXPECT_EQ to coordinate via the chosen
synchronization primitive instead of sleep/poll/yield so the test becomes
deterministic.
In `@tests/utils_coverage_tests.cpp`:
- Line 42: The test currently uses EXPECT_STREQ(v_null.to_string().c_str(),
"NULL"); — replace this with the idiomatic gtest string comparison using
EXPECT_EQ by calling EXPECT_EQ(v_null.to_string(), "NULL") (i.e., reference the
v_null.to_string() method and remove the .c_str() + EXPECT_STREQ usage so
failures show clearer std::string diffs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6acaf55a-a4f7-4249-bed3-5781540a4f6a
📒 Files selected for processing (2)
tests/transaction_coverage_tests.cpptests/utils_coverage_tests.cpp
Comment on lines
+133
to
+168
| // 1. Insert some data | ||
| auto rid1 = table->insert( | ||
| executor::Tuple({common::Value::make_int64(1), common::Value::make_text("A")}), | ||
| txn->get_id()); | ||
| txn->add_undo_log(UndoLog::Type::INSERT, "rollback_stress", rid1); | ||
|
|
||
| auto rid2 = table->insert( | ||
| executor::Tuple({common::Value::make_int64(2), common::Value::make_text("B")}), | ||
| txn->get_id()); | ||
| txn->add_undo_log(UndoLog::Type::INSERT, "rollback_stress", rid2); | ||
|
|
||
| // 2. Update data | ||
| table->remove(rid1, txn->get_id()); // Mark old version deleted | ||
| auto rid1_new = table->insert( | ||
| executor::Tuple({common::Value::make_int64(1), common::Value::make_text("A_NEW")}), | ||
| txn->get_id()); | ||
| txn->add_undo_log(UndoLog::Type::UPDATE, "rollback_stress", rid1_new, rid1); | ||
|
|
||
| // 3. Delete data | ||
| table->remove(rid2, txn->get_id()); | ||
| txn->add_undo_log(UndoLog::Type::DELETE, "rollback_stress", rid2); | ||
|
|
||
| EXPECT_EQ(table->tuple_count(), 1U); // rid1_new is active, rid1 and rid2 are logically deleted | ||
|
|
||
| // 4. Abort | ||
| tm->abort(txn); | ||
| txn = nullptr; // Marked as aborted and handled by TearDown if still set | ||
|
|
||
| // 5. Verify restoration | ||
| EXPECT_EQ(table->tuple_count(), | ||
| 0U); // Inserted rows should be physically removed or logically invisible | ||
|
|
||
| // The table should be empty because we aborted the inserts | ||
| auto iter = table->scan(); | ||
| executor::Tuple t; | ||
| EXPECT_FALSE(iter.next(t)); |
There was a problem hiding this comment.
Seed committed rows if you want to verify restore semantics.
Because every tuple in this scenario is created by the transaction that later aborts, the final tuple_count() == 0 assertion only proves “abort returns the table to empty.” It does not make the restoration half of DELETE undo or the old_rid half of UPDATE undo observable. Seed committed rows first, or split this into a second test, then abort a transaction that updates/deletes them and assert the original rows are visible again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/transaction_coverage_tests.cpp` around lines 133 - 168, The test
currently creates all rows inside the same transaction that is later aborted, so
restore semantics for UPDATE.old_rid and DELETE undo aren’t exercised; modify
the test to seed one or more committed rows first (use a separate transaction
that calls table->insert and commits via the transaction manager before starting
the txn used for modifications), then in the transaction under test perform
table->remove, table->insert and txn->add_undo_log (including
UndoLog::Type::UPDATE with the old_rid and UndoLog::Type::DELETE), call
tm->abort(txn), and finally assert via table->tuple_count() and table->scan()
that the original committed rows are visible again; alternatively split into two
tests where one seeds committed rows and the other aborts modifications to them.
…rministic network failure
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements the test coverage improvement plan by targeting the weakest modules: Catalog, Transaction, and Network/Utils. It adds 3 new test suites and integrates them into the CI pipeline.
Summary by CodeRabbit