[Tarantool-patches] [PATCH luajit][v2] cmake: replace prove with CTest
Sergey Bronnikov
sergeyb at tarantool.org
Tue Oct 31 13:42:42 MSK 2023
Hello,
On 10/24/23 16:41, Maxim Kokryashkin via Tarantool-patches wrote:
> 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/
Fixed.
>> generated `test` target. It means that target `test` now is executing
> Typo: s/now is/is now/
Fixed.
>> `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/
Fixed.
>> `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/
Fixed. But I would left "a future" because our future is undefined. :)
>> could change these test runners to specify test from outside and after
> Typo: s/test from/tests from/
> Typo: s/outside/outside,/
Fixed.
>> 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/
Fixed.
>> see [3]. It means that all object files and executables must be build
> Typo: s/build/built/
Fixed.
>> 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?
Yes, we do.
However, there is a nuance.
- exit code calculated in test:done method [1]. Under the hood :done
uses result provided by :check.
- test:done is not a mandatory in Lua test, you can run a test without it
- test will always exit with exit code equal to zero if someone will
forget to add test:done
I suggest to add atexit handler that will execute test:done.
1.
https://github.com/tarantool/luajit/blob/8aaac18647bcbfdf1f8c5a241219269b7829f9f7/test/tarantool-tests/tap.lua#L353-L362
> 2. Shall we keep the `tap` module, or we can move to something
> much simplier (even conventional assertions will do at this point)?
This patch changes test runner, not a unit-testing framework.
>
>> 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'.
Updated, thanks.
>
>> +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'
The same - updated, thanks.
>> +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/
Fixed.
>> +# 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/
Fixed.
>> +# with path to executable binaries.
> Typo: s/path/paths/
Fixed.
>> +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.
What is actually strange with LUA_TEST_ENV_MORE?
Value of LUA_TEST_ENV_MORE is different on Darwin and non-Darwin OSes
(see condition above in test/tarantool-tests/CMakeLists.txt).
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -120,6 +120,7 @@ else()
list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
endif()
+set(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)
@@ -130,7 +131,7 @@ foreach(test_path ${tests})
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}"
+ ENVIRONMENT "${ENV}"
LABELS tarantool
)
endforeach()
>> + 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