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

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

Hi, Sergey!
Thanks for the patch!
I'll proceed with a review within the original letter.

I look forward to applying these changes!
I hope we can use the full power of CTest. It will be nice to configure
heavy-memory tests to avoid running them together in parallel and
hanging the system:).

Please, consider my comments below.

On 12.03.24, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> The patch replaces the main test runner `prove(1)` with CTest,
> see [1] and [2].
> 
> Build and test steps looks the same as before:
> 
> $ 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 functional.
> 
> One could use `ctest` tool:
> 
> $ ctest --print-labels
> Test project <snipped>
> All Labels:
>   LuaJIT
>   PUC-Rio-Lua-5.1
>   lua-Harness
>   tarantool
>   tarantool-c
> $ ctest                             # Run all tests.
> $ ctest -L tarantool                # Run tests in tarantool-tests dir.
> $ ctest -L tarantool-c              # Run tests in tarantool-c-tests dir.
> $ ctest -L lua-Harness              # Run tests in lua-Harness-tests dir.
> $ ctest -L luajit                   # Run tests in LuaJIT-tests dir.
> $ ctest -L PUC-Rio-Lua-5.1          # Run tests in PUC-Rio-Lua-5.1-tests dir.

Please, remove the excess amount of spaces (9) in the formatting to make
the commit message fits 72 symbols.

Same for the paragraphs below.

> $ ctest --rerun-fail
> $ ctest --output-on-failure
> 
> 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 it is not possible to attach targets to the automatically
> generated `test` target. It means that target `test` is now executing
> `LuaJIT-test`, but not `LuaJIT-lint`.

Is it possible to introduce a new target like test-all to join them?

> 
> 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

Typo: s/Lua/Lua,/

> future we could change these test runners to specify tests from outside,

Typo: s/future/future,/

> and after this we could run tests separately by CTest in these test

Typo: s/this/this,/

> 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 workarounded by introducing a special test

Typo: s/workarounded/is workaraounded/

> for each testsuite that executes a target that builds test dependencies.
> 
> 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
> ---
> Changes v3:
> - rebased to master
> - applied fixes suggested by Igor Munkin
> 
> PR: https://github.com/tarantool/tarantool/pull/9286
> Branch: https://github.com/tarantool/luajit/tree/ligurio/enable_test_target
> 
>  .gitignore                                |  2 +
>  CMakeLists.txt                            | 11 ++++
>  test/CMakeLists.txt                       | 78 +++++++++++++++--------
>  test/LuaJIT-tests/CMakeLists.txt          | 40 +++++++-----
>  test/LuaJIT-tests/src/CMakeLists.txt      |  2 +-
>  test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++++---
>  test/lua-Harness-tests/CMakeLists.txt     | 48 ++++++++------
>  test/tarantool-c-tests/CMakeLists.txt     | 56 ++++++++--------
>  test/tarantool-tests/CMakeLists.txt       | 62 ++++++++----------
>  9 files changed, 194 insertions(+), 130 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..6b2f855f 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
> +# LuaJIT tests.
> +set(LUAJIT_TEST_DEPS "luajit-main" CACHE STRING
> +  "A list of target dependencies to be used for tests."
> +)

Why do we use luajit-main as a target dependency for tests?
Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
in tarantool-c-tests)?

> +
>  # FIXME: If LuaJIT is used in parent project, provide an option
>  # to pass Lua code to be run before tests are started.
>  # XXX: Attentive reader might point to LUA_INIT environment
> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>    "Lua code need to be run before tests are started."
>  )
>  
> +# XXX: Enable testing via CTest. Since CTest expects to find a
> +# test file in the build directory root, this command should be in

Nit: I suggest to mentioning that this command generates "test" target.
Something like the following:
| # XXX: Enable testing via CTest. This generates `test` target.

> +# the source directory root (hence, in this CMakeLists.txt).
> +enable_testing()

Should we generate the `test` target only if the `LUAJIT_USE_TEST`
option is set, like it was done before (see relevant changes in the
<test/CMakeLists.txt>)?

>  add_subdirectory(test)
>  
>  # --- Misc rules ---------------------------------------------------------------
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 3ad5d15f..90aaead2 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -77,35 +77,63 @@ separate_arguments(LUAJIT_TEST_COMMAND)
>  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
>  include(AddTestLib)
>  
> +# TEST_FLAGS is used by CMake targets in test suites.
> +# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains CTest options

Nit: the comment line is wider than 66 symbols.

