Add enable_testing() call into test\CMakeLists.txt#2590
Add enable_testing() call into test\CMakeLists.txt#2590SergeyKopienko wants to merge 4 commits intomainfrom
enable_testing() call into test\CMakeLists.txt#2590Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a call to enable_testing() in the test/CMakeLists.txt file to properly enable CTest functionality for tests defined in subdirectories. The change is necessary because the kt subdirectory is added before enable_testing() was previously called, and that subdirectory contains add_test() calls that require testing to be enabled first.
Changes:
- Added
enable_testing()call intest/CMakeLists.txtafter theadd_subdirectory(kt)line to ensure tests in subdirectories are properly registered with CTest.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # See https://llvm.org/LICENSE.txt for license information. | ||
| # | ||
| ##===----------------------------------------------------------------------===## | ||
| enable_testing() |
There was a problem hiding this comment.
This call to enable_testing() is redundant. The root CMakeLists.txt already calls enable_testing() at line 357 before adding the test subdirectory via add_subdirectory(test) at line 358. Since enable_testing() propagates to all subdirectories, tests in test/kt/ and other subdirectories are already enabled by the root-level call. While this redundant call is harmless (enable_testing() is idempotent), it doesn't provide any additional functionality.
| enable_testing() |
There was a problem hiding this comment.
Already existing call of enable_testing() at
Line 357 in 7500e3d
There was a problem hiding this comment.
Let's then remove enable_testing from the root directory and keep it here.
I think it will be closer to what enable_testing expects according to its documentation:
This command should be invoked in the top-level source directory because ctest(1) expects to find a test file in the top-level build directory.
The cmake puts executables into test subdirectory.
There was a problem hiding this comment.
Sorry, I got confused by this cmake note.
Indeed, our cmake will place tests inside test directory, but I now think that the right way is to place it inside the top-level CMakeLists after investigating the sources of other different projects, e.g. see oneTBB, OpenCV or JSON. Perhaps, we should change where add_test should put the files, or there is a bug in VS2026 and we should do it only for this configuration. Let me think about it...
… from the root directory.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR adds a call to
enable_testing()in thetest/CMakeLists.txtfile to properly enable CTest functionality for tests defined in subdirectories.For additional details please read: