feat(clp-s): Add end to end search tests for clp-s. #668
Conversation
WalkthroughThis pull request introduces substantial enhancements to the CLP (Compressed Log Processing) project, focusing on expanding search functionality, testing infrastructure, and build configuration. The changes include adding numerous source files for search operations, timestamp handling, and reducer components, updating the CMakeLists.txt build configuration, and introducing new test utilities. The modifications extend the project's capabilities in log processing, search transformations, and testing methodologies without disrupting existing functionalities. Changes
Sequence DiagramsequenceDiagram
participant SR as Search Request
participant TF as Transformation Passes
participant AR as ArchiveReader
participant SI as Search Index
participant OH as OutputHandler
SR->>TF: Apply standardization
TF->>TF: Narrow types
TF->>TF: Convert to existence
TF->>AR: Process archives
AR->>SI: Evaluate timestamp index
SI->>AR: Filter results
AR->>OH: Write matching results
Possibly Related PRs
Suggested Reviewers
Finishing Touches
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: 4
🧹 Nitpick comments (6)
components/core/tests/test_log_files/test_search.jsonl (1)
1-9: Consider adding more comprehensive test cases.The test data covers basic scenarios but could benefit from additional test cases to ensure robust search functionality:
- Empty strings and null values
- Unicode characters and emojis
- Deeply nested objects and arrays
- Very large strings to test size limits
- Special JSON characters like
\u0000components/core/tests/test-clp_s-end_to_end.cpp (1)
Line range hint
39-61: Extract magic numbers into named constants.The size constants should be extracted into named constants at class/namespace scope for better maintainability.
+namespace { +constexpr auto cGiB = 1024ULL * 1024 * 1024; +constexpr auto cMiB = 1024ULL * 1024; +constexpr auto cDefaultTargetEncodedSize = 8ULL * cGiB; +constexpr auto cDefaultMaxDocumentSize = 512ULL * cMiB; +constexpr auto cDefaultMinTableSize = 1ULL * cMiB; +} // namespace void compress(bool structurize_arrays, bool single_file_archive) { - constexpr auto cDefaultTargetEncodedSize = 8ULL * 1024 * 1024 * 1024; // 8 GiB - constexpr auto cDefaultMaxDocumentSize = 512ULL * 1024 * 1024; // 512 MiB - constexpr auto cDefaultMinTableSize = 1ULL * 1024 * 1024; // 1 MiBcomponents/core/tests/test-clp_s-search.cpp (4)
49-58: Add error handling for path operations.While the path utility functions are well-implemented, they should handle cases where the test input file doesn't exist.
auto get_test_input_local_path() -> std::string { std::filesystem::path const current_file_path{__FILE__}; auto const tests_dir{current_file_path.parent_path()}; - return (tests_dir / get_test_input_path_relative_to_tests_dir()).string(); + auto const input_path = tests_dir / get_test_input_path_relative_to_tests_dir(); + if (!std::filesystem::exists(input_path)) { + throw std::runtime_error(fmt::format("Test input file not found: {}", input_path.string())); + } + return input_path.string(); }
60-63: Extract magic numbers as named constants.The configuration values should be defined as named constants at the top of the file for better maintainability.
+constexpr auto cDefaultTargetEncodedSizeGiB = 8ULL; +constexpr auto cDefaultMaxDocumentSizeMiB = 512ULL; +constexpr auto cDefaultMinTableSizeMiB = 1ULL; +constexpr auto cDefaultCompressionLevel = 3; + - constexpr auto cDefaultTargetEncodedSize = 8ULL * 1024 * 1024 * 1024; // 8 GiB - constexpr auto cDefaultMaxDocumentSize = 512ULL * 1024 * 1024; // 512 MiB - constexpr auto cDefaultMinTableSize = 1ULL * 1024 * 1024; // 1 MiB - constexpr auto cDefaultCompressionLevel = 3; + constexpr auto cDefaultTargetEncodedSize = cDefaultTargetEncodedSizeGiB * 1024 * 1024 * 1024; + constexpr auto cDefaultMaxDocumentSize = cDefaultMaxDocumentSizeMiB * 1024 * 1024; + constexpr auto cDefaultMinTableSize = cDefaultMinTableSizeMiB * 1024 * 1024;
110-166: Enhance error messages and add documentation.The search function would benefit from more descriptive error messages and documentation of the transformation workflow.
+/** + * Executes a search query and validates the results. + * + * Workflow: + * 1. Parse KQL expression + * 2. Apply transformation passes: + * - Standardize to OR-of-AND form + * - Narrow types + * - Convert to exists + * 3. Process archives and collect results + * 4. Validate results against expected outcomes + */ void search(std::string const& query, bool ignore_case, std::vector<int64_t> const& expected_results) { - REQUIRE(expected_results.size() > 0); + REQUIRE_MESSAGE(expected_results.size() > 0, "Expected results vector cannot be empty"); auto query_stream = std::istringstream{query}; auto expr = clp_s::search::kql::parse_kql_expression(query_stream); - REQUIRE(nullptr != expr); - REQUIRE(nullptr == std::dynamic_pointer_cast<clp_s::search::EmptyExpr>(expr)); + REQUIRE_MESSAGE(nullptr != expr, fmt::format("Failed to parse query: {}", query)); + REQUIRE_MESSAGE(nullptr == std::dynamic_pointer_cast<clp_s::search::EmptyExpr>(expr), + fmt::format("Query resulted in empty expression: {}", query));
175-177: Track the skipped test case.The commented-out test case should be tracked as a GitHub issue for follow-up.
Would you like me to create a GitHub issue to track the bug in
Grep::process_raw_queryand this skipped test case?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/core/CMakeLists.txt(6 hunks)components/core/src/clp_s/search/OutputHandler.hpp(2 hunks)components/core/tests/LogSuppressor.hpp(2 hunks)components/core/tests/TestOutputCleaner.hpp(1 hunks)components/core/tests/test-clp_s-end_to_end.cpp(5 hunks)components/core/tests/test-clp_s-search.cpp(1 hunks)components/core/tests/test_log_files/test_search.jsonl(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
components/core/tests/TestOutputCleaner.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/LogSuppressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/test-clp_s-search.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/test-clp_s-end_to_end.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp_s/search/OutputHandler.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
📓 Learnings (1)
components/core/tests/test-clp_s-end_to_end.cpp (1)
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
🪛 cppcheck (2.10-2)
components/core/tests/test-clp_s-search.cpp
[error] 191-191: Exception thrown in function declared not to throw exceptions.
(throwInNoexceptFunction)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/core/tests/LogSuppressor.hpp (1)
18-22: LGTM! Well-structured RAII implementation.The deletion of copy/move operations prevents potential issues with logging level management. The trailing return type syntax is used consistently.
components/core/tests/test-clp_s-end_to_end.cpp (2)
130-134: LGTM! Proper cleanup initialization.The TestOutputCleaner is correctly initialized with all necessary paths, ensuring proper cleanup before and after tests.
Line range hint
95-121: Enhance shell command security and cross-platform compatibility.The shell commands should properly escape paths and handle spaces. Consider using a cross-platform solution.
- auto command = fmt::format( - "jq --sort-keys --compact-output '.' {} | sort > {}", - extracted_json_path.string(), - cTestEndToEndOutputSortedJson - ); + auto command = fmt::format( + "jq --sort-keys --compact-output '.' {} | sort > {}", + std::filesystem::path(extracted_json_path).make_preferred().string(), + std::filesystem::path(cTestEndToEndOutputSortedJson).make_preferred().string() + );Also, verify the commands exist before using them:
components/core/tests/test-clp_s-search.cpp (2)
1-37: LGTM! Well-organized includes and constants.The includes are logically grouped, and constants follow a consistent naming convention.
38-48: LGTM! Well-designed function declarations.Functions are appropriately scoped in an anonymous namespace and have clear signatures using const references.
components/core/src/clp_s/search/OutputHandler.hpp (1)
298-345: LGTM! Well-designed output handler implementation.The VectorOutputHandler class is well-implemented with:
- Appropriate use of const references
- Clear member variable ownership
- Consistent interface implementation
components/core/CMakeLists.txt (1)
Line range hint
302-664: LGTM! Well-organized build configuration updates.The changes maintain a consistent style and clear organization of source files.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/core/tests/test-clp_s-search.cpp (3)
100-103: Enhance error message with exception details.Include the exception message in the failure output for better debugging.
- FAIL(fmt::format("Invalid JSON in result: {}", result.message)); + FAIL(fmt::format("Invalid JSON in result: {}. Error: {}", result.message, e.what()));
117-127: Document transformation passes.Add comments explaining the purpose of each transformation pass for better maintainability.
+ // Convert the expression to a standardized form (OR of ANDs) clp_s::search::OrOfAndForm standardize_pass; expr = standardize_pass.run(expr); REQUIRE(nullptr != expr); + // Narrow down the types in the expression for more efficient processing clp_s::search::NarrowTypes narrow_pass; expr = narrow_pass.run(expr); REQUIRE(nullptr != expr); + // Convert applicable expressions to existence checks clp_s::search::ConvertToExists convert_pass; expr = convert_pass.run(expr); REQUIRE(nullptr != expr);
130-162: Use RAII for archive reader resources.Consider using a unique_ptr or scope guard to ensure archive reader is closed even if an exception occurs.
+ class ArchiveReaderGuard { + public: + explicit ArchiveReaderGuard(std::shared_ptr<clp_s::ArchiveReader> reader) + : reader_(std::move(reader)) {} + ~ArchiveReaderGuard() { + if (reader_) { + reader_->close(); + } + } + auto get() const -> clp_s::ArchiveReader* { return reader_.get(); } + private: + std::shared_ptr<clp_s::ArchiveReader> reader_; + }; for (auto const& entry : std::filesystem::directory_iterator(cTestSearchArchiveDirectory)) { auto archive_reader = std::make_shared<clp_s::ArchiveReader>(); + ArchiveReaderGuard guard(archive_reader); // ... rest of the code ... - archive_reader->close(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/core/tests/test-clp_s-search.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
components/core/tests/test-clp_s-search.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 cppcheck (2.10-2)
components/core/tests/test-clp_s-search.cpp
[error] 191-191: Exception thrown in function declared not to throw exceptions.
(throwInNoexceptFunction)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (4)
components/core/tests/test-clp_s-search.cpp (4)
1-37: LGTM! Well-organized headers and constants.The headers are logically organized, and constants follow a consistent naming convention with clear prefixes.
38-48: LGTM! Well-structured function declarations.Good use of anonymous namespace for test-only functions and modern C++ conventions with trailing return types.
66-67: Improve directory creation error handling.The directory creation should be wrapped in a try-catch block to ensure proper cleanup on failure.
189-191: Handle potential exceptions in the test loop.The static analysis tool indicates that exceptions might be thrown in a noexcept context.
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 191-191: Exception thrown in function declared not to throw exceptions.
(throwInNoexceptFunction)
Description
This PR adds end to end search tests for clp-s and associated boilerplate with initial tests inspired by #590 and #254. More tests can be added in the future to expand coverage.
This PR also touches test-clp_s-end-to-end in order to test single-file-archive support.
The basic flow for the search tests is:
Validation performed
Summary by CodeRabbit
Release Notes
New Features
Testing
clp_ssearch functionality.Performance
Infrastructure