> +# and is used by `test` target.
> +set(TEST_FLAGS

Minor: Maybe it is better to name this variable as `CTEST_FLAGS`, to
avoid confusion that these flags are arguments for some internal test
runner?

Unfortunately, this flag does nothing for `make test` command.
Is there the way to fix it?
BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.

> +  --output-on-failure
> +  --schedule-random
> +  --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}

I suppose it will be good to add the `--timeout` option here too.
5 minutes as a timeout for the single test sounds more than enough.

> +)
> +if(CMAKE_VERBOSE_MAKEFILE)
> +  list(APPEND TEST_FLAGS --verbose)
> +endif()
> +
> +function(add_test_suite_target target)
> +  set(prefix ARG)
> +  set(noValues)
> +  set(singleValues LABEL)
> +  set(multiValues DEPENDS)
> +
> +  # FIXME: if we update to CMake >= 3.5, can remove this line.
> +  include(CMakeParseArguments)
> +  cmake_parse_arguments(${prefix}
> +                        "${noValues}"
> +                        "${singleValues}"
> +                        "${multiValues}"
> +                        ${ARGN})
> +
> +  set(_DEPS_TARGET ${target}-deps)
> +
> +  add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
> +
> +  add_test(${_DEPS_TARGET}

Should the NAME parameter be ${target} instead? Or, maybe this line
should be ommitted since it is also used for normal test targets with
several tests.

For now `ctest -N` output looks a little bit inconsistent:

| Test project /home/burii/reviews/luajit/ctest
|   Test   #1: LuaJIT-tests-deps
|   Test   #2: test/LuaJIT-tests
|   Test   #3: PUC-Rio-Lua-5.1-tests-deps
|   Test   #4: test/PUC-Rio-Lua-5.1
|   Test   #5: lua-Harness-tests-deps
|   Test   #6: test/lua-Harness-tests/000-sanity.t
|   ...
|   Test  #56: tarantool-c-tests-deps
|   Test  #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
|   ...
|   Test  #67: tarantool-tests-deps
|   Test  #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
|   ...

If tests like the following are needed to run tests, I suggest naming
them like `LuaJIT-tests-build` to avoid confusion.
|   Test   #1: LuaJIT-tests-deps

Also, as discussed with Sergey offline:
For now, we observe an error since dependencies (C libraries) for tests
are not built if we build LuaJIT and try to run ctest target like the
following (without using make for a specific test target):

| cmake . && make -j
| ctest -L tarantool-tests --verbose
| 68: /home/burii/reviews/luajit/ctest/src/luajit: .../test/tarantool-tests/arm64-ccall-fp-convention.test.lua:4: libfficcall.so: cannot open shared object file: No such file or directory

> +           ${CMAKE_COMMAND}
> +             --build ${CMAKE_BINARY_DIR}
> +             --target ${_DEPS_TARGET}
> +  )
> +
> +  message(STATUS "Add test suite ${ARG_LABEL}")

Note: If we don't run this function for multi-test targets after
applying the changes required above, this STATUS message should be
copied to the correponding CMakeLists.txt.

> +
> +  add_custom_target(${target}
> +    COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
> +    DEPENDS ${_DEPS_TARGET}
> +  )
> +
> +  unset(_DEPS_TARGET)

There is no need in `unset()` since we are using `function()` here,
which opens a new scope.

> +endfunction()
> +
>  add_subdirectory(LuaJIT-tests)
>  add_subdirectory(PUC-Rio-Lua-5.1-tests)
>  add_subdirectory(tarantool-c-tests)
>  add_subdirectory(tarantool-tests)
>  
> -add_custom_target(${PROJECT_NAME}-test DEPENDS
> -  LuaJIT-tests
> -  PUC-Rio-Lua-5.1-tests
> -  lua-Harness-tests
> -  tarantool-c-tests
> -  tarantool-tests
> +add_custom_target(${PROJECT_NAME}-test
> +  COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}

Why do we need to use custom command for this test?
IINM, all "subtargets" are used the same ${TEST_FLAGS}
See also Igor's comment about this test.

> +  DEPENDS tarantool-c-tests-deps
> +          tarantool-tests-deps
> +          lua-Harness-tests-deps
> +          PUC-Rio-Lua-5.1-tests-deps
> +          LuaJIT-tests-deps
>  )
> -
> -if(LUAJIT_USE_TEST)

Within this change the `test` target is always generated.
See the comment above.

> -  if(POLICY CMP0037)

<snipped>

> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index a0fb5440..1080987a 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -3,17 +3,13 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>  
>  add_subdirectory(src)
>  
> -add_custom_target(LuaJIT-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} LuaJIT-tests-libs
> -)
> -
>  make_lua_path(LUA_CPATH
>    PATHS
>    ${CMAKE_CURRENT_BINARY_DIR}/src/?${CMAKE_SHARED_LIBRARY_SUFFIX}
>  )
>  
>  set(LUAJIT_TESTS_ENV
> -  "LUA_CPATH=\"${LUA_CPATH}\""
> +  "LUA_CPATH=${LUA_CPATH}\;\;"
>  )

These semicolons look excessive, see also Igor's comment.

Side note: I suppose that double quotes are used to avoid escaping
various special symbols or spaces. But for now, all these special cases
of directory names are just not working, so I see nothing crucial in
removing these quotes.

>  
>  set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:")
> @@ -50,10 +46,11 @@ if(LUAJIT_USE_ASAN)
>    if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
>      LibRealPath(LIB_ASAN libasan.so)
>      # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`.
> -    # ";" will expand to " " as we want.
> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}")
> +    # ";" will expand to " " as we want after wrapping

This is not true. After these changes the environment variable
`LD_PRELOAD` is the following:
| LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
But it should be:
| LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
This is why this quotes is used.
For some reason, with the debug flags, paths are separated via \n:
| 2:  LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
| 2:  /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32

So, I got the following error, when I build LuaJIT with GCC and use the
flag `-DLUAJIT_ENABLE_ASAN`:
| AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
|   #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69

> +    # LUAJIT_TESTS_ENV by double quotes.
> +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN};${LIB_STDCPP})
>    else()
> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}")
> +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP})
>    endif()
>  endif()
>  
> @@ -64,12 +61,25 @@ if(LUAJIT_NO_UNWIND)

<snipped>

> +set(TEST_SUITE_NAME "LuaJIT-tests")
> +
> +# XXX: The call produces both test and target
> +# <LuaJIT-tests-deps> as a side effect.

Nit: It looks like the <LuaJIT-tests-deps> quotation may be placed at
the previous line, fitting 66 symbols of the comment width.

> +add_test_suite_target(LuaJIT-tests
> +  LABEL ${TEST_SUITE_NAME}
> +  DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs

Why don't use `${LUAJIT_TEST_BINARY}` here?

> +)
> +
> +# 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"

Why don't use just LuaJIT-tests as a test name here?
The same question to the tests below.

> +  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
> +  LABELS ${TEST_SUITE_NAME}
> +  ENVIRONMENT "${LUAJIT_TESTS_ENV}"
> +  DEPENDS LuaJIT-tests-deps
> +)
> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
> index 2f90da86..f0e639e1 100644
> --- a/test/LuaJIT-tests/src/CMakeLists.txt
> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
> @@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c)
>  enable_language(CXX)
>  AddTestLib(libcpptest libcpptest.cpp)
>  
> -add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS})
> +add_custom_target(LuaJIT-tests-deps-libs DEPENDS ${TESTLIBS})

Why do you prefer to rename this target? I see no conflict between this
name and the previous version.

> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> index 98277f9a..af89bfe5 100644
> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> @@ -34,17 +34,26 @@ add_subdirectory(libs)
>  # But, unfortunately, <ltests.c> depends on specific PUC-Rio
>  # Lua 5.1 internal headers and should be adapted for LuaJIT.
>  
> -add_custom_target(PUC-Rio-Lua-5.1-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} PUC-Rio-Lua-5.1-tests-prepare
> +set(TEST_SUITE_NAME "PUC-Rio-Lua-5.1-tests")
> +
> +# XXX: The call produces both test and target
> +# <PUC-Rio-Lua-5.1-tests-deps> as a side effect.
> +add_test_suite_target(PUC-Rio-Lua-5.1-tests
> +  LABEL ${TEST_SUITE_NAME}
> +  DEPENDS ${LUAJIT_TEST_DEPS} PUC-Rio-Lua-5.1-tests-prepare

Why don't use `${LUAJIT_TEST_BINARY}` here?
Here and below.

>  )
>  
> -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"
> +  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" PROPERTIES
> +  ENVIRONMENT "LUA_PATH=${LUA_PATH}\;\;"

Typo: These semicolons look excessive.

> +  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..d454ba41 100644
> --- a/test/lua-Harness-tests/CMakeLists.txt
> +++ b/test/lua-Harness-tests/CMakeLists.txt
> @@ -4,12 +4,6 @@

<snipped>

