Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Igor Munkin <imun@tarantool.org>
Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
Date: Mon, 18 Mar 2024 17:58:27 +0300	[thread overview]
Message-ID: <24db4ab3-b7c0-422c-970c-7af0a4f03254@tarantool.org> (raw)
In-Reply-To: <ZfXS4fes0xOtX6UP@tarantool.org>

Igor,

thanks for the next review iteration.

See my comments below.

On 3/16/24 20:12, Igor Munkin wrote:
> Sergey,
>
> Thanks for the fixes! The patch L(almost)GTM now, but I still make some
> comments. You can see my changes here 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
> Typo: Labels are updated since v2.

Updated to:

$ 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                # 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.
>> $ 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`.
> Side note: I guees something like <test-all> can save the behaviour
> broken by this change. However, do we really need this? I left the final
> decision to skaplun@ here.
>
>> 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 workarounded by introducing a special test
>> for each testsuite that executes a target that builds test dependencies.
> Minor: Maybe it's worth to mention <add_test_suite_target> macro here?

Rephrased to:

   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 introduced a macro
   `add_test_suite_target` that adds a CMake-test 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/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."
>> +)
> This is worth to be mentioned in the commit message (at least).

Added:

     The commit introduce a CMake option LUAJIT_TEST_DEPS that set
     dependencies to be used for running and building LuaJIT 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
> <snipped>
>
>> 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
> Typo: We still follows 66 symbol limit for comments in tarantool/luajit,
> but I also left the final decision to skaplun@ here.

Fixed:

--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -78,8 +78,8 @@ 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
-# and is used by `test` target.
+# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains
+# CTest options and is used by `test` target.
  set(TEST_FLAGS
    --output-on-failure
    --schedule-random

>
>> +# and is used by `test` target.
> <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}
>> +  DEPENDS tarantool-c-tests-deps
>> +          tarantool-tests-deps
>> +          lua-Harness-tests-deps
>> +          PUC-Rio-Lua-5.1-tests-deps
>> +          LuaJIT-tests-deps
> I suggest to make LuaJIT-test an umbrella rule without the particular
> implementation. See the changes below:
>
> ================================================================================
>
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 90aaead2..ad447e72 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -130,10 +130,9 @@ add_subdirectory(tarantool-c-tests)
>   add_subdirectory(tarantool-tests)
>   
>   add_custom_target(${PROJECT_NAME}-test
> -  COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}
> -  DEPENDS tarantool-c-tests-deps
> -          tarantool-tests-deps
> -          lua-Harness-tests-deps
> -          PUC-Rio-Lua-5.1-tests-deps
> -          LuaJIT-tests-deps
> +  DEPENDS tarantool-c-tests
> +          tarantool-tests
> +          lua-Harness-tests
> +          PUC-Rio-Lua-5.1-tests
> +          LuaJIT-tests
>   )
>
> ================================================================================

Here we go again. Already discussed it verbally some time ago.

With proposed patch ctest splits output for each testsuite:

$ ninja LuaJIT-test
[9/15] cd 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/t...ctest 
-L tarantool-c-tests --output-on-failure --schedule-random --parallel 8
Test project 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-c-tests 

       Start  7: 
test/tarantool-c-tests/lj-962-premature-stack-overflow.c_test

<snipped>

10/10 Test  #9: test/tarantool-c-tests/misclib-getmetrics-capi.c_test 
...................   Passed    0.06 sec

100% tests passed, 0 tests failed out of 10

Label Time Summary:
tarantool-c-tests    =   0.67 sec*proc (10 tests)

Total Test time (real) =   0.13 sec
[12/15] cd 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/...ctest 
-L lua-Harness-tests --output-on-failure --schedule-random --parallel 8
Test project 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/lua-Harness-tests 


<snipped>

0/50 Test #33: test/lua-Harness-tests/241-standalone.t .... Passed    
1.18 sec [0/6793]

100% tests passed, 0 tests failed out of 50

Label Time Summary:
lua-Harness-tests    =   5.22 sec*proc (50 tests)

Total Test time (real) =   1.53 sec
[13/15] cd 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/.../bin/ctest 
-L LuaJIT-tests --output-on-failure --schedule-random --parallel 8
Test project 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/LuaJIT-tests 

     Start 2: test/LuaJIT-tests
1/1 Test #2: test/LuaJIT-tests ................   Passed    3.65 sec

<snipped>


This output looks ugly and is not convenient in daily use. Left it 
without changes.

>
>>   )
> <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}\;\;"
> The trailing semicolons are automatically added by <make_lua_path>. My
> bad, remove it please.
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -9,7 +9,7 @@ make_lua_path(LUA_CPATH
  )

  set(LUAJIT_TESTS_ENV
-  "LUA_CPATH=${LUA_CPATH}\;\;"
+  "LUA_CPATH=${LUA_CPATH}"
  )

  set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:")

>
>>   )
>>   
>>   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
>> +    # 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()
> I really do not understand, why this quotation is required in the
> original series and breaks everything now. If skaplun@ is bothered more
> than me, he can spend more time with CMake to understand this. Sad, but
> OK, so I'm out here.
>
>>   
> <snipped>
>
>> 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
> <snipped>
>
>> +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.
>> +  # 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: Excess whitespace and condition in <endif>.
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -32,7 +32,7 @@ foreach(test_path ${tests})
    # 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})
+  endif()
    set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
    add_test(NAME ${test_title}
             COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}

>
>> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
>> +  add_test(NAME ${test_title}
>> +           COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
>> +           WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> Typo: Looks like misindent, doesn't it?

Fixed:

    set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
    add_test(NAME ${test_title}
-           COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
-           WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+    COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
+    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
    )
    set_tests_properties(${test_title} PROPERTIES
      ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"

>
>> +  )
>> +  set_tests_properties(${test_title} PROPERTIES
>> +    ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
> Typo: The semicolon looks excess here.
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -39,7 +39,7 @@ foreach(test_path ${tests})
      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
    )
    set_tests_properties(${test_title} PROPERTIES
-    ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
+    ENVIRONMENT "LUA_PATH=${LUA_PATH}"
      LABELS ${TEST_SUITE_NAME}
      DEPENDS lua-Harness-tests-deps
    )

>
>> +    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
> <snipped>
>
>> @@ -49,20 +38,35 @@ foreach(test_source ${tests})
> <snipped>
>
>> +# XXX: The call produces both test and target
>> +# <LuaJIT-tests-deps> as a side effect.
> Typo: Miscopypasting :)

Fixed, thanks!

--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -41,7 +41,7 @@ endforeach()
  set(TEST_SUITE_NAME "tarantool-c-tests")

  # XXX: The call produces both test and target
-# <LuaJIT-tests-deps> as a side effect.
+# <tarantool-c-tests-deps> as a side effect.
  add_test_suite_target(tarantool-c-tests
    LABEL ${TEST_SUITE_NAME}
    DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED}

>
>> +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
>> +# To workaround this, we glob source files and generated tests
>> +# with paths to executable binaries.
>> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
>> +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: Excess whitespace and condition in <endif>.
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -59,7 +59,7 @@ foreach(test_path ${tests})
    # 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})
+  endif()
    set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
    add_test(NAME ${test_title}
      COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}

>
>> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
> <snipped>
>
>> 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
> <snipped>
>
>> @@ -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_BINARY_DIR}/?.lua.

Fixed:

--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -54,7 +54,6 @@ make_lua_path(LUA_PATH
      ${PROJECT_SOURCE_DIR}/tools/?.lua
      ${LUAJIT_SOURCE_DIR}/?.lua
      ${LUAJIT_BINARY_DIR}/?.lua
-    ${PROJECT_BINARY_DIR}/src/?.lua
  )

  # Update LUA_CPATH with the library paths collected within

>>   )
> <snipped>
>
>> @@ -110,22 +97,29 @@ else()
> <snipped>
>
>> +# 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}")
> The trailing semicolons are automatically added by <make_lua_path>.
> Please, remove it.

Fixed:

--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -107,7 +107,7 @@ add_test_suite_target(tarantool-tests

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

>
>> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> Minor: Why is <GLOB_RECURSE> used here, and the hack with manual
> directory name filtering is omitted?

it is better to search test files recursively, no one can guarantee that 
files always will be

in a top testsuite dir.

However in lua-Harness suite we cannot use GLOB_RECURSE because suite

has a number of files with equal filenames:

$ find  test/lua-Harness-tests/ -name lexico.t
test/lua-Harness-tests/lexico53/lexico.t
test/lua-Harness-tests/lexicojit/lexico.t
test/lua-Harness-tests/lexico52/lexico.t
test/lua-Harness-tests/lexico54/lexico.t

due to this CMake configuration has failed with

"add_test given test NAME "test/lua-Harness-tests/lexico.t" which 
already exists in this directory."


>
>> +foreach(test_path ${tests})
>> +  get_filename_component(test_name ${test_path} NAME)
>> +  set(test_title "test/tarantool-tests/${test_name}")
>> +  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
>>

  reply	other threads:[~2024-03-18 14:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-12  7:06 Sergey Bronnikov via Tarantool-patches
2024-03-16 17:12 ` Igor Munkin via Tarantool-patches
2024-03-18 14:58   ` Sergey Bronnikov via Tarantool-patches [this message]
2024-03-18 15:54 ` Sergey Kaplun via Tarantool-patches
2024-03-20 19:44   ` Sergey Bronnikov via Tarantool-patches
2024-03-21 11:17     ` Sergey Kaplun via Tarantool-patches
2024-03-22 13:06       ` Sergey Bronnikov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=24db4ab3-b7c0-422c-970c-7af0a4f03254@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox