Re: [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 25093 bytes --]

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<sergeyb@tarantool.org>
>>
>> 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 <ffi.load>. 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 <snipped>
>> 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
> <snipped>
>
>> 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.
> <snipped>
>
>> @@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
> <snipped>
>
>> +if (LUAJIT_USE_TEST)
> Typo: s/if (/if(/
Fixed.
> <snipped>
>
>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>> index 62478441..06a1d365 100644
>> --- a/test/CMakeLists.txt
>> +++ b/test/CMakeLists.txt
> <snipped>
>
>> +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 <tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c>,
>> +  # 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
> <snipped>
>
>> -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.
> <snipped>
>
>> 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
> <snipped>
>
>> +# 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}
>>   )
> <snipped>
>
>> 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
> <snipped>
>
>> -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
> <snipped>
>
>> +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()
> <snipped>
>
>> 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
> <snipped>
>
>> @@ -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)
> <snipped>
>
>> @@ -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}
>> -)
>> -
> <snipped>
>
>> 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 @@
> <snipped>
>
>> -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
>>   # <BuildTestLib> 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
>> +# <tarantool-tests-deps> 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
>>

[-- Attachment #2: Type: text/html, Size: 40115 bytes --]