> +# XXX: The call produces both test and target
> +# <lua-Harness-tests-deps> as a side effect.
> +add_test_suite_target(lua-Harness-tests
> +  LABEL ${TEST_SUITE_NAME}
> +  DEPENDS ${LUAJIT_TEST_DEPS}
>  )
>  
> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.t")
> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME)
> +  # FIXME: By default GLOB lists directories.

Typo: s/By default/By default,/

> +  # 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})
> +    continue()
> +  endif (${test_name} STREQUAL ${TEST_SUITE_NAME})

Typo: s/endif (/endif(/

> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}")

I suppose, that general part "test/" may be ommited from the title.

> +  add_test(NAME ${test_title}
> +           COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
> +           WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}

Minor: Looks like misindent.

> +  )
> +  set_tests_properties(${test_title} PROPERTIES
> +    ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"

Minor: Semicolon looks excess here.

> +    LABELS ${TEST_SUITE_NAME}
> +    DEPENDS lua-Harness-tests-deps
> +  )
> +endforeach()
> +
>  # vim: expandtab tabstop=2 shiftwidth=2
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index 4b1d8832..2de4a0c8 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -1,15 +1,4 @@

<snipped>

>  # Build libtest.
>  
> @@ -49,20 +38,35 @@ foreach(test_source ${tests})
>    LIST(APPEND TESTS_COMPILED ${exe})
>  endforeach()
>  
> -add_custom_target(tarantool-c-tests
> -  DEPENDS libluajit libtest ${TESTS_COMPILED}
> -)
> +set(TEST_SUITE_NAME "tarantool-c-tests")
>  
> -add_custom_command(TARGET tarantool-c-tests
> -  COMMENT "Running Tarantool C tests"
> -  COMMAND
> -  ${PROVE}
> -    ${CMAKE_CURRENT_BINARY_DIR}
> -    --ext ${C_TEST_SUFFIX}
> -    --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> -    # Report any TAP parse errors, if any, since test module is
> -    # maintained by us.
> -    --parse
> -    ${C_TEST_FLAGS}
> -  WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +# XXX: The call produces both test and target
> +# <LuaJIT-tests-deps> as a side effect.

Typo: s/LuaJIT-tests-deps/tarantool-c-tests-deps/

> +add_test_suite_target(tarantool-c-tests
> +  LABEL ${TEST_SUITE_NAME}
> +  DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED}
>  )
> +
> +# XXX: Note that we cannot glob generated files in CMake, see
> +# https://stackoverflow.com/questions/44076307/cmake-globbing-generated-files

Nit: Missed dot at the end of the sentence.

> +# To workaround this, we glob source files and generated tests
> +# with paths to executable binaries.
> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")

Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".

You may use the cycle above, which adds build targets.

> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME_WE)
> +  # 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})
> +    continue()
> +  endif (${test_name} STREQUAL ${TEST_SUITE_NAME})

Typo: s/endif (/endif(/

> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")

I suppose, that general part "test/" may be ommited from the title.

> +  add_test(NAME ${test_title}
> +    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}
> +    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +  )
> +  set_tests_properties(${test_title} PROPERTIES
> +    LABELS ${TEST_SUITE_NAME}
> +    DEPENDS tarantool-c-tests-deps
> +  )
> +endforeach()
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index e6d12984..bb41e53b 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-deps-libs

Why do you prefer to rename this target? I see no conflict between this
name and the previous version.

>    DEPENDS ${LUAJIT_TEST_BINARY}
>  )
>  
>  macro(BuildTestCLib lib sources)
>    AddTestLib(${lib} ${sources})
> -  add_dependencies(tarantool-tests ${lib})
> +  add_dependencies(tarantool-tests-deps-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
> @@ -60,21 +54,14 @@ make_lua_path(LUA_PATH
>      ${PROJECT_SOURCE_DIR}/tools/?.lua
>      ${LUAJIT_SOURCE_DIR}/?.lua
>      ${LUAJIT_BINARY_DIR}/?.lua
> +    ${PROJECT_BINARY_DIR}/src/?.lua

This is the same directory as a ${LUAJIT_SOURCE_DIR}/?.lua.

>  )
> +
>  # Update LUA_CPATH with the library paths collected within
>  # <BuildTestLib> macro.
>  make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS})
>  

<snipped>

> +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
> +  LABEL ${TEST_SUITE_NAME}
> +  DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-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}")

Why do we need GLOB_RECURSE here, instead of GLOB?

> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME)
> +  set(test_title "test/tarantool-tests/${test_name}")

I suppose, that general part "test/" may be ommited from the title.

> +  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
> 

-- 
Best regards,
Sergey Kaplun