build: Add unit test build control through BUILD_TESTING and YSTDLIB_CPP_BUILD_TESTING flags; Support CTest through Catch2 functions.#30
Conversation
WalkthroughThis pull request updates the CMake configuration for building unit tests in the project. The modifications replace the old Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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 (
|
Bill-hbrhbr
changed the title
BUILD_TESTING to YSTDLIB_CPP_BUILD_TESTING for better namespace clarity.YSTDLIB_CPP_BUILD_TESTING for unit tests when ystdlib-cpp is not the top-level project.
Bill-hbrhbr
changed the title
YSTDLIB_CPP_BUILD_TESTING for unit tests when ystdlib-cpp is not the top-level project.YSTDLIB_CPP_BUILD_TESTING for unit tests building when ystdlib-cpp is not the top-level project.
Bill-hbrhbr
changed the title
YSTDLIB_CPP_BUILD_TESTING for unit tests building when ystdlib-cpp is not the top-level project.YSTDLIB_CPP_BUILD_TESTING for unit tests building when ystdlib-cpp is not the top-level project.
Bill-hbrhbr
changed the title
YSTDLIB_CPP_BUILD_TESTING for unit tests building when ystdlib-cpp is not the top-level project.YSTDLIB_CPP_BUILD_TESTING for unit tests building when ystdlib-cpp is not the top-level project.
Bill-hbrhbr
changed the title
YSTDLIB_CPP_BUILD_TESTING for unit tests building when ystdlib-cpp is not the top-level project.YSTDLIB_CPP_BUILD_TESTING for projects using ystdlib-cpp.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
davidlion
left a comment
davidlion
left a comment
There was a problem hiding this comment.
- integrate with ctest since it is so easy https://github.com/catchorg/Catch2/blob/devel/docs/cmake-integration.md#usage
- simplify the flag logic a bit: follow
BUILD_TESTINGbut allow an override throughYSTDLIB_CPP_BUILD_TESTING- default
YSTDLIB_CPP_BUILD_TESTINGtoON if (NOT (YSTDLIB_CPP_BUILD_TESTING AND BUILD_TESTING)) set(YSTDLIB_CPP_ENABLE_TESTS OFF)(if both flags are set run unit tests if either is off we turn off enable_tests)- as a side-effect we don't need to set/modify
BUILD_TESTING(and can leave it to the user setting) - can drop
YSTDLIB_CPP_MASTER_PROJECT - use
YSTDLIB_CPP_ENABLE_TESTSsame as before
- as a side-effect we don't need to set/modify
- default
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
6-7: Heading Style Consistency
The new header “## Via CMake's add_subdirectory()” uses an ATX-style heading. However, static analysis hints (MD003) expect a setext style. Please confirm the preferred header style for the project and update accordingly if needed.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
CMakeLists.txt (1)
32-39: Catch2 Configuration and Version Specificity
The block that conditionally finds Catch2 and issues a status or fatal error is straightforward and beneficial for user feedback. However, consider whether the version requirement (3.8.0) might be too restrictive over time or if a minimum version check would be more appropriate in the long term.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(3 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
7-7: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (5)
README.md (1)
10-11: Clarify Test Disabling Instructions
The commented option clearly instructs users to disable unit tests when desired. Consider adding a brief note that explains why one might want to disable the unit tests in the context of subproject integration, reinforcing the purpose of the new option.CMakeLists.txt (4)
6-7: Include Dependent Option Module
IncludingCMakeDependentOptionat this stage is crucial for supporting the new dependent option logic later in the file. Ensure that this module is available in the CMake module path in all environments where the project is built.
16-19: Conditional CTest Inclusion
The conditional block usingYSTDLIB_CPP_IS_TOP_LEVELto include CTest is clear and fulfils the intended design. Please verify that theYSTDLIB_CPP_IS_TOP_LEVELvariable is reliably set in all downstream projects to avoid unexpected behavior regarding test integration.
20-27: New Testing Options Configuration
Introducing theYSTDLIB_CPP_BUILD_TESTINGoption along with the dependent optionYSTDLIB_CPP_ENABLE_TESTSeffectively decouples the parent project’sBUILD_TESTINGsetting from this subproject’s testing configuration. The dependency string"BUILD_TESTING;YSTDLIB_CPP_BUILD_TESTING"should be carefully verified to ensure that tests are enabled only under the intended conditions.
39-54: Unified Unit Test Target Setup
The configuration of the unified unit test target (unit-test-all) is implemented cleanly. The explicit setting of the output directory and the use ofcatch_discover_testsare good practices that facilitate test discovery. Ensure that any consumers of this target are made aware of the directory customisation.
Bill-hbrhbr
changed the title
YSTDLIB_CPP_BUILD_TESTING for projects using ystdlib-cpp.YSTDLIB_CPP_BUILD_TESTING to control unit test building for ystdlib-cpp.
Bill-hbrhbr
changed the title
YSTDLIB_CPP_BUILD_TESTING to control unit test building for ystdlib-cpp.YSTDLIB_CPP_BUILD_TESTING to control unit test building for ystdlib-cpp; Integrate CTest with Catch2.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (2)
6-7: Suggestion: Confirm Heading Style ConsistencyThe new section heading ("## Via CMake's add_subdirectory()") uses ATX style, but a static analysis hint suggests that a setext style might be expected. Please confirm whether this style is intentional according to your project's markdown guidelines.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
10-11: Clarification on Testing InstructionsThe comment instructs users to uncomment the line if they do not wish to build unit tests. This is clear, but a brief note on the impact of disabling tests (e.g. any downstream effects) would improve clarity.
CMakeLists.txt (1)
18-21: Refine Test Enabling Condition for Undefined BUILD_TESTINGThe condition used:
if(BUILD_TESTING AND YSTDLIB_CPP_BUILD_TESTING) set(YSTDLIB_CPP_ENABLE_TESTS ON) endif()only enables tests when
BUILD_TESTINGis explicitly true. According to the PR objectives, tests should also be enabled whenBUILD_TESTINGis undefined. Consider revising the condition as follows:-if(BUILD_TESTING AND YSTDLIB_CPP_BUILD_TESTING) - set(YSTDLIB_CPP_ENABLE_TESTS ON) -endif() +if((NOT DEFINED BUILD_TESTING OR BUILD_TESTING) AND YSTDLIB_CPP_BUILD_TESTING) + set(YSTDLIB_CPP_ENABLE_TESTS ON) +endif()This change would more closely align with the intended behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(2 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
7-7: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (3)
CMakeLists.txt (3)
14-16: Conditional Inclusion of CTestThe conditional inclusion of CTest using
if(YSTDLIB_CPP_IS_TOP_LEVEL)is well implemented, ensuring that CTest is only included when the project is the top-level one.
26-34: Integration of Catch2 for Unit TestingThe updated block correctly locates and includes Catch2 (version 3.8.0) and prints a descriptive status message. This ensures that test discovery is only performed when tests are enabled. Verify that pinning the version to 3.8.0 meets your project's compatibility requirements.
47-47: Verification of Test Discovery SetupThe call to
catch_discover_tests(${UNIFIED_UNIT_TEST_TARGET} WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/testbin)is appropriate for automating test discovery. Please double-check that the working directory is correctly set in all target environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
6-7: Heading Style Consideration
The new section heading"## Via CMake's add_subdirectory()"uses an ATX style, but markdownlint suggests using a setext style for consistency (as noted in the static analysis hint). Please review the project's markdown style guidelines and adjust if necessary.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(2 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
7-7: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (4)
README.md (1)
10-11: Clarify Instruction Comment for Unit Tests
The commented option (# option(YSTDLIB_CPP_BUILD_TESTING "" OFF)) clearly informs users how to disable unit tests when including the library via CMake. This is a useful addition—just ensure the descriptive text in the comment aligns with the overall documentation style.CMakeLists.txt (3)
14-18: Conditional Inclusion of CTest
Introducing the conditional block that includes CTest only whenYSTDLIB_CPP_IS_TOP_LEVELis true is a sound strategy to avoid conflicts with the parent project’s testing settings. Make sure this behaviour is well documented so that users know that unit test support is only auto-enabled in top-level projects.
19-22: New Testing Option Logic
The new optionYSTDLIB_CPP_BUILD_TESTINGand the subsequent logic checking that bothBUILD_TESTINGandYSTDLIB_CPP_BUILD_TESTINGare true to setYSTDLIB_CPP_ENABLE_TESTSare implemented clearly. This approach effectively isolates test building configuration for the subproject.
27-49: Unified Unit Test Target and Catch2 Integration
The block handling test integration is well structured. A couple of points to consider:
- The requirement for Catch2 version
3.8.0is strict; verify that this meets your project needs or consider allowing a broader version range if flexibility is desired.- The use of
catch_discover_teststo configure the unified test target is an effective approach. Ensure that linking withCatch2::Catch2WithMainand setting C++20 features are compatible across your target platforms.
Overall, this section meets the intended goals for unit test configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
7-7: Nitpick: Consider Consistent Heading Style
The heading “## Via CMake's add_subdirectory()” uses the atx style. Static analysis suggests using setext style for consistency. You might consider changing it to a setext-style heading if that aligns with the project’s markdown conventions.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
CMakeLists.txt (1)
24-27: Verify: Conditional Enabling of Unit Tests
The new optionYSTDLIB_CPP_BUILD_TESTINGis combined withBUILD_TESTINGto setYSTDLIB_CPP_ENABLE_TESTS. Please verify that this logic meets the intended behaviour whenBUILD_TESTINGis either undefined or set to ON in parent projects. A brief clarification in the accompanying documentation might help users understand the condition when ystdlib-cpp is added as a subproject.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(3 hunks)README.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
7-7: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
README.md (1)
67-70: Approved: Clear Explanation of Unit Test Configuration
The new text clearly explains that as a top-level CMake project,ystdlib-cppalways builds unit tests, and when included as a subproject it respects the parent-scopeBUILD_TESTINGflag. This enhancement addresses the earlier review comment about documenting the interplay betweenBUILD_TESTINGandYSTDLIB_CPP_BUILD_TESTING. If feasible, you might also consider restating this explanation in the “## Testing” section to further assist users.CMakeLists.txt (5)
1-1: Approved: Updated Minimum Required CMake Version
The minimum required CMake version has been bumped to 3.22.1, which is appropriate given the newer features and improved handling (e.g. usingPROJECT-NAME_IS_TOP_LEVEL). Ensure this change is clearly documented for users upgrading from older versions.
14-22: Approved: Conditional Inclusion of CTest
Usingif(YSTDLIB_CPP_IS_TOP_LEVEL)to conditionally include the CTest module is a sensible design choice. This ensures that dependency settings and testing modules are only activated when the project is the top-level project.
32-40: Approved: Catch2 Integration for Unit Testing
The block that checksif(YSTDLIB_CPP_ENABLE_TESTS)and then finds Catch2 (version 3.8.0) is well implemented. The informative status message and fatal error in case of failure provide clear feedback.
41-54: Approved: Unified Unit Test Target Setup
The creation of the unified unit test target—including linking withCatch2::Catch2WithMain, setting the C++ standard, and configuring the output directory—is well implemented. The use ofcatch_discover_testsfurther facilitates automated test discovery.
56-59: Approved: Inclusion of Helper Functions and Subdirectory
The final inclusion ofystdlib-cpp-helpersand adding the subdirectory forsrc/ystdlibintegrate the library’s structure coherently.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
I already added doc to explain build testing flags. |
davidlion
changed the title
YSTDLIB_CPP_BUILD_TESTING to control unit test building for ystdlib-cpp; Integrate CTest with Catch2.BUILD_TESTING and YSTDLIB_CPP_BUILD_TESTING flags; Support CTest through Catch2 functions.
Co-authored-by: davidlion <davidlion2@protonmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
6-6: Consistent Markdown Header StyleConsider converting the “## Via CMake's add_subdirectory()” header to setext style to match the project’s markdown conventions. For example, replacing it with:
-## Via CMake's add_subdirectory() +Via CMake's add_subdirectory() +-------------------------------This small change will improve the consistency of the document formatting.
9-10: Clarify Commented OptionThe commented-out CMake option (
# option(YSTDLIB_CPP_BUILD_TESTING "" OFF)) is clear in intent. You might consider adding a brief note that, in CMake, any accepted boolean (e.g. OFF/false) is valid, which can help avoid ambiguity for users less familiar with CMake’s variations in boolean inputs.
66-69: Enhance Testing Target ExplanationThere is a minor grammatical issue in the wording (“testing target” should be clarified for noun/verb agreement) and some potential ambiguity. Consider rephrasing the lines as follows:
-When generating testing target the CMake variable `BUILD_TESTING` is followed, unless overruled by -setting `YSTDLIB_CPP_BUILD_TESTING` to false. By default, if built as a top-level project -`BUILD_TESTING` will be set to true and unit tests will be built. +When generating a testing target, the CMake variable `BUILD_TESTING` is followed (unless overruled by setting `YSTDLIB_CPP_BUILD_TESTING` to false). By default, if built as a top-level project, `BUILD_TESTING` is set to true and unit tests are built.This adjustment improves grammatical accuracy and clarity for users.
[grammar]🧰 Tools
🪛 LanguageTool
[grammar] ~66-~66: There seems to be a noun/verb agreement error. Did you mean “targets” or “targeted”?
Context: ...<lib_name> ``` When generating testing target the CMake variableBUILD_TESTINGis f...(SINGULAR_NOUN_VERB_AGREEMENT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~66-~66: There seems to be a noun/verb agreement error. Did you mean “targets” or “targeted”?
Context: ...<lib_name> ``` When generating testing target the CMake variable BUILD_TESTING is f...
(SINGULAR_NOUN_VERB_AGREEMENT)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
README.md (1)
1-1: Header Title CheckThe main header (“# ystdlib-cpp”) is clear and adheres to the project’s branding.
|
@coderabbitai I thought the README already uses ATX style markdown |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@Bill-hbrhbr You're absolutely right! After checking the README.md file, I can confirm it consistently uses ATX style headings (with # symbols) throughout the document. My previous nitpick suggesting a change to setext style was incorrect. The "## Via CMake's add_subdirectory()" heading is already consistent with the document's established style. I apologize for the confusion. 🏁 Script executed: Length of output: 591 @Bill-hbrhbr Thank you for pointing this out. The script results confirm that you're absolutely right! The README.md consistently uses ATX style headings (with # symbols) throughout the document, including:
No setext style headings (underlined with = or -) were found in the file. My previous nitpick suggestion to change "## Via CMake's add_subdirectory()" to setext style was incorrect and would have actually made it inconsistent with the rest of the document. I apologize for the confusion. ✏️ Learnings added
|
|
@coderabbitai You no longer comment directly on the files, so how can I |
|
Tip For best results, initiate chat on the files or code changes. @Bill-hbrhbr You're right to point out this confusion! Let me clarify how you can initiate chat about specific files or code changes:
The tip was meant to encourage focused questions about specific code rather than general queries, but I understand how it was confusing given the current interface. I can still analyze and respond to questions about any file in the repository - you just need to tell me which file and what aspect you're interested in. Is there a specific part of the code you'd like to discuss? |
3 participants
Description
When including
ystdlib-cppas a subproject, all options should be prefixed withYSTDLIB_CPP_to avoid affecting parent project settings.Regarding unit test building, we want to respect the
BUILD_TESTINGflag of the parent project. However, if users wish to turn off unit test building only forystdlib-cpp, they can specifybefore including
ystdlib-cppas a subproject.We also bring in
CTestframework since it's easy to integrate it withCatch2'scatch_discover_tests().Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit