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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On Mon, Oct 23, 2023 at 12:48:56PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> The patch replaces the main test runner prove(1) with CTest, see [1] and
> [2].
> 
> The benefits are:
> 
> - less external dependencies, prove(1) is gone
> - running tests by test title
> - extended test running options in comparison to prove(1)
> - unified test output (finally!)
> 
> Note, it is not possible to attach targets to the automatically
Typo: s/Note,/Note that/
> generated `test` target. It means that target `test` now is executing
Typo: s/now is/is now/
> `LuaJIT-test`, but not `LuaJIT-lint`.
> 
> Note, the testsuites in `test/LuaJIT-tests` and in
Typo: s/Note,/Note that/
Typo: s/and in/and/
> `test/PUC-Rio-Lua-5.1` directories have their own test runners and
> currently CTest runs these testsuites as a single tests. In a future we
Typo: s/as a/as/
Typo: s/In a future/In the future/
> could change these test runners to specify test from outside and after
Typo: s/test from/tests from/
Typo: s/outside/outside,/
> this we could run tests separately by CTest in these test suites.
> 
> Note, it is not possible to add dependencies to `add_test()` in CMake,
Typo: s/Note,/Note that/
> see [3]. It means that all object files and executables must be build
Typo: s/build/built/
> before running `ctest(1)`.
> 
> Examples of using CTest in a build directory:
> 
> $ ctest                             # Running all tests.
> $ ctest -L tarantool                # Running tests in tarantool-tests dir.
> $ ctest -L tarantool-c              # Running tests in tarantool-c-tests dir.
> $ ctest -L lua-Harness              # Running tests in lua-Harness dir.
> $ ctest -L luajit                   # Running tests in LuaJIT dir.
> $ ctest -L lua                      # Running tests in PUC-Rio-Lua-5.1 dir.
> $ ctest --rerun-fail
> $ ctest --output-on-failure
> 
> 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
> ---
> Changes v2:
> - rebased to master
> - fixed tests that require loading object files
> - fixed backward compatibility with CMake targets
> 
> PR: https://github.com/tarantool/tarantool/pull/9286
> Branch: https://github.com/tarantool/luajit/tree/ligurio/enable_test_target
For whatever reason, CI consistently fails for ASAN tests.

Also, AFAIK, `ctest` just checks the exit status for each of the test cases.
Hence, here are two questions:
1. Do we consider this a sufficient validation?
2. Shall we keep the `tap` module, or we can move to something
much simplier (even conventional assertions will do at this point)?

> Patch v1 was sent as RFC without list address in CC.
> 
>  CMakeLists.txt                            |  1 +
>  test/CMakeLists.txt                       | 28 ++++--------
>  test/LuaJIT-tests/CMakeLists.txt          | 17 +++++--
>  test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 23 ++++++----
>  test/lua-Harness-tests/CMakeLists.txt     | 42 +++++++++---------
>  test/tarantool-c-tests/CMakeLists.txt     | 49 ++++++++++----------
>  test/tarantool-tests/CMakeLists.txt       | 54 ++++++++---------------
>  7 files changed, 100 insertions(+), 114 deletions(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index eebf3d6f..1943d894 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -356,6 +356,7 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>    "Lua code need to be run before tests are started."
>  )
>  
> +enable_testing()
>  add_subdirectory(test)
>  
>  # --- Misc rules ---------------------------------------------------------------
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 58cba5ba..bc0c8dd8 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -73,6 +73,15 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
>  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
>  separate_arguments(LUAJIT_TEST_COMMAND)
>  
> +set(TEST_FLAGS
> +  --output-on-failure
> +  --schedule-random
> +  --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
> +)
> +if(CMAKE_VERBOSE_MAKEFILE)
> +  list(APPEND TEST_FLAGS --verbose)
> +endif()
> +
>  add_subdirectory(LuaJIT-tests)
>  add_subdirectory(PUC-Rio-Lua-5.1-tests)
>  add_subdirectory(lua-Harness-tests)
> @@ -86,22 +95,3 @@ add_custom_target(${PROJECT_NAME}-test DEPENDS
>    tarantool-c-tests
>    tarantool-tests
>  )
> -
> -if(LUAJIT_USE_TEST)
> -  if(POLICY CMP0037)
> -    if(CMAKE_VERSION VERSION_LESS 3.11)
> -      # CMake below 3.11 reserves the name 'test'. Use old policy.
> -      # https://cmake.org/cmake/help/v3.11/release/3.11.html#other-changes
> -      cmake_policy(SET CMP0037 OLD)
> -    else()
> -      # Starting from CMake 3.11 the name 'test' is reserved in
> -      # special cases and can be used as a target name.
> -      cmake_policy(SET CMP0037 NEW)
> -    endif()
> -  endif(POLICY CMP0037)
> -
> -  add_custom_target(test DEPENDS
> -    ${PROJECT_NAME}-test
> -    ${PROJECT_NAME}-lint
> -  )
> -endif()
> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index 9cd76ee9..aa36097a 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -1,12 +1,21 @@
>  # See the rationale in the root CMakeLists.txt
>  cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>  
> -add_custom_target(LuaJIT-tests DEPENDS ${LUAJIT_TEST_BINARY})
> -
> -add_custom_command(TARGET LuaJIT-tests
> -  COMMENT "Running LuaJIT-tests"
> +# The test suite has its own test runner
> +# (test/LuaJIT-tests/test.lua), it is not possible
> +# to run these tests separately by CTest.
It is better to say 'one-by-one' than 'separately'.

> +message(STATUS "Add test test/LuaJIT-tests")
> +add_test(NAME "test/LuaJIT-tests"
>    COMMAND
>      ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
>      +slow +ffi +bit +jit
>    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
>  )
> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
> +  LABELS luajit
> +)
> +
> +add_custom_target(LuaJIT-tests
> +  COMMAND ${CMAKE_CTEST_COMMAND} -L luajit ${TEST_FLAGS}
> +  DEPENDS luajit-main
> +)
> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> index 98277f9a..af180c0a 100644
> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> @@ -34,17 +34,22 @@ 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
> +# 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 separately by CTest.
Same comment about 'one-by-one'
> +message(STATUS "Add test test/PUC-Rio-Lua-5.1")
> +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}\;\;"
> +  LABELS lua
>  )
>  
> -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
> -  WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +add_custom_target(PUC-Rio-Lua-5.1-tests
> +  COMMAND ${CMAKE_CTEST_COMMAND} -L lua ${TEST_FLAGS}
> +  DEPENDS luajit-main PUC-Rio-Lua-5.1-tests-prepare
>  )
>  
>  # vim: expandtab tabstop=2 shiftwidth=2
> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
> index f748a8fd..70489061 100644
> --- a/test/lua-Harness-tests/CMakeLists.txt
> +++ b/test/lua-Harness-tests/CMakeLists.txt
> @@ -4,12 +4,6 @@
>  # See the rationale in the root CMakeLists.txt
>  cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>  
> -find_program(PROVE prove)
> -if(NOT PROVE)
> -  message(WARNING "`prove' is not found, so lua-Harness-tests target is not generated")
> -  return()
> -endif()
> -
>  # Tests create temporary files (see 303-package.t and 411-luajit.t
>  # for example) to be required. Also, they require some files from
>  # the original test source directory.
> @@ -20,23 +14,29 @@ make_lua_path(LUA_PATH
>      ${LUAJIT_SOURCE_DIR}/?.lua
>      ${LUAJIT_BINARY_DIR}/?.lua
>  )
> -set(LUA_TEST_FLAGS --failures --shuffle)
>  
> -if(CMAKE_VERBOSE_MAKEFILE)
> -  list(APPEND LUA_TEST_FLAGS --verbose)
> -endif()
> +set(SUITE_NAME "lua-Harness-tests")
> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.t")
> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME)
> +  if (${test_name} STREQUAL ${SUITE_NAME})
> +    continue()
> +  endif (${test_name} STREQUAL ${SUITE_NAME})
> +  set(test_title "test/${SUITE_NAME}/${test_name}")
> +  message(STATUS "Add test ${test_title}")
> +  add_test(NAME ${test_title}
> +           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}\;"
> +    LABELS lua-Harness
> +  )
> +endforeach()
>  
> -add_custom_target(lua-Harness-tests DEPENDS ${LUAJIT_TEST_BINARY})
> -add_custom_command(TARGET lua-Harness-tests
> -  COMMENT "Running lua-Harness tests"
> -  COMMAND
> -  env
> -    LUA_PATH="${LUA_PATH}"
> -    ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
> -      --exec '${LUAJIT_TEST_COMMAND} -l profile_luajit21'
> -      --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> -      ${LUA_TEST_FLAGS}
> -  WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +add_custom_target(lua-Harness-tests
> +  COMMAND ${CMAKE_CTEST_COMMAND} -L lua-Harness ${TEST_FLAGS}
> +  DEPENDS luajit-main
>  )
>  
>  # vim: expandtab tabstop=2 shiftwidth=2
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index 17255345..902828bf 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -1,15 +1,4 @@
> -find_program(PROVE prove)
> -if(NOT PROVE)
> -  message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
> -  return()
> -endif()
> -
>  set(C_TEST_SUFFIX .c_test)
> -set(C_TEST_FLAGS --failures --shuffle)
> -
> -if(CMAKE_VERBOSE_MAKEFILE)
> -  list(APPEND C_TEST_FLAGS --verbose)
> -endif()
>  
>  # Build libtest.
>  
> @@ -49,21 +38,29 @@ foreach(test_source ${tests})
>    LIST(APPEND TESTS_COMPILED ${exe})
>  endforeach()
>  
> +# Note, we cannot globbing generated files in CMake, see
Typo: s/Note,/Note that/
Typo: s/globbing/glob/
> +# https://stackoverflow.com/questions/44076307/cmake-globbing-generated-files
> +# To workaround this we globbing source files and generated tests
Typo: s/this/this,/
Typo: s/globbing/glob/
> +# with path to executable binaries.
Typo: s/path/paths/
> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
> +set(SUITE_NAME "tarantool-c-tests")
> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME_WE)
> +  if (${test_name} STREQUAL ${SUITE_NAME})
> +    continue()
> +  endif (${test_name} STREQUAL ${SUITE_NAME})
> +  set(test_title "test/${SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
> +  message(STATUS "Add test ${test_title}")
> +  add_test(NAME ${test_title}
> +    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}
> +    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +  )
> +  set_tests_properties(${test_title} PROPERTIES
> +    LABELS tarantool-c
> +  )
> +endforeach()
> +
>  add_custom_target(tarantool-c-tests
> +  COMMAND ${CMAKE_CTEST_COMMAND} -L tarantool-c ${TEST_FLAGS}
>    DEPENDS libluajit libtest ${TESTS_COMPILED}
>  )
> -
> -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}
> -)
> -
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index c15d6037..e9d89366 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -4,12 +4,6 @@
>  # See the rationale in the root CMakeLists.txt.
>  cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>  
> -find_program(PROVE prove)
> -if(NOT PROVE)
> -  message(WARNING "`prove' is not found, so tarantool-tests target is not generated")
> -  return()
> -endif()
> -
>  macro(BuildTestCLib lib sources)
>    add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources})
>    target_include_directories(${lib} PRIVATE
> @@ -83,21 +77,14 @@ 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
>  # <BuildTestLib> macro.
>  make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS})
>  
>  set(LUA_TEST_SUFFIX .test.lua)
> -set(LUA_TEST_FLAGS --failures --shuffle)
> -set(LUA_TEST_ENV
> -  "LUA_PATH=\"${LUA_PATH}\""
> -  "LUA_CPATH=\"${LUA_CPATH}\""
> -)
> -
> -if(CMAKE_VERBOSE_MAKEFILE)
> -  list(APPEND LUA_TEST_FLAGS --verbose)
> -endif()
>  
>  # XXX: Since the auxiliary libraries are built as a dynamically
>  # loaded modules on MacOS instead of shared libraries as it is
> @@ -133,25 +120,22 @@ else()
>    list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>  endif()
>  
> -# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> -# with dependecies are set in scope of BuildTestLib macro.
> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME)
> +  set(test_title "test/tarantool-tests/${test_name}")
> +  message(STATUS "Add test ${test_title}")
> +  add_test(NAME ${test_title}
> +    COMMAND ${LUAJIT_TEST_COMMAND} ${test_path}
> +    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +  )
> +  set_tests_properties(${test_title} PROPERTIES
> +    ENVIRONMENT "LUA_PATH=${LUA_PATH};\;\;;LUA_CPATH=${LUA_CPATH}\;\;;${LUA_TEST_ENV_MORE}"
Not sure if it is better to construct the test env here. IMO, it becomes
more readable, when constructed prior to the iteration.
Also, I tend to believe that it kind of starnge to have `LUA_TEST_ENV_MORE`
now. IINM, it was needed because of the shell shenenigans and `prove` usage,
but know it sould be possible to put everything into a single env.
> +    LABELS tarantool
> +  )
> +endforeach()
> +
>  add_custom_target(tarantool-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
> +  COMMAND ${CMAKE_CTEST_COMMAND} -L tarantool ${TEST_FLAGS}
> +  DEPENDS luajit-main ${TESTLIBS}
>  )
> -add_custom_command(TARGET tarantool-tests
> -  COMMENT "Running Tarantool tests"
> -  COMMAND
> -  # XXX: We can't move everything to the "inner" env, since there
> -  # are some issues with escaping ';' for different shells. As
> -  # a result LUA_PATH/LUA_CPATH variables are set via the "outer"
> -  # env, since they are not stripped by SIP like LD_*/DYLD_* are.
> -  env
> -    ${LUA_TEST_ENV}
> -    ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
> -      --exec 'env ${LUA_TEST_ENV_MORE} ${LUAJIT_TEST_COMMAND}'
> -      --ext ${LUA_TEST_SUFFIX}
> -      --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> -      ${LUA_TEST_FLAGS}
> -  WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> -)
> -
> -- 
> 2.34.1
>