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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

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

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

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

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

================================================================================

>  )

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

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

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

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

Typo: The 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

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

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

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

>  )

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

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

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

-- 
Best regards,
IM