Sergey, On 3/26/24 14:03, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > Please consider some minor comments and nits below. > > On 26.03.24, Sergey Bronnikov wrote: >> From: Sergey Bronnikov >> >> The patch replaces the main test runner `prove(1)` with CTest, > Since we are dropping the usage of `prove`, we should reformulate > related comments about it, shouldn't we? Updated: --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -79,15 +79,15 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")    # since some programs sanitize the environment before they start    # child processes. Specifically, environment variables starting    # with DYLD_ and LD_ are unset for child process started by -  # other programs (like /usr/bin/prove --exec using for launching -  # test suite). For more info, see the docs[2] below. +  # other programs (like `ctest` using for launching tests). +  # For more info, see the docs[2] below.    #    # These environment variables are used by FFI machinery to find    # the proper shared library, hence we can still tweak testing    # environment before calling . However, the value    # can't be passed via the standard environment variable, so we -  # use env call in prove's --exec flag value to get around SIP -  # magic tricks. +  # use ENVIRONMENT property in `set_tests_properties` to get +  # around SIP magic tricks.    #    # [1]: https://support.apple.com/en-us/HT204899    # [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html > Also, maybe the > LUA_TEST_ENV_MORE can be removed? I'm not sure. I suspect the same trick is required with CTest on macOS too. >> see [1] and [2]. >> >> Build and test steps looks the same as before: > Typo: s/looks/look/ Fixed. >> $ cmake -S . -B build >> $ cmake --build build --parallel >> $ cmake --build build --target [LuaJIT-test, test] >> >> CMake targets lua-Harness-tests, LuaJIT-tests, >> PUC-Rio-Lua-5.1-tests, tarantool-tests and tarantool-c-tests >> are still operating. >> >> CMake custom target `test` was replaced by a target with the same > Typo: s/CMake/The CMake/ Fixed. >> name that is automatically generated by CMake. It is not possible >> to attach targets to this automatically generated `test` target. >> It means that target `test` is now executing `LuaJIT-test`, >> but not `LuaJIT-lint`. To mitigate this a new target > Typo: s/To mitigate this/To mitigate this,/ Fixed. >> `LuaJIT-check-all` is introduced, it has the same behaviour like > Typo: s/introduced, it/introduced. It/ > Typo: s/like/as/ Fixed. >> an old `test` target: it runs all functional tests and linters. >> >> One could use `ctest` tool: > Typo: s/use `ctest` tool/use the `ctest` tool/ Fixed. >> $ ctest --print-labels >> Test project >> All Labels: >> LuaJIT-tests >> PUC-Rio-Lua-5.1-tests >> lua-Harness-tests >> tarantool-c-tests >> tarantool-tests >> $ ctest # Run all tests. >> $ ctest -L tarantool-tests # Run tests by label. >> $ ctest -L tarantool-c-tests # Ditto. >> $ ctest -L lua-Harness-tests # Ditto. >> $ ctest -L LuaJIT-tests # Ditto. >> $ ctest -L PUC-Rio-Lua-5.1-tests # Ditto. >> $ ctest --rerun-fail # Run only the tests >> # that failed previously. >> $ ctest --verbose # Print details to stdout. >> $ ctest --output-on-failure # Print details to stdout >> # on test failure only. >> >> The benefits are: >> >> - less external dependencies, `prove(1)` is gone, `ctest` >> is a part of CMake >> - running tests by test title >> - extended test running options in comparison to prove(1) >> - unified test output (finally!) >> >> Note that the testsuites in `test/LuaJIT-tests` and >> `test/PUC-Rio-Lua-5.1` directories have their own test runners >> written in Lua, and currently CTest runs these testsuites >> as single tests. In the future, we could change these test runners >> to specify tests from outside, and after this, we could run tests >> separately by CTest in these test suites. >> >> Note that it is not possible to add dependencies to `add_test()` >> in CMake, see [3]. CMake 3.7 introduces FIXTURES_REQUIRED [4] and >> FIXTURES_SETUP [5], but these test properties cannot be used - >> used CMake version is lower. This is workarounded by an introduced > Typo: s/used CMake version/the currently supported CMake version/ > Typo: s/an introduced/the introduced/ Fixed. >> macro `add_test_suite_target` that adds a CMake-test for each >> testsuite that executes a target that builds test dependencies. >> >> The commit introduce a CMake option LUAJIT_TEST_DEPS that set > Typo: s/introduce/introduces/ > Typo: s/a CMake option/the CMake option/ > Typo: s/set/sets/ Fixed. >> dependencies to be used for running and building LuaJIT tests. >> >> 1.https://cmake.org/cmake/help/latest/manual/ctest.1.html >> 2.https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html >> 3.https://gitlab.kitware.com/cmake/cmake/-/issues/8774 >> 4.https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html >> 5.https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html >> --- >> .gitignore | 2 + >> CMakeLists.txt | 14 +++ >> test/CMakeLists.txt | 107 +++++++++++++++++----- >> test/LuaJIT-tests/CMakeLists.txt | 33 ++++--- >> test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++-- >> test/lua-Harness-tests/CMakeLists.txt | 50 +++++----- >> test/tarantool-c-tests/CMakeLists.txt | 58 ++++++------ >> test/tarantool-tests/CMakeLists.txt | 68 +++++++------- >> 8 files changed, 226 insertions(+), 131 deletions(-) >> >> diff --git a/.gitignore b/.gitignore >> index dc5ea5fc..dd1f8888 100644 >> --- a/.gitignore >> +++ b/.gitignore > > >> diff --git a/CMakeLists.txt b/CMakeLists.txt >> index 7f5e2afb..96f85285 100644 >> --- a/CMakeLists.txt >> +++ b/CMakeLists.txt >> @@ -345,6 +345,13 @@ set(LUAJIT_TEST_BINARY ${LUAJIT_BINARY} CACHE STRING >> "Lua implementation to be used for tests. Default is 'luajit'." >> ) >> >> +# If LuaJIT is used in a parent project, provide an option >> +# to choose what dependencies to be used for running and building > Typo: s/to be used/to use/ or s/to be used/are used/ Fixed. >> +# LuaJIT tests. > > >> @@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR > > >> +if (LUAJIT_USE_TEST) > Typo: s/if (/if(/ Fixed. > > >> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt >> index 62478441..06a1d365 100644 >> --- a/test/CMakeLists.txt >> +++ b/test/CMakeLists.txt > > >> +set(CTEST_FLAGS >> + --output-on-failure >> + --schedule-random >> + # Timeout in seconds. Most of LuaJIT tests are fast, > Typo: s/Most of LuaJIT tests are fast, however,/Most LuaJIT tests are fast. However,/ > >> + # however, some tests can hang or execute infinite loop. > Typo: s/infinite loop/an infinite loop/ Fixed. >> + # See , >> + # commit caa99865c206 ("test: rewrite sysprof test using managed execution"). >> + # Moreover, tests for instrumented LuaJIT requires more time. > Typo: s/requires/require/ > Fixed. >> + # 30 minutes is a sane timeout. >> + --timeout 1800 >> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL} >> +) >> +if(CMAKE_VERBOSE_MAKEFILE) >> + list(APPEND CTEST_FLAGS --verbose) >> +endif() >> + >> +# It is not possible to add dependencies to `add_test()` >> +# in CMake, see [1]. CMake 3.7 introduces FIXTURES_REQUIRED [2] >> +# and FIXTURES_SETUP [3], but these test properties cannot be >> +# used - it is unsupported in a current CMake version. > Typo: s/it is unsupported/this feature is unsupported/ > Typo: s/a current/the current/ Fixed. >> +# To workaround this a function `add_test_suite_target` is > Typo: s/To workaround this a/To workaround this, the/ > Typo: s/is introduced, it/is introduced. It/ Fixed. >> +# introduced, it adds a CMake target that builds testsuite's >> +# prerequisites and CMake test that executes that target. > Typo: s/CMake test/a CMake test/ Fixed. >> +# >> +# 1.https://gitlab.kitware.com/cmake/cmake/-/issues/8774 >> +# 2.https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html >> +# 3.https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html > > >> -add_custom_target(${PROJECT_NAME}-test DEPENDS >> - LuaJIT-tests >> - PUC-Rio-Lua-5.1-tests >> - lua-Harness-tests >> - tarantool-c-tests >> - tarantool-tests >> +# Each testsuite has it's own CMake target, but combining these > Typo: s/it's/its/ Fixed. >> +# target to a single one is not desired, because each target > Typo: s/these target to/these targets into/ > Typo: s/desired,/desired/ > > Fixed. >> +# runs it's own ctest command, that each time enumerates tests > Typo: s/command, that/command, which/ > Typo? s/ctest/CTest/ replaced with `ctest` >> +# from zero and prints test summary at the end of test run. > Typo: s/prints test summary/prints the test summary/ > Typo: s/test run/ the test run/ Fixed. >> +# For common target this output looks inconvenient. > Typo: s/For common target/For a common target,/ > > Fixed. >> +# Therefore target below executes a single instance of ctest > Typo: s/Therefore target/Therefore, the target/ > Typo: s/instance of ctest/instance of the CTest/ > > Fixed. >> +# command that runs all generated CMake tests. > > >> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt >> index 003f8de6..6ad05aa6 100644 >> --- a/test/LuaJIT-tests/CMakeLists.txt >> +++ b/test/LuaJIT-tests/CMakeLists.txt > > >> +# The test suite has its own test runner >> +# (test/LuaJIT-tests/test.lua), it is not possible to run these >> +# tests one-by-one by CTest. >> +add_test(NAME "test/LuaJIT-tests" > Maybe it is better to use > | add_test(NAME "test/${TEST_SUITE_NAME}" > for consistency for all suites? > Also, it avoids confusing (for me, this is some kind of typo opposed to > the `add_test_suite_target()` command above). > Feel free to ignore, since it is subjectivity. Updated: diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index 6ad05aa6..b8e4dfc4 100644 --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -64,12 +64,13 @@ add_test_suite_target(LuaJIT-tests  # The test suite has its own test runner  # (test/LuaJIT-tests/test.lua), it is not possible to run these  # tests one-by-one by CTest. -add_test(NAME "test/LuaJIT-tests" +set(test_title "test/${TEST_SUITE_NAME}") +add_test(NAME "${test_title}"    COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua                                   +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}  ) -set_tests_properties("test/LuaJIT-tests" PROPERTIES +set_tests_properties("${test_title}" PROPERTIES    LABELS ${TEST_SUITE_NAME}    ENVIRONMENT "${LUAJIT_TESTS_ENV}"    DEPENDS LuaJIT-tests-deps diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt index a26e38d4..0942c62d 100644 --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt @@ -46,11 +46,12 @@ add_test_suite_target(PUC-Rio-Lua-5.1-tests  # The test suite has its own test runner  # (test/PUC-Rio-Lua-5.1-tests/all.lua), it is not possible  # to run these tests one-by-one by CTest. -add_test(NAME "test/PUC-Rio-Lua-5.1-tests" +set(test_title "test/${TEST_SUITE_NAME}") +add_test(NAME "${test_title}"    COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}  ) -set_tests_properties("test/PUC-Rio-Lua-5.1-tests" PROPERTIES +set_tests_properties("${test_title}" PROPERTIES    ENVIRONMENT "LUA_PATH=${LUA_PATH}"    LABELS ${TEST_SUITE_NAME}    DEPENDS PUC-Rio-Lua-5.1-tests-deps >> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua >> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA} >> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} >> ) > > >> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> index 98277f9a..a26e38d4 100644 >> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt > > >> -add_custom_command(TARGET PUC-Rio-Lua-5.1-tests >> - COMMENT "Running PUC-Rio Lua 5.1 tests" >> - COMMAND >> - env >> - LUA_PATH="${LUA_PATH}" >> - ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua >> +# The test suite has its own test runner >> +# (test/PUC-Rio-Lua-5.1-tests/all.lua), it is not possible >> +# to run these tests one-by-one by CTest. >> +add_test(NAME "test/PUC-Rio-Lua-5.1-tests" >> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua >> WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> ) >> +set_tests_properties("test/PUC-Rio-Lua-5.1-tests" PROPERTIES >> + ENVIRONMENT "LUA_PATH=${LUA_PATH}" > Nit: Do we need to move the " back to the start of the declaration? > If it is possible, please use LUA_PATH="${LUA_PATH}" instead. it breaks suite:     Start 2: test/PUC-Rio-Lua-5.1-tests 2/2 Test #2: test/PUC-Rio-Lua-5.1-tests .......***Failed    0.02 sec /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc32/src/luajit: module '/tmp/lua_joUdrU' not found:         no field package.preload['/tmp/lua_joUdrU']         no file '"/tmp/lua_joUdrU'         no file './build/gc64/test/tarantool-tests/lj-1087-vm-handler-call//tmp/lua_joUdrU.so' stack traceback:         [C]: at 0x63fdd720d320         [C]: at 0x63fdd7145e30 /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc32/src/luajit: ...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lu a:66: assertion failed! stack traceback:         [C]: in function 'assert' ...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lua:66: in function 'RUN' ...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lua:77: in function '_dofile' ...ol/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/all.lua:74: in main chunk         [C]: at 0x5f99fec32e30 >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS PUC-Rio-Lua-5.1-tests-deps >> +) >> >> # vim: expandtab tabstop=2 shiftwidth=2 >> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt >> index f748a8fd..6f7b5697 100644 >> --- a/test/lua-Harness-tests/CMakeLists.txt >> +++ b/test/lua-Harness-tests/CMakeLists.txt > > >> +foreach(test_path ${tests}) >> + get_filename_component(test_name ${test_path} NAME) >> + # FIXME: By default, GLOB lists directories. >> + # Directories are omitted in the result if LIST_DIRECTORIES >> + # is set to false. New in version CMake 3.3. >> + if (${test_name} STREQUAL ${TEST_SUITE_NAME}) > Typo: s/if (/if(/ > Fixed. >> + continue() >> + endif() > > >> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt >> index 4b1d8832..5affbb9e 100644 >> --- a/test/tarantool-c-tests/CMakeLists.txt >> +++ b/test/tarantool-c-tests/CMakeLists.txt > > >> @@ -30,6 +19,23 @@ AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS}) >> # test binary can be run from any directory. >> AppendFlags(TESTS_C_FLAGS "-D__LJ_TEST_DIR__='\"${CMAKE_CURRENT_SOURCE_DIR}\"'") >> >> +set(TEST_SUITE_NAME "tarantool-c-tests") >> + >> +# Proxy CMake target with all targets that builds C tests. > Typo: s/Proxy/The proxy/ > Typo: s/that builds/that build/ Fixed. >> +# This is needed because targets for each C test are generated >> +# at the same time with CMake tests, and all prerequisites must be > Typo: s/at the same time with/at the same time as/ > Typo: s/must be already exist/must already exist/ Fixed. >> +# already exist at this moment. >> +add_custom_target(tarantool-c-tests-build) > > >> @@ -47,22 +53,16 @@ foreach(test_source ${tests}) >> ) >> target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY}) >> LIST(APPEND TESTS_COMPILED ${exe}) > Should this line be removed since TESTS_COMPILED is no longer in use? Sure, thanks! >> -endforeach() >> + add_dependencies(tarantool-c-tests-build ${exe}) >> >> -add_custom_target(tarantool-c-tests >> - DEPENDS libluajit libtest ${TESTS_COMPILED} >> -) >> - > > >> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt >> index 35bcc5ef..9086a890 100644 >> --- a/test/tarantool-tests/CMakeLists.txt >> +++ b/test/tarantool-tests/CMakeLists.txt >> @@ -4,19 +4,13 @@ > > >> -add_custom_target(tarantool-tests >> +add_custom_target(tarantool-tests-libs >> DEPENDS ${LUAJIT_TEST_BINARY} > Should it depend on libluajit instead? > We don't need the LuaJIT binary to link with the LuaJIT library. Fixed. --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -5,7 +5,7 @@  cmake_minimum_required(VERSION 3.1 FATAL_ERROR)  add_custom_target(tarantool-tests-libs -  DEPENDS ${LUAJIT_TEST_BINARY} +  DEPENDS libluajit  )  macro(BuildTestCLib lib sources) >> ) >> >> macro(BuildTestCLib lib sources) >> AddTestLib(${lib} ${sources}) >> - add_dependencies(tarantool-tests ${lib}) >> + add_dependencies(tarantool-tests-libs ${lib}) >> # Add the directory where the lib is built to the list with >> # entries for LUA_CPATH environment variable, so LuaJIT can find >> # and load it. See the comment about extending the list in the >> @@ -61,20 +55,12 @@ make_lua_path(LUA_PATH >> ${LUAJIT_SOURCE_DIR}/?.lua >> ${LUAJIT_BINARY_DIR}/?.lua >> ) >> + >> # Update LUA_CPATH with the library paths collected within >> # macro. >> make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS}) >> >> set(LUA_TEST_SUFFIX .test.lua) >> -set(LUA_TEST_FLAGS --failures --shuffle) >> -set(LUA_TEST_ENV >> - "LUA_PATH=\"${LUA_PATH}\"" >> - "LUA_CPATH=\"${LUA_CPATH}\"" >> -) >> - >> -if(CMAKE_VERBOSE_MAKEFILE) >> - list(APPEND LUA_TEST_FLAGS --verbose) >> -endif() >> >> # XXX: Since the auxiliary libraries are built as a dynamically >> # loaded modules on MacOS instead of shared libraries as it is >> @@ -118,29 +104,37 @@ endif() >> # process. >> # See also:https://github.com/tarantool/tarantool/issues/9656. >> if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") >> - # FIXME: After using CTest instead of `prove` we should set this >> - # environment variable only for the corresponding tests to avoid >> - # warnings from the libc and etc. >> + # FIXME: test_title test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua > Typo: No dot at the end of the sentence. > Nit: Comment line width is more than 66 symbols. Fixed. > Also, I didn't get the idea of the first sentence. > Also, will we add an additional patch that fixes this comment in this > patch series (as an additional commit) or will we do it later? I propose to address this later. We have an issue to set env variables specific for test, lets fix this in scope of that issue too. --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -118,9 +118,9 @@ endif()  # process.  # See also: https://github.com/tarantool/tarantool/issues/9656.  if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") -  # FIXME: After using CTest instead of `prove` we should set this -  # environment variable only for the corresponding tests to avoid -  # warnings from the libc and etc. +  # FIXME: We should set this environment variable only +  # for the corresponding tests to avoid warnings from +  # the GNU libc and other libc implementations.    LibRealPath(LIB_ASAN libasan.so)    list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN})  endif() >> + # With CTest we should set this environment variable only > Typo: s/With CTest/With CTest,/ Fixed. >> + # for the corresponding tests to avoid warnings from >> + # the GNU libc and other libc implementations. > Should this change be the part of the commit "test: fix > lj-802-panic-at-mcode-protfail GCC+ASan"? Fixed. >> LibRealPath(LIB_ASAN libasan.so) >> list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN}) >> endif() >> >> -# LUA_CPATH and LD_LIBRARY_PATH variables and also dependencies >> -# list with libraries are set in scope of BuildTestLib macro. >> -add_custom_command(TARGET tarantool-tests >> - COMMENT "Running Tarantool tests" >> - COMMAND >> - # XXX: We can't move everything to the "inner" env, since there >> - # are some issues with escaping ';' for different shells. As >> - # a result LUA_PATH/LUA_CPATH variables are set via the "outer" >> - # env, since they are not stripped by SIP like LD_*/DYLD_* are. >> - env >> - ${LUA_TEST_ENV} >> - ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR} >> - --exec 'env ${LUA_TEST_ENV_MORE} ${LUAJIT_TEST_COMMAND}' >> - --ext ${LUA_TEST_SUFFIX} >> - --jobs ${CMAKE_BUILD_PARALLEL_LEVEL} >> - ${LUA_TEST_FLAGS} >> - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> +set(TEST_SUITE_NAME "tarantool-tests") >> + >> +# XXX: The call produces both test and target >> +# as a side effect. >> +add_test_suite_target(tarantool-tests >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs >> ) >> >> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list >> +# with dependencies are set in scope of BuildTestLib macro. > I prefer to use the old paraphrasing of this paragraph. > Otherwise, the reader may think: "What the heck is TESTLIBS?" --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -114,9 +114,9 @@ foreach(test_path ${tests})      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}    )    set_tests_properties(${test_title} PROPERTIES -    # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS -    # list with dependencies are set in scope of BuildTestLib -    # macro. +    # LUA_CPATH and LD_LIBRARY_PATH variables and also +    # dependencies list with libraries are set in scope of +    # BuildTestLib macro.      ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"      LABELS ${TEST_SUITE_NAME}      DEPENDS tarantool-tests-deps >> +set(TEST_ENV "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}") > Maybe it is better to use `list(APPEND` here for better readability? It was fine before patch for ctest support. Why we need to change this? > Or even don't use the additional variable that is used once and provide > these ENVIROMENT variables directly below. Done. (but for my taste previously was better and actually it is not related to ctest patch at all) --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -121,9 +121,6 @@ add_test_suite_target(tarantool-tests    DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs  ) -# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list -# with dependencies are set in scope of BuildTestLib macro. -set(TEST_ENV "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}")  file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")  foreach(test_path ${tests})    get_filename_component(test_name ${test_path} NAME) @@ -133,7 +130,10 @@ foreach(test_path ${tests})      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}    )    set_tests_properties(${test_title} PROPERTIES -    ENVIRONMENT "${TEST_ENV}" +    # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS +    # list with dependencies are set in scope of BuildTestLib +    # macro. +    ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"      LABELS ${TEST_SUITE_NAME}      DEPENDS tarantool-tests-deps    ) >> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") >> +foreach(test_path ${tests}) >> + get_filename_component(test_name ${test_path} NAME) >> + set(test_title "test/tarantool-tests/${test_name}") > Should it be "test/${TEST_SUITE_NAME}/${test_name}" by analogy with > other test suites? Fixed. >> + add_test(NAME ${test_title} >> + COMMAND ${LUAJIT_TEST_COMMAND} ${test_path} >> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> + ) >> + set_tests_properties(${test_title} PROPERTIES >> + ENVIRONMENT "${TEST_ENV}" >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS tarantool-tests-deps >> + ) >> +endforeach() >> -- >> 2.34.1 >>