fix: storage gtest #184
fix: storage gtest #184
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request focuses on cleaning up and streamlining the test files in the storage module. The changes primarily involve removing unnecessary include directives, such as Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/storage/tests/CMakeLists.txt(1 hunks)src/storage/tests/custom_comparator_test.cc(0 hunks)src/storage/tests/flush_oldest_cf_test.cc(0 hunks)src/storage/tests/hashes_filter_test.cc(1 hunks)src/storage/tests/kv_format_test.cc(0 hunks)src/storage/tests/lists_filter_test.cc(0 hunks)src/storage/tests/log_index_test.cc(0 hunks)src/storage/tests/options_test.cc(0 hunks)src/storage/tests/sets_test.cc(0 hunks)src/storage/tests/strings_filter_test.cc(0 hunks)src/storage/tests/strings_test.cc(0 hunks)src/storage/tests/zsets_test.cc(0 hunks)
💤 Files with no reviewable changes (10)
- src/storage/tests/kv_format_test.cc
- src/storage/tests/strings_test.cc
- src/storage/tests/log_index_test.cc
- src/storage/tests/sets_test.cc
- src/storage/tests/custom_comparator_test.cc
- src/storage/tests/lists_filter_test.cc
- src/storage/tests/options_test.cc
- src/storage/tests/strings_filter_test.cc
- src/storage/tests/flush_oldest_cf_test.cc
- src/storage/tests/zsets_test.cc
✅ Files skipped from review due to trivial changes (1)
- src/storage/tests/hashes_filter_test.cc
🔇 Additional comments (1)
src/storage/tests/CMakeLists.txt (1)
39-39: LGTM! Good improvement to test discovery.Adding
GTEST_DISCOVER_TESTSis a valuable enhancement that enables automatic discovery of individual test cases at configure time. This improves test organization and allows for more granular test execution and reporting.
| @@ -36,4 +36,5 @@ FOREACH (TEST_SOURCE ${TEST_SOURCES}) | |||
| PRIVATE ${zstd_LIBRARIES} | |||
| PRIVATE binlog_pb | |||
| ) | |||
| GTEST_DISCOVER_TESTS(${TEST_NAME}) | |||
There was a problem hiding this comment.
💡 Codebase verification
Add missing CMake minimum version requirement
The file uses GTEST_DISCOVER_TESTS which requires CMake 3.10 or newer, but no minimum CMake version is specified. For consistency with other test directories (which use 3.18 and 3.25), add cmake_minimum_required(VERSION 3.18) at the beginning of the file.
🔗 Analysis chain
Verify CMake version compatibility.
The GTEST_DISCOVER_TESTS command requires CMake 3.10 or newer. Let's verify the minimum CMake version requirement is properly set.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if minimum CMake version is specified and compatible
rg -A 1 "cmake_minimum_required"
Length of output: 229
Script:
#!/bin/bash
# Check root and parent CMakeLists.txt files
echo "=== Root CMakeLists.txt ==="
cat CMakeLists.txt 2>/dev/null | grep -A 1 "cmake_minimum" || echo "No root CMakeLists.txt found"
echo -e "\n=== src/storage/CMakeLists.txt ==="
cat src/storage/CMakeLists.txt 2>/dev/null | grep -A 1 "cmake_minimum" || echo "No src/storage/CMakeLists.txt found"
echo -e "\n=== src/storage/tests/CMakeLists.txt ==="
cat src/storage/tests/CMakeLists.txt 2>/dev/null | grep -A 1 "cmake_minimum" || echo "No src/storage/tests/CMakeLists.txt found"
Length of output: 658
Script:
#!/bin/bash
echo "=== Checking file existence ==="
ls -l src/storage/tests/CMakeLists.txt 2>/dev/null || echo "File does not exist"
echo -e "\n=== File content if exists ==="
cat src/storage/tests/CMakeLists.txt 2>/dev/null || echo "Cannot read file content"
Length of output: 1746
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/storage/tests/keys_test.cc (1)
Line range hint
5204-5214: Cleanup commented-out logging configuration.The main function contains commented-out logging configuration code that should be either removed or enabled.
Either remove or enable the logging configuration:
- // FLAGS_log_dir = "./log"; - // FLAGS_minloglevel = 0; - // FLAGS_max_log_size = 1800; - // FLAGS_logbufsecs = 0; - // ::google::InitGoogleLogging("keys_test");
🧹 Nitpick comments (8)
src/storage/tests/log_index_collector_test.cc (1)
Line range hint
137-143: Consider making member variables const and addressing thread safety.Two suggestions for improvement:
- Member variables
name_andstart_could be marked asconstsince they're only set in the constructor.- Be aware that
fmt::printlnis not guaranteed to be thread-safe. Consider using a thread-safe logging mechanism for production code.struct TimerGuard { TimerGuard(std::string_view name = "Test") : name_(name), start_(std::chrono::system_clock::now()) {} ~TimerGuard() { auto end = std::chrono::system_clock::now(); auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end - start_); fmt::println("{} cost {}ms", name_, duration.count()); } - std::string_view name_; - std::chrono::time_point<std::chrono::system_clock> start_; + const std::string_view name_; + const std::chrono::time_point<std::chrono::system_clock> start_; };src/storage/tests/hashes_test.cc (3)
58-59: Consider consolidating duplicate field_value_match functions.The two
field_value_matchfunctions share similar logic. Consider consolidating them into a single function with an optional database parameter to reduce code duplication.-static bool field_value_match(storage::Storage *const db, const Slice &key, - const std::vector<FieldValue> &expect_field_value) { - std::vector<FieldValue> field_value_out; - Status s = db->HGetall(key, &field_value_out); - if (!s.ok() && !s.IsNotFound()) { - return false; - } - if (field_value_out.size() != expect_field_value.size()) { - return false; - } - if (s.IsNotFound() && expect_field_value.empty()) { - return true; - } - for (const auto &field_value : expect_field_value) { - if (find(field_value_out.begin(), field_value_out.end(), field_value) == field_value_out.end()) { - return false; - } - } - return true; -} - -static bool field_value_match(const std::vector<FieldValue> &field_value_out, - const std::vector<FieldValue> &expect_field_value) { +static bool field_value_match(const std::vector<FieldValue> &expect_field_value, + const std::vector<FieldValue> &field_value_out, + storage::Storage *const db = nullptr, + const Slice &key = Slice()) { + std::vector<FieldValue> actual_values; + if (db != nullptr) { + Status s = db->HGetall(key, &actual_values); + if (!s.ok() && !s.IsNotFound()) { + return false; + } + if (s.IsNotFound() && expect_field_value.empty()) { + return true; + } + field_value_out = actual_values; + } if (field_value_out.size() != expect_field_value.size()) { return false; } for (const auto &field_value : expect_field_value) { if (find(field_value_out.begin(), field_value_out.end(), field_value) == field_value_out.end()) { return false; } } return true; }Also applies to: 79-80
1004-1005: Document the reason for NOLINT.The NOLINT comment should include a reason why the lint check is being suppressed. This helps future maintainers understand the rationale.
-TEST_F(HashesTest, HScanTest) { - // NOLINT +TEST_F(HashesTest, HScanTest) { // NOLINT(readability/function_size) - Complex test requiring multiple scenarios
Line range hint
2445-2454: Remove or document commented logging setup code.The commented logging setup code should either be removed if it's no longer needed or documented explaining why it's kept but commented out.
- // FLAGS_log_dir = "./log"; - // FLAGS_minloglevel = 0; - // FLAGS_max_log_size = 1800; - // FLAGS_logbufsecs = 0; - // ::google::InitGoogleLogging("hashes_test"); + // TODO: Logging setup temporarily disabled until we migrate to a new logging systemsrc/storage/tests/keys_test.cc (4)
33-33: Add documentation for the destructor.The destructor is marked as default but lacks documentation explaining why it's needed and what cleanup it performs.
Add a comment explaining the purpose of the destructor:
+ // Virtual destructor to allow proper cleanup of derived test classes ~KeysTest() override = default;
49-50: Document empty test suite lifecycle methods.The empty
SetUpTestSuiteandTearDownTestSuitemethods lack documentation explaining their purpose.Add comments explaining why these methods are empty:
+ // No global test suite setup needed static void SetUpTestSuite() {} + // No global test suite cleanup needed static void TearDownTestSuite() {}
99-100: Improve test execution order comment.The comment about test execution order is important but could be more descriptive.
Enhance the comment to better explain the test dependencies:
- // Note: This test needs to execute at first because all of the data is - // predetermined. + // IMPORTANT: This test must execute first as it relies on a specific + // predetermined dataset. Later tests may modify this data.
115-115: Consider using const references in range-based for loops.The range-based for loops could be optimized by using const references to avoid unnecessary copying.
Update the loops to use const references:
- for (const auto &kv : kvs) { + for (const auto& kv : kvs) {This change should be applied to all similar loops in the file.
Also applies to: 295-295, 490-490, 682-682, 875-875, 1088-1088, 1267-1267, 1478-1478, 1689-1689, 1900-1900
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/storage/tests/custom_comparator_test.cc(1 hunks)src/storage/tests/flush_oldest_cf_test.cc(8 hunks)src/storage/tests/hashes_filter_test.cc(9 hunks)src/storage/tests/hashes_test.cc(7 hunks)src/storage/tests/keys_test.cc(21 hunks)src/storage/tests/kv_format_test.cc(2 hunks)src/storage/tests/lists_filter_test.cc(4 hunks)src/storage/tests/lists_test.cc(21 hunks)src/storage/tests/lock_mgr_test.cc(1 hunks)src/storage/tests/log_index_collector_test.cc(3 hunks)src/storage/tests/log_index_test.cc(8 hunks)src/storage/tests/lru_cache_test.cc(1 hunks)src/storage/tests/options_test.cc(2 hunks)src/storage/tests/sets_test.cc(21 hunks)src/storage/tests/strings_filter_test.cc(1 hunks)src/storage/tests/strings_test.cc(5 hunks)src/storage/tests/zsets_test.cc(27 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/storage/tests/lock_mgr_test.cc
- src/storage/tests/lru_cache_test.cc
- src/storage/tests/lists_test.cc
🚧 Files skipped from review as they are similar to previous changes (11)
- src/storage/tests/hashes_filter_test.cc
- src/storage/tests/kv_format_test.cc
- src/storage/tests/log_index_test.cc
- src/storage/tests/flush_oldest_cf_test.cc
- src/storage/tests/strings_filter_test.cc
- src/storage/tests/options_test.cc
- src/storage/tests/strings_test.cc
- src/storage/tests/custom_comparator_test.cc
- src/storage/tests/zsets_test.cc
- src/storage/tests/lists_filter_test.cc
- src/storage/tests/sets_test.cc
🔇 Additional comments (4)
src/storage/tests/log_index_collector_test.cc (2)
25-27: Well-implemented thread-safe sequence generator!The
Next()method is correctly implemented using atomic fetch-and-add operation, ensuring thread safety while maintaining efficiency.
35-36: Test case formatting improves readability.Moving the NOLINT comments to separate lines enhances code readability while maintaining the same functionality.
Also applies to: 80-81
src/storage/tests/hashes_test.cc (2)
32-32: LGTM! Proper destructor declaration.The destructor is correctly marked as override and defaulted, following best practices for virtual destructors.
49-49: Consider implementing test suite setup/teardown methods.The static
SetUpTestSuiteandTearDownTestSuitemethods are empty. Consider adding suite-wide setup/teardown logic if needed, or document why they are intentionally empty.Let's check if other test files have implemented these methods:
| for (const auto &ts : type_status) { | ||
| WARN("type: {} status: {}", storage::DataTypeStrings[static_cast<int>(ts.first)], ts.second.ToString()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug logging statements.
The warning log statements appear to be debug code that was accidentally committed.
Remove the debug logging:
- WARN("ret: {}", ret);
- for (const auto &ts : type_status) {
- WARN("type: {} status: {}", storage::DataTypeStrings[static_cast<int>(ts.first)], ts.second.ToString());
- }Committable suggestion skipped: line range outside the PR's diff.
修复
storageGTest failSummary by CodeRabbit
Chores
Tests