[Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
Igor Munkin
imun at tarantool.org
Sat Mar 16 20:12:01 MSK 2024
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 at 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
More information about the Tarantool-patches
mailing list