[Tarantool-patches] [PATCH luajit][v2] cmake: replace prove with CTest
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Tue Oct 24 16:41:47 MSK 2023
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 at 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
>
More information about the Tarantool-patches
mailing list