build(deps-core): Bump Catch2 version from v2.13.7 to v3.8.0. #981
Conversation
WalkthroughRequire Catch2 v3.8.0, switch tests to modular Catch2 headers, remove the custom Catch2 main (delete test-main.cpp), link unit tests against Catch2::Catch2WithMain, and update docs and dependency task to fetch and build Catch2 v3.8.0 with C++20. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev
participant CMake
participant Catch2
participant UnitTests
Dev->>CMake: configure project
CMake->>Catch2: find_package(Catch2 3.8.0 REQUIRED)
CMake->>UnitTests: link Catch2::Catch2WithMain
Note right of UnitTests: local test-main.cpp removed — Catch2 provides main()
Dev->>UnitTests: build & run tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-08-09T04:07:27.083ZApplied to files:
📚 Learning: 2025-08-04T17:23:14.646ZApplied to files:
📚 Learning: 2025-08-13T19:58:26.033ZApplied to files:
⏰ 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). (9)
🔇 Additional comments (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
![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
🔭 Outside diff range comments (7)
components/core/tests/test-EncodedVariableInterpreter.cpp (2)
382-386: Typo: “metching” → “matching” in SECTION nameFix the test’s SECTION title for clarity.
- SECTION("Test multiple metching values") { + SECTION("Test multiple matching values") {
66-192: Style: Prefer ‘false == expr’ over ‘!expr’ to follow project guidelineSeveral REQUIRE calls negate the expression (e.g., REQUIRE(!EncodedVariableInterpreter::...)). The repository’s guideline prefers false == .
Example refactor pattern:
- From: REQUIRE(!EncodedVariableInterpreter::convert_string_to_representable_integer_var(value, encoded_var));
- To: REQUIRE(false == EncodedVariableInterpreter::convert_string_to_representable_integer_var(value, encoded_var));
components/core/tests/clp_s_test_utils.cpp (1)
50-52: Replace ‘REQUIRE(false)’ with ‘FAIL’ for unreachable branchMore expressive and clearer failure.
- // This branch should be unreachable. - REQUIRE(false); + // This branch should be unreachable. + FAIL("Unreachable branch reached");components/core/tests/test-clp_s-end_to_end.cpp (3)
72-79: Quote paths in shell commands to handle spaces/special charactersProtects against failures if paths contain spaces.
- 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 > '{}'", + extracted_json_path.string(), + cTestEndToEndOutputSortedJson + );
84-91: Also quote arguments in diff invocationAvoids misparsing if filenames contain spaces.
- command = fmt::format( - "diff --unified {} {} > /dev/null", - cTestEndToEndOutputSortedJson, - get_test_input_local_path() - ); + command = fmt::format( + "diff --unified '{}' '{}' > /dev/null", + cTestEndToEndOutputSortedJson, + get_test_input_local_path() + );
53-56: Use ‘=’ for designated initializers (C++20) — fix occurrencesC++20 designated initializers require ‘=’. The repo contains
.member{...}uses that should be changed to.member = ...(may fail on strict compilers).Files/locations to update:
- components/core/tests/test-clp_s-end_to_end.cpp: lines ~54–55
- components/core/tests/test-clp_s-search.cpp: lines ~157–158
- components/core/tests/test-clp_s-range_index.cpp: lines ~132–133
- components/core/src/clp_s/ArchiveWriter.cpp: lines ~184, 241, 246
Example replacements (apply same pattern to all occurrences):
- tests (example):
- constructor_option.archive_path = clp_s::Path{ - .source{clp_s::InputSource::Filesystem}, - .path{entry.path().string()} - }; + constructor_option.archive_path = clp_s::Path{ + .source = clp_s::InputSource::Filesystem, + .path = entry.path().string() + };
- ArchiveWriter.cpp (examples):
- ArchiveFileInfoPacket archive_file_info{.files{files}}; + ArchiveFileInfoPacket archive_file_info{ .files = files }; - .magic_number{0}, - .reserved_padding{0}, + .magic_number = 0, + .reserved_padding = 0,Please update all listed sites to use ‘=’ so the code is valid C++20.
components/core/tests/test-clp_s-search.cpp (1)
156-160: Designated initializers: fix to use ‘=’ per C++20Same issue as in end-to-end test; current syntax is non-conforming.
- auto archive_path = clp_s::Path{ - .source{clp_s::InputSource::Filesystem}, - .path{entry.path().string()} - }; + auto archive_path = clp_s::Path{ + .source = clp_s::InputSource::Filesystem, + .path = entry.path().string() + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
components/core/CMakeLists.txt(2 hunks)components/core/tests/clp_s_test_utils.cpp(1 hunks)components/core/tests/test-EncodedVariableInterpreter.cpp(1 hunks)components/core/tests/test-ParserWithUserSchema.cpp(1 hunks)components/core/tests/test-clp_s-end_to_end.cpp(1 hunks)components/core/tests/test-clp_s-range_index.cpp(1 hunks)components/core/tests/test-clp_s-search.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/tests/test-clp_s-end_to_end.cppcomponents/core/tests/test-clp_s-search.cppcomponents/core/tests/test-ParserWithUserSchema.cppcomponents/core/tests/test-clp_s-range_index.cppcomponents/core/tests/test-EncodedVariableInterpreter.cppcomponents/core/tests/clp_s_test_utils.cpp
🧠 Learnings (6)
📚 Learning: 2024-11-19T17:30:04.970Z
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.
Applied to files:
components/core/tests/test-clp_s-end_to_end.cppcomponents/core/tests/clp_s_test_utils.cpp
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/tests/test-clp_s-end_to_end.cppcomponents/core/tests/test-clp_s-search.cppcomponents/core/tests/test-ParserWithUserSchema.cppcomponents/core/tests/test-clp_s-range_index.cppcomponents/core/tests/test-EncodedVariableInterpreter.cppcomponents/core/tests/clp_s_test_utils.cpp
📚 Learning: 2024-10-14T03:42:10.355Z
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Applied to files:
components/core/tests/test-clp_s-search.cppcomponents/core/tests/test-clp_s-range_index.cppcomponents/core/tests/test-EncodedVariableInterpreter.cpp
📚 Learning: 2024-10-13T09:27:43.408Z
Learnt from: LinZhihao-723
PR: y-scope/clp#557
File: components/core/tests/test-ir_encoding_methods.cpp:1216-1286
Timestamp: 2024-10-13T09:27:43.408Z
Learning: In the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp`, suppressing the `readability-function-cognitive-complexity` warning is acceptable due to the expansion of Catch2 macros in C++ tests, and such test cases may not have readability issues.
Applied to files:
components/core/tests/test-ParserWithUserSchema.cppcomponents/core/tests/test-EncodedVariableInterpreter.cpp
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/tests/clp_s_test_utils.cpp
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
components/core/CMakeLists.txt
⏰ 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). (7)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (5)
components/core/tests/test-EncodedVariableInterpreter.cpp (1)
3-3: Catch2 v3 modular include is correct hereSwitching to catch_test_macros.hpp matches the macros used in this file (REQUIRE, SECTION, etc.). No additional Catch2 headers are needed.
components/core/tests/clp_s_test_utils.cpp (1)
7-7: Catch2 include update is appropriateOnly macros (REQUIRE, REQUIRE_NOTHROW) are used; catch_test_macros.hpp suffices.
components/core/tests/test-ParserWithUserSchema.cpp (1)
10-11: Correct Catch2 v3 modular includes
- catch_test_macros.hpp covers REQUIRE, REQUIRE_THROWS_WITH, CAPTURE, etc.
- catch_matchers.hpp is fine if you plan to use matchers; safe to keep.
components/core/tests/test-clp_s-end_to_end.cpp (1)
9-10: Correct Catch2 v3 modular includesMacros and generators used here are properly covered by catch_test_macros.hpp and catch_generators.hpp.
components/core/tests/test-clp_s-search.cpp (1)
12-13: Catch2 v3 modular includes are correctTests use macros and generators; the chosen headers are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/core/CMakeLists.txt (1)
168-170: Prefer config-mode to avoid stale FindCatch2 modules (repeat suggestion)Force CONFIG to ensure you pick up Catch2 v3’s package config and not an unintended FindCatch2.cmake via CMAKE_MODULE_PATH. REQUIRED already guarantees the subsequent message is safe.
Apply this diff:
-if(CLP_NEED_CATCH2) - find_package(Catch2 3.8.0 REQUIRED) - message(STATUS "Found Catch2 ${Catch2_VERSION}") -endif() +if(CLP_NEED_CATCH2) + find_package(Catch2 3.8.0 CONFIG REQUIRED) + message(STATUS "Found Catch2 ${Catch2_VERSION}") +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
components/core/CMakeLists.txt(2 hunks)components/core/tests/test-GlobalMetadataDBConfig.cpp(1 hunks)components/core/tests/test-GrepCore.cpp(1 hunks)components/core/tests/test-clp_s-delta-encode-log-order.cpp(1 hunks)taskfiles/deps/main.yaml(1 hunks)taskfiles/lint.yaml(0 hunks)
💤 Files with no reviewable changes (1)
- taskfiles/lint.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/tests/test-clp_s-delta-encode-log-order.cppcomponents/core/tests/test-GrepCore.cppcomponents/core/tests/test-GlobalMetadataDBConfig.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/tests/test-clp_s-delta-encode-log-order.cppcomponents/core/tests/test-GrepCore.cppcomponents/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: 2024-11-19T17:30:04.970Z
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.
Applied to files:
components/core/tests/test-clp_s-delta-encode-log-order.cpp
📚 Learning: 2025-07-01T14:49:07.333Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/tests/test-clp_s-delta-encode-log-order.cpp
📚 Learning: 2024-10-13T09:27:43.408Z
Learnt from: LinZhihao-723
PR: y-scope/clp#557
File: components/core/tests/test-ir_encoding_methods.cpp:1216-1286
Timestamp: 2024-10-13T09:27:43.408Z
Learning: In the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp`, suppressing the `readability-function-cognitive-complexity` warning is acceptable due to the expansion of Catch2 macros in C++ tests, and such test cases may not have readability issues.
Applied to files:
components/core/tests/test-GrepCore.cpp
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-08-13T19:58:26.033Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: taskfiles/deps/main.yaml:0-0
Timestamp: 2025-08-13T19:58:26.033Z
Learning: In the CLP project, custom CMake find modules (like FindLibLZMA.cmake) are designed to flexibly link against either shared or static libraries based on availability, rather than requiring both variants to be built. The find modules use flags like LibLZMA_USE_STATIC_LIBS and temporarily modify CMAKE_FIND_LIBRARY_SUFFIXES to prefer the desired library type while gracefully falling back if needed.
Applied to files:
components/core/CMakeLists.txt
⏰ 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). (9)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (6)
components/core/tests/test-GlobalMetadataDBConfig.cpp (1)
10-10: Catch2 v3 modular include is correctSwitching to catch_test_macros.hpp is appropriate for v3 since only test macros are used here.
components/core/tests/test-GrepCore.cpp (1)
3-3: Good move to Catch2 v3 split headerUsing catch_test_macros.hpp is sufficient here; no generators or matchers are used in this file.
components/core/tests/test-clp_s-delta-encode-log-order.cpp (1)
9-10: Correct split headers for macros and generatorsIncluding both catch_test_macros.hpp and generators/catch_generators.hpp is the right combination for REQUIRE/SECTION and GENERATE.
taskfiles/deps/main.yaml (2)
191-196: Catch2 v3.8.0 bump and C++20 for the build look good
- URL and SHA256 updated to v3.8.0.
- Forcing CMAKE_CXX_STANDARD=20 at build time is a practical fix for std::string_view comparison/linking issues.
191-196: Catch2 tarball SHA256 verifiedDownloaded the v3.8.0 tarball and computed its SHA‑256 — it matches the value in the repo (1ab2de20460d4641553addfdfe6acd4109d871d5531f8f519a52ea4926303087).
- Location: taskfiles/deps/main.yaml — lines 191–196 (TARBALL_URL / TARBALL_SHA256)
No changes required.
components/core/CMakeLists.txt (1)
749-749: Linking tests against Catch2::Catch2WithMain is correctThis aligns with removing the custom test main and the v3 package layout.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/deps/main.yaml (1)
191-193: C++20 enforcement looks good; resolves the std::string_view issuesSetting CMAKE_CXX_STANDARD to 20 and marking it REQUIRED is exactly what we need for Catch2 v3.8.0 and aligns with the PR objective.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
taskfiles/deps/main.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
📚 Learning: 2025-08-04T17:23:14.646Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/CMakeLists.txt:171-172
Timestamp: 2025-08-04T17:23:14.646Z
Learning: Bill-hbrhbr prefers concise, straightforward code over verbose defensive programming in CMake files. When find_package uses REQUIRED flag, additional existence checks are redundant and should be avoided.
Applied to files:
taskfiles/deps/main.yaml
⏰ 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). (6)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (1)
taskfiles/deps/main.yaml (1)
195-196: [runscripts]#!/bin/bash set -euo pipefail URL="https://github.com/catchorg/Catch2/archive/refs/tags/v3.8.0.tar.gz" EXPECTED_SHA="1ab2de20460d4641553addfdfe6acd4109d871d5531f8f519a52ea4926303087" TMP="$(mktemp -d)" trap 'rm -rf "$TMP"' EXIT echo "Downloading Catch2 v3.8.0..." curl -fsSL "$URL" -o "$TMP/Catch2-v3.8.0.tar.gz" echo "Computing SHA-256 via Python..." ACTUAL_SHA="$(python3 - << 'EOF' import hashlib path = "${TMP}/Catch2-v3.8.0.tar.gz" h = hashlib.sha256() with open(path, "rb") as f: for chunk in iter(lambda: f.read(8192), b""): h.update(chunk) print(h.hexdigest()) EOF )" echo "Expected: $EXPECTED_SHA" echo "Actual: $ACTUAL_SHA" if [[ "$ACTUAL_SHA" != "$EXPECTED_SHA" ]]; then echo "ERROR: SHA-256 mismatch." >&2 exit 1 fi echo "OK: SHA-256 matches."
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
components/core/tests/test-string_utils.cpp (5)
1-7: Add missing standard headers now that Catch2’s single-header umbrella is gone.This file relies on transitive includes for several standard types/symbols (string, vector, pair, chrono, int64_t, INT64_MAX/MIN). With Catch2 v3’s split headers, those transitive includes may no longer be present, causing brittle or failing builds.
Apply this diff to explicitly include what you use:
#include <iostream> +#include <chrono> +#include <cstdint> +#include <string> +#include <utility> +#include <vector> #include <boost/foreach.hpp> #include <boost/range/combine.hpp> #include <catch2/catch_test_macros.hpp> +#include <catch2/generators/catch_generators.hpp> #include <string_utils/constants.hpp> #include <string_utils/string_utils.hpp>
552-560: Bug: “next best implementation” updates the wrong accumulator variable.Inside the “Profile next best implementation” loop, you’re updating allPassed_currentImplementation instead of allPassed_nextBestImplementation (Lines 558–560). This prevents the “next best” pass/fail from being tracked correctly.
Apply this diff:
while (testReps--) { // Replace this part with slow implementation BOOST_FOREACH (boost::tie(tameStr, wildStr), boost::combine(tameVec, wildVec)) { - allPassed_currentImplementation - &= wildcard_match_unsafe_case_sensitive(tameStr, wildStr); + allPassed_nextBestImplementation + &= wildcard_match_unsafe_case_sensitive(tameStr, wildStr); } }
354-360: Remove duplicate assertions.These two lines repeat the exact same assertion block twice (Lines 355 and 359/360). Trim the duplicate.
Apply this diff:
GIVEN("All lower case tame and mixed lower and upper case wild") { tameString = "abcde", wildString = "A?c*"; REQUIRE(wildcard_match_unsafe(tameString, wildString, isCaseSensitive) == true); - - tameString = "abcde", wildString = "A?c*"; - REQUIRE(wildcard_match_unsafe(tameString, wildString, isCaseSensitive) == true); }
403-404: Remove duplicate test line.The two consecutive lines are identical and redundant.
Apply this diff:
- allPassed &= wildcard_match_unsafe_case_sensitive("abAbac", "*Abac*"); - allPassed &= wildcard_match_unsafe_case_sensitive("abAbac", "*Abac*"); + allPassed &= wildcard_match_unsafe_case_sensitive("abAbac", "*Abac*");
370-373: Align with project style: Prefer ‘false == expr’ over ‘!expr’.Per the repository’s guidelines, prefer “false == ” instead of using the logical negation operator. There are multiple occurrences (examples in the ranges above).
Here are a few examples showing the intended change:
- allPassed - &= !wildcard_match_unsafe_case_sensitive("xxxx*zzzzzzzzy*f", "xxxx*zzy*fffff"); + allPassed + &= (false == wildcard_match_unsafe_case_sensitive("xxxx*zzzzzzzzy*f", "xxxx*zzy*fffff")); - allPassed &= !wildcard_match_unsafe_case_sensitive("a12b12", "*12*23"); + allPassed &= (false == wildcard_match_unsafe_case_sensitive("a12b12", "*12*23")); - REQUIRE(!wildcard_match_unsafe_case_sensitive("bLah", "bLaH") == true); + REQUIRE(false == wildcard_match_unsafe_case_sensitive("bLah", "bLaH"));If you prefer, I can generate a follow-up patch that updates all occurrences across this file for consistency.
Also applies to: 381-383, 391-392, 405-406, 424-431, 436-441, 455-463, 479-481, 492-495
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/core/tests/test-string_utils.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/tests/test-string_utils.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/tests/test-string_utils.cpp
⏰ 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). (7)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (2)
components/core/tests/test-string_utils.cpp (2)
206-214: Fix mismatch between test description and pattern (“?” vs “?”).The GIVEN text says “'?' pattern with 1 matched char for ''” (Line 211), but the code sets wildString to "?cd" (Line 212), which corresponds to "?" not "?*". This is confusing and makes the test misleading.
Apply this diff to align the test with its description:
- GIVEN("Wild begins with \"?*\" pattern with 1 matched char for \"*\"") { - tameString = "abcd", wildString = "*?cd"; + GIVEN("Wild begins with \"?*\" pattern with 1 matched char for \"*\"") { + tameString = "abcd", wildString = "?*cd"; REQUIRE(wildcard_match_unsafe_case_sensitive(tameString, wildString) == true); }Please verify if the intended pattern was indeed "?". If "?" was intended, update the description instead.
216-224: Fix another “?” vs “?” mismatch (“ends with … with 0 matched char”).Similar to the previous comment: the GIVEN says “'?' pattern” (Line 221) but code uses "abc?" (Line 222), which is "?" not "?".
Apply this diff to correct the pattern:
- GIVEN("Wild ends with \"?*\" pattern with 0 matched char for \"*\"") { - tameString = "abcd", wildString = "abc*?"; + GIVEN("Wild ends with \"?*\" pattern with 0 matched char for \"*\"") { + tameString = "abcd", wildString = "abc?*"; REQUIRE(wildcard_match_unsafe_case_sensitive(tameString, wildString) == true); }Confirm the intended pattern and adjust either description or string accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
Description
Bump the version of Catch2 to v3 because that's what we use in ystdlib-cpp. In the near future, we will build and install
ystdlib-cppwith the same package thatclpuses. Several changes to note:test-main.cppand link theunitTesttarget againstCatch2::Catch2WithMainstd::string_viewcomparison issuesMore v3 features can be found in the Catch2 doc.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Chores
Tests
Documentation