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

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

Sergey,

thanks for review!

see my comments below

On 3/18/24 18:54, Sergey Kaplun wrote:
> 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.
  Fixed.

>
> Same for the paragraphs below.
Fixed.
>> $ 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?

Discussed target name in a private conversation and decided to introduce

"LuaJIT-check-all".

--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -140,3 +140,8 @@ add_custom_target(${PROJECT_NAME}-test
            PUC-Rio-Lua-5.1-tests-deps
            LuaJIT-tests-deps
  )
+
+add_custom_target(${PROJECT_NAME}-check-all
+  DEPENDS ${PROJECT_NAME}-test
+          ${PROJECT_NAME}-lint
+)

>
>> 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,/
>
Added comma.
>> future we could change these test runners to specify tests from outside,
> Typo: s/future/future,/
Added comma.
>
>> and after this we could run tests separately by CTest in these test
> Typo: s/this/this,/
Added comma.
>
>> 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/

Typo is in "workaraounded".


Fixed.

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

Added:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -369,9 +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
-# the source directory root (hence, in this CMakeLists.txt).
+# XXX: Enable testing via CTest. This automatically generates
+# `test` target. Since CTest expects to find a test file in
+# the build directory root, this command should be in the source
+# directory root (hence, in this CMakeLists.txt).
  enable_testing()
  add_subdirectory(test)


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

Added option LUAJIT_USE_TEST back:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -373,7 +373,9 @@ set(LUAJIT_TEST_INIT 
"${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
  # `test` target. Since CTest expects to find a test file in
  # the build directory root, this command should be in the source
  # directory root (hence, in this CMakeLists.txt).
-enable_testing()
+if (LUAJIT_USE_TEST)
+  enable_testing()
+endif()
  add_subdirectory(test)

  # --- Misc rules 
---------------------------------------------------------------


>>   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.
Already fixed.
>
>> +# 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?
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 899bf31a..ca298118 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -77,16 +77,16 @@ 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.
+# CTEST_FLAGS is used by CMake targets in test suites.
  # XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains
  # CTest options and is used by `test` target.
-set(TEST_FLAGS
+set(CTEST_FLAGS
    --output-on-failure
    --schedule-random
    --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
  )
  if(CMAKE_VERBOSE_MAKEFILE)
-  list(APPEND TEST_FLAGS --verbose)
+  list(APPEND CTEST_FLAGS --verbose)
  endif()

  function(add_test_suite_target target)
@@ -119,7 +119,7 @@ function(add_test_suite_target target)
    message(STATUS "Add test suite ${ARG_LABEL}")

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

@@ -133,7 +133,7 @@ add_subdirectory(tarantool-c-tests)
  add_subdirectory(tarantool-tests)

  add_custom_target(${PROJECT_NAME}-test
-  COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}
+  COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}
    DEPENDS tarantool-c-tests-deps
            tarantool-tests-deps
            lua-Harness-tests-deps
>
> Unfortunately, this flag does nothing for `make test` command.
> Is there the way to fix it?

Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that 
contains
CTest options and is used by `test` target. As a comment says.

BTW you can fix it by using CMake workflows and specify any CTest 
arguments you want in CMakePreset.json.

> BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
Sure, these are our own targets and we pass desired CTest options to it.
>> +  --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.
>
Added.
>> +)
>> +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?

Added.


> Or, maybe this line
> should be ommitted since it is also used for normal test targets with
> several tests.

CMake test for a target with dependencies must be generated,

otherwise dependencies will not be built.

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

AFAIR this naming was suggested by Igor.

What kind of confusion you observe? For me something like

`LuaJIT-tests-build`  much more confusing than "xxx-deps".

> |   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
>
it was fixed
>> +           ${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.
This macro executed one time for each test suite.
>
>> +
>> +  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.
Removed.
>
>> +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.

Answered to Igor.

because output will be ugly in that case,

just change as you suggest and run LuaJIT-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.
Generated when LUAJIT_USE_TEST is ON.
>
>> -  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.
already fixed and force-pushed
>>   
>>   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
>
Fixed.

Unfortunately there is a yet another test that will never worked in 
configuration GCC + ASAN.

Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to 
fix it, but as

it was never worked before we don't need to do it in scope of ctest support.


diff --git a/test/LuaJIT-tests/CMakeLists.txt 
b/test/LuaJIT-tests/CMakeLists.txt
index 0c85f15c..4c9cf901 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)
    # https://github.com/google/sanitizers/issues/934.
    macro(LibRealPath output lib)
      execute_process(
-      COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
+      COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
        OUTPUT_VARIABLE LIB_LINK
        OUTPUT_STRIP_TRAILING_WHITESPACE
+      TIMEOUT 5
+      RESULT_VARIABLE RES
      )
+    if (${LIB_LINK} STREQUAL ${lib})
+      message(FATAL_ERROR "library ${lib} is not found")
+    endif()
+    if (NOT RES EQUAL 0)
+      message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed")
+    endif()
+
      # Fortunately, we are not interested in macOS here, so we can
      # use realpath.
+    # Beware, builtin command returns exit code equal to 0,
+    # we cannot check is it failed or not.
      execute_process(
        COMMAND realpath ${LIB_LINK}
        OUTPUT_VARIABLE ${output}
        OUTPUT_STRIP_TRAILING_WHITESPACE
+      TIMEOUT 5
      )
    endmacro()
    LibRealPath(LIB_STDCPP libstdc++.so)
-  # XXX: GCC requires both. Clang requires only libstdc++.
    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}")
+    # XXX: The items of the list in LD_PRELOAD can be separated
+    # by spaces or colons, and there is no support for escaping
+    # either separator, see ld.so(8).
+    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()


>> +    # 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.
Ok, added to the previous line
>> +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?


AFAIR we decided with Igor in a verbal conversation

to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}.

LUAJIT_TEST_BINARY is a single target that can be used as a command,

see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a 
list of targets

that should be build before running 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"
> Why don't use just LuaJIT-tests as a test name here?
For consistency with other test titles.
> 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.

Seems this a change made by Igor. I'm totally ok with a previous name.

reverted

>
>> 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.
the same as above
>
>>   )
>>   
>> -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.

Removed.


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

Fixed.


>
>> +  # 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(/
Already fixed.
>
>> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
> I suppose, that general part "test/" may be ommited from the title.
I would left as is. The main idea is to map test title to a file/dir on 
the filesystem.
>
>> +  add_test(NAME ${test_title}
>> +           COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
>> +           WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> Minor: Looks like misindent.
It is ok in a current version.
>
>> +  )
>> +  set_tests_properties(${test_title} PROPERTIES
>> +    ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
> Minor: Semicolon looks excess here.
It is ok in a current version.
>
>> +    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/
It is ok in a current version.
>> +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.
Added a dot to the end of 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.

This is a good idea, but anyway we need two loops.

First should build a list TESTS_COMPILED that is used in 
add_test_suite_target.

The second one should generate  CMake tests.

If you are still want to have a single loop for everything then

we need to build -deps target and CMake test for each tarantool C test.

Left without changes.

>
>> +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(/
this is ok in a current version
>
>> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
> I suppose, that general part "test/" may be ommited from the title.
No, explained above.
>
>> +  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.

As I said before I'm totally ok with old name.

The change was introduced when "Igor automatically squashed all the
tweaks made to the original commit."

Reverted

>
>>     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.
fixed in a current version
>
>>   )
>> +
>>   # 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?


it is better to search test files recursively, no one can guarantee that
files always will be in a top testsuite dir.


>
>> +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.
answered above
>
>> +  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
>>