Use CMLIB validated TEST.cmake and test_var.cmake #13
Conversation
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
WalkthroughThe changes replace all local macro and function definitions for testing and cache variable management in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ 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). (1)
✨ 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. 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
🧹 Nitpick comments (2)
test/TEST.cmake (1)
2-3: Fix typo and clarify dependency statement.There's a spelling error: "unnecesssary" should be "unnecessary". Also, the comment about "avoiding dependencies" seems contradictory since this change actually introduces a dependency on CMLIB.
-# TEST.cmake are shared with CMLIB to avoid unnecesssary dependencies. -# TESTS.cmake is properly validated. +# TEST.cmake is shared with CMLIB to avoid duplicate dependencies. +# TEST.cmake is properly validated.test/cache_var.cmake (1)
2-3: Fix typo and clarify dependency statement.Same issues as in TEST.cmake: "unnecesssary" should be "unnecessary", and the dependency statement could be clearer.
-# cache_var.cmake are shared with CMLIB to avoid unnecesssary dependencies. +# cache_var.cmake is shared with CMLIB to avoid duplicate dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/TEST.cmake(1 hunks)test/cache_var.cmake(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt:120-122
Timestamp: 2025-06-16T20:53:24.161Z
Learning: The CACHE_VAR_FORCE_SET macro in cache_var.cmake is designed to handle multiple calls safely - it only saves the original cache variable state on the first call and preserves that saved state through subsequent calls, allowing proper restoration to the pre-test value.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/TEST.cmake:248-253
Timestamp: 2025-07-15T21:03:21.495Z
Learning: In CMake's GET_PROPERTY and MATCHES operations, explicit NOTFOUND checking is not needed for functions like TEST_CHECK_TARGET_PROPERTY_CONTAINS because CMake handles missing properties automatically during pattern matching operations.
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt:93-95
Timestamp: 2025-06-16T19:51:02.357Z
Learning: In modern CMake, the IF command automatically dereferences variables in conditions. For example, `IF(variable_name MATCHES pattern)` works correctly without needing explicit variable expansion syntax like `IF("${variable_name}" MATCHES pattern)`. The explicit syntax is not required for basic variable dereferencing in IF conditions.
📚 Learning: the cache_var_force_set macro in cache_var.cmake is designed to handle multiple calls safely - it on...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_LIBRARY/test_cases/cache_variables_validation/CMakeLists.txt:120-122
Timestamp: 2025-06-16T20:53:24.161Z
Learning: The CACHE_VAR_FORCE_SET macro in cache_var.cmake is designed to handle multiple calls safely - it only saves the original cache variable state on the first call and preserves that saved state through subsequent calls, allowing proper restoration to the pre-test value.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: when overriding built-in cmake commands (like install) for testing purposes, macro() must be used in...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/install_override.cmake:29-52
Timestamp: 2025-06-16T19:50:42.083Z
Learning: When overriding built-in CMake commands (like INSTALL) for testing purposes, MACRO() must be used instead of function() to properly shadow the original command. This is a specific case where the macro form is technically required despite general preferences for functions to avoid variable leakage.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: the cmakelib-component-cmdef project uses a custom testing framework with test.cmake and custom macr...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMakeLists.txt:9-11
Timestamp: 2025-06-17T22:15:13.853Z
Learning: The cmakelib-component-cmdef project uses a custom testing framework with TEST.cmake and custom macros like TEST_RUN, TEST_RUN_AND_CHECK_OUTPUT rather than CTest. CTest integration is not needed or used in this project.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: in cmdef_env cache variable validation tests, hardcoded platform-specific values (like windows archi...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ENV/cache_variables_validation/CMakeLists.txt:215-218
Timestamp: 2025-06-17T21:44:57.056Z
Learning: In CMDEF_ENV cache variable validation tests, hardcoded platform-specific values (like Windows architecture "x86-64" and version "1") are intentional and used to validate expected cache variable values in the test environment, rather than testing dynamic detection capabilities which are covered by other test cases.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: in cmake's get_property and matches operations, explicit notfound checking is not needed for functio...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/TEST.cmake:248-253
Timestamp: 2025-07-15T21:03:21.495Z
Learning: In CMake's GET_PROPERTY and MATCHES operations, explicit NOTFOUND checking is not needed for functions like TEST_CHECK_TARGET_PROPERTY_CONTAINS because CMake handles missing properties automatically during pattern matching operations.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: for cmake test cases that verify error conditions (like missing required parameters), use test_run_a...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:14-17
Timestamp: 2025-06-08T19:13:14.984Z
Learning: For CMake test cases that verify error conditions (like missing required parameters), use TEST_RUN_AND_CHECK_OUTPUT or similar test harness macros instead of directly calling the function being tested. Direct calls will abort CMake without proper test assertion, while test harnesses can capture and validate expected error messages.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: in the cmdef_install system, export destination paths intentionally contain double slashes like `${c...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/install_related/include_dirs/CMakeLists.txt:0-0
Timestamp: 2025-06-16T19:47:01.662Z
Learning: In the CMDEF_INSTALL system, export destination paths intentionally contain double slashes like `${CMDEF_LIBRARY_INSTALL_DIR}/cmake//` - this is the actual format produced by the implementation, not a formatting error in tests.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: in cmdef test suites, individual error test cases (like missing_target_error) should not include tes...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_ADD_EXECUTABLE/test_cases/missing_target_error/CMakeLists.txt:11-12
Timestamp: 2025-06-16T19:45:39.391Z
Learning: In CMDEF test suites, individual error test cases (like missing_target_error) should NOT include TEST.cmake. They are designed to be executed by main test orchestrators that use TEST_RUN_AND_CHECK_OUTPUT to capture and validate the expected fatal errors. Individual test cases should simply trigger the error condition.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: in modern cmake, the if command automatically dereferences variables in conditions. for example, `if...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_INSTALL/test_cases/interface_lib/CMakeLists.txt:93-95
Timestamp: 2025-06-16T19:51:02.357Z
Learning: In modern CMake, the IF command automatically dereferences variables in conditions. For example, `IF(variable_name MATCHES pattern)` works correctly without needing explicit variable expansion syntax like `IF("${variable_name}" MATCHES pattern)`. The explicit syntax is not required for basic variable dereferencing in IF conditions.
Applied to files:
test/cache_var.cmaketest/TEST.cmake
📚 Learning: cmake does not have an `unset_property` command. the only way to clear a property in cmake is using ...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/cache_var.cmake:77-79
Timestamp: 2025-06-16T20:53:31.710Z
Learning: CMake does not have an `UNSET_PROPERTY` command. The only way to clear a property in CMake is using `SET_PROPERTY(GLOBAL PROPERTY <prop>)` without a value, which sets the property to an empty value rather than removing it completely.
Applied to files:
test/cache_var.cmake
📚 Learning: cmake does not have an `unset_property` command. the only way to clear a property in cmake is using ...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/cache_var.cmake:77-79
Timestamp: 2025-06-16T20:53:31.710Z
Learning: CMake does not have an `UNSET_PROPERTY` command. The only way to clear a property in CMake is using `SET_PROPERTY(GLOBAL PROPERTY <prop>)` without a value. There is an `UNSET()` command but it only works for variables, not properties.
Applied to files:
test/cache_var.cmake
📚 Learning: the cmdef_package system creates a custom target named "package" (uppercase) in multiconfig builds, ...
Learnt from: koudis
PR: cmakelib/cmakelib-component-cmdef#11
File: test/CMDEF_PACKAGE/package_test_macros.cmake:92-96
Timestamp: 2025-06-16T20:53:43.698Z
Learning: The CMDEF_PACKAGE system creates a custom target named "PACKAGE" (uppercase) in multiconfig builds, which should not exist in single-config builds. This is different from CMake's built-in CPack "package" target (lowercase).
Applied to files:
test/TEST.cmake
🔇 Additional comments (5)
test/TEST.cmake (2)
8-10: Good defensive programming with directory validation.The directory existence check provides clear error messaging and prevents cryptic include failures. If
FIND_PACKAGE(CMLIB REQUIRED)is adopted, this check may become redundant but doesn't hurt to keep for extra safety.
12-12: Ensure external TEST.cmake implements all required test macrosI’ve confirmed that the codebase still uses the full suite of test macros across many test cases, for example:
• TEST_RUN
• TEST_RUN_AND_CHECK_OUTPUT
• TEST_CHECK_TARGET_PROPERTY
• TEST_CHECK_TARGET_INCLUDE_DIRECTORIES_CONTAINS
• TEST_CHECK_TARGET_LACKS_PROPERTY
• TEST_CHECK_TARGET_PROPERTY_CONTAINS
…among others.Please verify that the external
${CMLIB_DIR}/test/TEST.cmakeyou’re including at line 12 provides definitions for all these macros; missing any will break the existing test suite.test/cache_var.cmake (3)
6-6: Good use of REQUIRED flag for explicit dependency management.This correctly uses
FIND_PACKAGE(CMLIB REQUIRED)which provides better error messages and makes the dependency explicit, unlike the optional approach in TEST.cmake.
8-10: Defensive programming with directory validation.The directory existence check is good defensive programming. Since
FIND_PACKAGE(CMLIB REQUIRED)is used, this check may be redundant but provides extra safety and clear error messaging.
12-12: Ensure External Cache Variable Macros Maintain Safety GuaranteesThe
INCLUDE("${CMLIB_DIR}/test/cache_var.cmake")at line 12 correctly replaces the local cache‐variable routines with those bundled in CMLIB. We’ve confirmed that across the test suite the three macros—CACHE_VAR_FORCE_SET,CACHE_VAR_FORCE_UNSET, andCACHE_VAR_RESTORE—are invoked repeatedly without error, which demonstrates they are available and in use.Please manually verify in the CMLIB source that:
- Each macro is defined (via
macro(), notfunction()) to ensure the internal “first‐call only” capture of the original cache state.- Repeated invocations do not overwrite or leak the saved state.
- Restoration reliably returns to the pre-test value.
Once you’ve confirmed these definitions and safety guarantees in the CMLIB version, this inclusion can be approved without further changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit