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

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

Sergey,

thanks for the answers. See my comments.


On 3/21/24 14:17, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> Please consider my concerns below.
> I suggest moving our conversation forward with the new patch version.
>
> On 20.03.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>> thanks for review!
>>
>> see my comments below
>>
>> On 3/18/24 18:54, Sergey Kaplun wrote:
> <snipped>
>
>>>> 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)?
> See my comment below.
>
>>>> +
>>>>    # 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
> Typo: s/`test`/the `test`/

Fixed.


>
>> +# the build directory root, this command should be in the source
>> +# directory root (hence, in this CMakeLists.txt).
>>    enable_testing()
>>    add_subdirectory(test)
>>
>>
> <snipped>
>
>>> 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.
> I got it thanks for the clarification!
> May you please add a comment with the "FIXME:" mark and mention the
> CMake version and the CMAKE_CTEST_ARGUMENTS variable?

Already there, see v4.


>>> 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.
> Unfortunately, as you mentioned offline, for some builds [1], even 5
> minutes is not enough:
> | 211/211 Test #109: test/tarantool-tests/lj-1034-tabov-error-frame.test.lua ........................***Timeout 300.54 sec
> I suggest increasing the timeout to 30 minutes.
>
Already updated and added comment with explanation.


>>>> +)
>>>> +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.
> Sorry, I've misguided you with my comment. I thought that we should name
> the test as "${target}", not "${_DEPS_TARGET}" (I hadn't gotten the idea
> of proxy targets before, but I'm OK with it now).
> Please revert this change.
> My bad.

Actually nothing bad is happen.

Change below was made after your comment:


@@ -107,10 +114,10 @@ function(add_test_suite_target target)

    add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})

-  add_test(${_DEPS_TARGET}
-           ${CMAKE_COMMAND}
-             --build ${CMAKE_BINARY_DIR}
-             --target ${_DEPS_TARGET}
+  add_test(NAME ${_DEPS_TARGET}
+    COMMAND ${CMAKE_COMMAND}
+            --build ${CMAKE_BINARY_DIR}
+            --target ${_DEPS_TARGET}
    )
    set_tests_properties(${_DEPS_TARGET} PROPERTIES
      LABELS ${target}

Seems according to BNF in [1] `NAME` is required before test name  in 
`add_test`.

But in fact `NAME` can be omitted. I would left `NAME` to be consistent 
with CMake documentation.


1. https://cmake.org/cmake/help/latest/command/add_test.html




> Also, please add the comment that this proxy test allows us to be sure that
> all prerequisites must be built or generated before the main test
> execution.

Added.

--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -96,6 +96,17 @@ 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.
+# To workaround this a function `add_test_suite_target` is
+# introduced, it adds a CMake target that builds testsuite's
+# prerequisites and CMake test that executes that target.
+#
+# 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
  function(add_test_suite_target target)
    set(prefix ARG)
    set(noValues)

>>
>>> 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
> That we are really testing something with this strange name.
> OK, my bad ignore it.
>
>> `LuaJIT-tests-build`  much more confusing than "xxx-deps".
>>
>>> |   Test   #1: LuaJIT-tests-deps
> OK, let's leave it as it is.
>
> <snipped>
>
>>>> -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.
> Please, mention the desired output format in the comment.
@@ -137,6 +148,13 @@ add_subdirectory(lua-Harness-tests)
  add_subdirectory(tarantool-c-tests)
  add_subdirectory(tarantool-tests)

+# Each testsuite has it's own CMake target, but combining these
+# target to a single one is not desired, because each target
+# runs it's own ctest command, that each time enumerates tests
+# from zero and prints test summary at the end of test run.
+# For common target this output looks inconvenient.
+# Therefore target below executes a single instance of ctest
+# command that runs all generated CMake tests.
  add_custom_target(${PROJECT_NAME}-test
    COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}
    DEPENDS tarantool-c-tests-deps

>
> <snipped>
>
>>>>    
>>>>    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.
> It works in our CI, where we use the clang build.
>
> For now, we may just disable this test for GCC + clang build with the
> link to the issue.


Added:


--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -123,7 +123,8 @@ foreach(test_path ${tests})
    )
  endforeach()

-# Test is broken when GCC with enabled ASAN is used.
+# Test is broken when GCC with enabled ASAN is used,
+# see https://github.com/tarantool/tarantool/issues/9856.
  if (LUAJIT_USE_ASAN AND (CMAKE_C_COMPILER_ID STREQUAL "GNU"))
    set(test_title 
test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua)
    set_tests_properties(${test_title} PROPERTIES

>
> On branch, you're using global LD_PRELOAD from the aforementioned test
> (lj-802-panic-at-mcode-protfail.test.lua). As a result, tons of tests
> fail with the following error:
> | got:  PANIC: unprotected error in call to Lua API (runtime code generation failed, restricted kernel?)
>
> May be it is better to proxy parent LD_PRELOAD to the test instead?
Ok
>
>> 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}
> Please mention in the comment that the C++ and C compilers use the same
> tooling, so the returned library path is the same.

Already commented in v4:

        # CMAKE_C_COMPILER is used because it has the same behaviour
         # as CMAKE_CXX_COMPILER, but CMAKE_CXX_COMPILER is not required
         # for building LuaJIT and can be missed in GH Actions.
         COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
         OUTPUT_VARIABLE LIB_LINK
OUTPUT_STRIP_TRAILING_WHITESPACE
         RESULT_VARIABLE RES

>
>>          OUTPUT_VARIABLE LIB_LINK
>>          OUTPUT_STRIP_TRAILING_WHITESPACE
>> +      TIMEOUT 5
> Why do we need timeout here? (*)
Removed in v4.
>
>> +      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")
> Nit: the message is not very informative. I still don't know how to
> rewrite it, since compilers just print the input for unexisting files.
> Hence, I am not sure: do we really need this error message, since the
> given command is valid?

Discussed verbally and in v4 is the following message:


       if (NOT RES EQUAL 0)
         message(FATAL_ERROR "Executing '${CMAKE_C_COMPILER} \
                              -print-file-name=${lib}' has failed")
       endif()

>> +    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
> (*) And here?
Removed in v4.
>
>>        )
>>      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()
> Minor: Maybe it is better to move this change to a separate patch.
> Feel free to ignore.

Ignored. Before introducing CTest this worked, changes above fixes

behaviour broken by patch with CTest.

>
> <snipped>
>
>>>> +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.
> Yes, but we are now setting LUAJIT_TEST_DEPS unconditionally.
> Also, as I mentioned above for tarantool-c-tests, there is no need to
> build the luajit-main target -- libluajit is enough. Hence, this change
> may be dropped.
> CC imun@ 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?
>> For consistency with other test titles.
> Side note: It looks inconsistent anyway (partially):
>
> |       Start   1: LuaJIT-tests-deps
> | 1/211 Test   #1: LuaJIT-tests-deps ..............................................................   Passed    0.29 sec
> |       Start   2: test/LuaJIT-tests
> | 2/211 Test   #2: test/LuaJIT-tests ..............................................................   Passed    3.56 sec
> |       Start   3: PUC-Rio-Lua-5.1-tests-deps
> | 3/211 Test   #3: PUC-Rio-Lua-5.1-tests-deps .....................................................   Passed    0.35 sec
> |       Start   4: test/PUC-Rio-Lua-5.1
>
> As you can see, *-deps targets have no test in their name.
> But I can't come up with something meaningful, so feel free to ignore
> it.


Okay, I left as is.

>>> 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
> <snipped>
>
>>>> +  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.
> It's kind of obvious to me that tests are placed in the directory
> "test". I'm not insisting, but the names of the tests are already long
> enough, it will be nicer to decrease their length with unnecessary
> information. The "test" directory is still the root directory for all
> tests.

It is not clear for me why you are care about test title length.

ctest accepts partial test titles too:

`ctest -R lj-802` 
(test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua will be 
executed)

> OTOH, it is more convenient to copy-paste the filenames, but for sure, we
> must use the whole filename to be consistent with the prove's output we
> are replacing.
>
> <snipped>
>
>>>> +# 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.
> Shouldn't it be enough to set
> | DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${exe}
> for each test? But, yes, in this way, the order of compilation and
> execution isn't sequential...
> Maybe we should use the proxy target here. In the loop, we append a
> dependency for this target and set it as a dependency for each test
> target. May it work?


seems yes


diff --git a/test/tarantool-c-tests/CMakeLists.txt 
b/test/tarantool-c-tests/CMakeLists.txt
index a4bc3a76..abf44a46 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -19,6 +19,21 @@ 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.
+# This is needed because targets for each C test are generated
+# at the same time with CMake tests, and all prerequisites must be
+# already exist at this moment.
+add_custom_target(tarantool-c-tests-build)
+
+# XXX: The call produces both test and target
+# <tarantool-c-tests-deps> as a side effect.
+add_test_suite_target(tarantool-c-tests
+  LABELS ${TEST_SUITE_NAME}
+  DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest tarantool-c-tests-build
+)
+
  set(CTEST_SRC_SUFFIX ".test.c")
  file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
  foreach(test_source ${tests})
@@ -36,27 +51,12 @@ foreach(test_source ${tests})
    )
    target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
    LIST(APPEND TESTS_COMPILED ${exe})
-endforeach()
-
-set(TEST_SUITE_NAME "tarantool-c-tests")
-
-# XXX: The call produces both test and target
-# <tarantool-c-tests-deps> as a side effect.
-add_test_suite_target(tarantool-c-tests
-  LABELS ${TEST_SUITE_NAME}
-  DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED}
-)
+  add_dependencies(tarantool-c-tests-build ${exe})

-# XXX: Note that we cannot glob generated files in CMake, see
-# 
https://stackoverflow.com/questions/44076307/cmake-globbing-generated-files.
-# To workaround this, we glob source files and generated tests
-# with paths to executable binaries.
-file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${CTEST_SRC_SUFFIX}")
-foreach(test_path ${tests})
-  get_filename_component(test_name ${test_path} NAME_WE)
-  set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
+  # Generate CMake tests.
+  set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")
    add_test(NAME ${test_title}
-    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}
+    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}
      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
    )
    set_tests_properties(${test_title} PROPERTIES

>
>> Left without changes.
> May you please add a comment about it to avoid confusion in the future?


patch is above


>
> <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.
> Ok, thanks for the clarification.
>
> <snipped>
>