* [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
@ 2024-03-12 7:06 Sergey Bronnikov via Tarantool-patches
2024-03-16 17:12 ` Igor Munkin via Tarantool-patches
2024-03-18 15:54 ` Sergey Kaplun via Tarantool-patches
0 siblings, 2 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-12 7:06 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, max.kokryashkin, Igor Munkin
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
$ 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`.
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.
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/.gitignore b/.gitignore
index dc5ea5fc..dd1f8888 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,7 +15,9 @@
.ninja_log
CMakeCache.txt
CMakeFiles
+CTestTestfile.cmake
Makefile
+Testing
build.ninja
cmake_install.cmake
cmake_uninstall.cmake
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."
+)
+
# 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
@@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
"Lua code need to be run before tests are started."
)
+# XXX: Enable testing via CTest. Since CTest expects to find a
+# test file in the build directory root, this command should be in
+# the source directory root (hence, in this CMakeLists.txt).
+enable_testing()
add_subdirectory(test)
# --- Misc rules ---------------------------------------------------------------
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
+# and is used by `test` target.
+set(TEST_FLAGS
+ --output-on-failure
+ --schedule-random
+ --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
+)
+if(CMAKE_VERBOSE_MAKEFILE)
+ list(APPEND TEST_FLAGS --verbose)
+endif()
+
+function(add_test_suite_target target)
+ set(prefix ARG)
+ set(noValues)
+ set(singleValues LABEL)
+ set(multiValues DEPENDS)
+
+ # FIXME: if we update to CMake >= 3.5, can remove this line.
+ include(CMakeParseArguments)
+ cmake_parse_arguments(${prefix}
+ "${noValues}"
+ "${singleValues}"
+ "${multiValues}"
+ ${ARGN})
+
+ set(_DEPS_TARGET ${target}-deps)
+
+ add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
+
+ add_test(${_DEPS_TARGET}
+ ${CMAKE_COMMAND}
+ --build ${CMAKE_BINARY_DIR}
+ --target ${_DEPS_TARGET}
+ )
+
+ message(STATUS "Add test suite ${ARG_LABEL}")
+
+ add_custom_target(${target}
+ COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
+ DEPENDS ${_DEPS_TARGET}
+ )
+
+ unset(_DEPS_TARGET)
+endfunction()
+
add_subdirectory(LuaJIT-tests)
add_subdirectory(PUC-Rio-Lua-5.1-tests)
add_subdirectory(lua-Harness-tests)
add_subdirectory(tarantool-c-tests)
add_subdirectory(tarantool-tests)
-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
)
-
-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 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}\;\;"
)
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()
@@ -64,12 +61,25 @@ if(LUAJIT_NO_UNWIND)
set(LUAJIT_TEST_TAGS_EXTRA +internal_unwinder)
endif()
-add_custom_command(TARGET LuaJIT-tests
- COMMENT "Running LuaJIT-tests"
- COMMAND
- env
- ${LUAJIT_TESTS_ENV}
- ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
- +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
+set(TEST_SUITE_NAME "LuaJIT-tests")
+
+# XXX: The call produces both test and target
+# <LuaJIT-tests-deps> as a side effect.
+add_test_suite_target(LuaJIT-tests
+ LABEL ${TEST_SUITE_NAME}
+ DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
+)
+
+# The test suite has its own test runner
+# (test/LuaJIT-tests/test.lua), it is not possible to run these
+# tests one-by-one by CTest.
+add_test(NAME "test/LuaJIT-tests"
+ COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
+ +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
)
+set_tests_properties("test/LuaJIT-tests" PROPERTIES
+ LABELS ${TEST_SUITE_NAME}
+ ENVIRONMENT "${LUAJIT_TESTS_ENV}"
+ DEPENDS LuaJIT-tests-deps
+)
diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
index 2f90da86..f0e639e1 100644
--- a/test/LuaJIT-tests/src/CMakeLists.txt
+++ b/test/LuaJIT-tests/src/CMakeLists.txt
@@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c)
enable_language(CXX)
AddTestLib(libcpptest libcpptest.cpp)
-add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS})
+add_custom_target(LuaJIT-tests-deps-libs DEPENDS ${TESTLIBS})
diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
index 98277f9a..af89bfe5 100644
--- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
+++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
@@ -34,17 +34,26 @@ 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
+set(TEST_SUITE_NAME "PUC-Rio-Lua-5.1-tests")
+
+# XXX: The call produces both test and target
+# <PUC-Rio-Lua-5.1-tests-deps> as a side effect.
+add_test_suite_target(PUC-Rio-Lua-5.1-tests
+ LABEL ${TEST_SUITE_NAME}
+ DEPENDS ${LUAJIT_TEST_DEPS} PUC-Rio-Lua-5.1-tests-prepare
)
-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
+# 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 one-by-one by CTest.
+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 ${TEST_SUITE_NAME}
+ DEPENDS PUC-Rio-Lua-5.1-tests-deps
+)
# vim: expandtab tabstop=2 shiftwidth=2
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
@@ -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,35 @@ 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(TEST_SUITE_NAME "lua-Harness-tests")
-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}
+# XXX: The call produces both test and target
+# <lua-Harness-tests-deps> as a side effect.
+add_test_suite_target(lua-Harness-tests
+ LABEL ${TEST_SUITE_NAME}
+ DEPENDS ${LUAJIT_TEST_DEPS}
)
+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})
+ 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}
+ )
+ set_tests_properties(${test_title} PROPERTIES
+ ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
+ 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
@@ -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,20 +38,35 @@ foreach(test_source ${tests})
LIST(APPEND TESTS_COMPILED ${exe})
endforeach()
-add_custom_target(tarantool-c-tests
- DEPENDS libluajit libtest ${TESTS_COMPILED}
-)
+set(TEST_SUITE_NAME "tarantool-c-tests")
-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}
+# XXX: The call produces both test and target
+# <LuaJIT-tests-deps> as a side effect.
+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})
+ set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
+ 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 ${TEST_SUITE_NAME}
+ DEPENDS tarantool-c-tests-deps
+ )
+endforeach()
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
@@ -4,19 +4,13 @@
# 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()
-
-add_custom_target(tarantool-tests
+add_custom_target(tarantool-tests-deps-libs
DEPENDS ${LUAJIT_TEST_BINARY}
)
macro(BuildTestCLib lib sources)
AddTestLib(${lib} ${sources})
- add_dependencies(tarantool-tests ${lib})
+ add_dependencies(tarantool-tests-deps-libs ${lib})
# Add the directory where the lib is built to the list with
# entries for LUA_CPATH environment variable, so LuaJIT can find
# and load it. See the comment about extending the list in the
@@ -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
)
+
# 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
@@ -110,22 +97,29 @@ else()
list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
endif()
-# LUA_CPATH and LD_LIBRARY_PATH variables and also dependencies
-# list with libraries are set in scope of BuildTestLib macro.
-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}
+set(TEST_SUITE_NAME "tarantool-tests")
+
+# XXX: The call produces both test and target
+# <tarantool-tests-deps> as a side effect.
+add_test_suite_target(tarantool-tests
+ LABEL ${TEST_SUITE_NAME}
+ DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
)
+# 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}")
+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}")
+ 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
2024-03-12 7:06 [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches
@ 2024-03-16 17:12 ` Igor Munkin via Tarantool-patches
2024-03-18 14:58 ` Sergey Bronnikov via Tarantool-patches
2024-03-18 15:54 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2024-03-16 17:12 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
2024-03-16 17:12 ` Igor Munkin via Tarantool-patches
@ 2024-03-18 14:58 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-18 14:58 UTC (permalink / raw)
To: Igor Munkin; +Cc: max.kokryashkin, tarantool-patches
Igor,
thanks for the next review iteration.
See my comments below.
On 3/16/24 20:12, Igor Munkin wrote:
> 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.
Updated to:
$ ctest --print-labels
Test project <snipped>
All Labels:
LuaJIT-tests
PUC-Rio-Lua-5.1-tests
lua-Harness-tests
tarantool-c-tests
tarantool-tests
>
>> $ 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?
Rephrased to:
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 introduced a macro
`add_test_suite_target` that adds a CMake-test for each testsuite
that executes a target that builds test dependencies.
>
>> 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).
Added:
The commit introduce a CMake option LUAJIT_TEST_DEPS that set
dependencies to be used for running and building LuaJIT tests.
>
>> +
>> # 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.
Fixed:
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -78,8 +78,8 @@ 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
-# and is used by `test` target.
+# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains
+# CTest options and is used by `test` target.
set(TEST_FLAGS
--output-on-failure
--schedule-random
>
>> +# 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
> )
>
> ================================================================================
Here we go again. Already discussed it verbally some time ago.
With proposed patch ctest splits output for each testsuite:
$ ninja LuaJIT-test
[9/15] cd
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/t...ctest
-L tarantool-c-tests --output-on-failure --schedule-random --parallel 8
Test project
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-c-tests
Start 7:
test/tarantool-c-tests/lj-962-premature-stack-overflow.c_test
<snipped>
10/10 Test #9: test/tarantool-c-tests/misclib-getmetrics-capi.c_test
................... Passed 0.06 sec
100% tests passed, 0 tests failed out of 10
Label Time Summary:
tarantool-c-tests = 0.67 sec*proc (10 tests)
Total Test time (real) = 0.13 sec
[12/15] cd
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/...ctest
-L lua-Harness-tests --output-on-failure --schedule-random --parallel 8
Test project
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/lua-Harness-tests
<snipped>
0/50 Test #33: test/lua-Harness-tests/241-standalone.t .... Passed
1.18 sec [0/6793]
100% tests passed, 0 tests failed out of 50
Label Time Summary:
lua-Harness-tests = 5.22 sec*proc (50 tests)
Total Test time (real) = 1.53 sec
[13/15] cd
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/.../bin/ctest
-L LuaJIT-tests --output-on-failure --schedule-random --parallel 8
Test project
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/LuaJIT-tests
Start 2: test/LuaJIT-tests
1/1 Test #2: test/LuaJIT-tests ................ Passed 3.65 sec
<snipped>
This output looks ugly and is not convenient in daily use. Left it
without changes.
>
>> )
> <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.
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -9,7 +9,7 @@ make_lua_path(LUA_CPATH
)
set(LUAJIT_TESTS_ENV
- "LUA_CPATH=${LUA_CPATH}\;\;"
+ "LUA_CPATH=${LUA_CPATH}"
)
set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:")
>
>> )
>>
>> 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>.
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -32,7 +32,7 @@ foreach(test_path ${tests})
# 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})
+ endif()
set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
add_test(NAME ${test_title}
COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
>
>> + 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?
Fixed:
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}
+ 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}\;"
>
>> + )
>> + set_tests_properties(${test_title} PROPERTIES
>> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
> Typo: The semicolon looks excess here.
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -39,7 +39,7 @@ foreach(test_path ${tests})
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
)
set_tests_properties(${test_title} PROPERTIES
- ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
+ ENVIRONMENT "LUA_PATH=${LUA_PATH}"
LABELS ${TEST_SUITE_NAME}
DEPENDS lua-Harness-tests-deps
)
>
>> + 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 :)
Fixed, thanks!
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -41,7 +41,7 @@ endforeach()
set(TEST_SUITE_NAME "tarantool-c-tests")
# XXX: The call produces both test and target
-# <LuaJIT-tests-deps> as a side effect.
+# <tarantool-c-tests-deps> as a side effect.
add_test_suite_target(tarantool-c-tests
LABEL ${TEST_SUITE_NAME}
DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED}
>
>> +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>.
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -59,7 +59,7 @@ foreach(test_path ${tests})
# 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})
+ endif()
set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
add_test(NAME ${test_title}
COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}
>
>> + 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.
Fixed:
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -54,7 +54,6 @@ 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
>> )
> <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.
Fixed:
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -107,7 +107,7 @@ add_test_suite_target(tarantool-tests
# 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}")
+set(TEST_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)
>
>> +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?
it is better to search test files recursively, no one can guarantee that
files always will be
in a top testsuite dir.
However in lua-Harness suite we cannot use GLOB_RECURSE because suite
has a number of files with equal filenames:
$ find test/lua-Harness-tests/ -name lexico.t
test/lua-Harness-tests/lexico53/lexico.t
test/lua-Harness-tests/lexicojit/lexico.t
test/lua-Harness-tests/lexico52/lexico.t
test/lua-Harness-tests/lexico54/lexico.t
due to this CMake configuration has failed with
"add_test given test NAME "test/lua-Harness-tests/lexico.t" which
already exists in this directory."
>
>> +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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
2024-03-12 7:06 [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches
2024-03-16 17:12 ` Igor Munkin via Tarantool-patches
@ 2024-03-18 15:54 ` Sergey Kaplun via Tarantool-patches
2024-03-20 19:44 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-03-18 15:54 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
Hi, Sergey!
Thanks for the patch!
I'll proceed with a review within the original letter.
I look forward to applying these changes!
I hope we can use the full power of CTest. It will be nice to configure
heavy-memory tests to avoid running them together in parallel and
hanging the system:).
Please, consider my comments 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
> $ 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.
Please, remove the excess amount of spaces (9) in the formatting to make
the commit message fits 72 symbols.
Same for the paragraphs below.
> $ 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`.
Is it possible to introduce a new target like test-all to join them?
>
> 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
Typo: s/Lua/Lua,/
> future we could change these test runners to specify tests from outside,
Typo: s/future/future,/
> and after this we could run tests separately by CTest in these test
Typo: s/this/this,/
> 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
Typo: s/workarounded/is workaraounded/
> for each testsuite that executes a target that builds test dependencies.
>
> 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/.gitignore b/.gitignore
> index dc5ea5fc..dd1f8888 100644
> --- a/.gitignore
> +++ b/.gitignore
<snipped>
> 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."
> +)
Why do we use luajit-main as a target dependency for tests?
Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
in tarantool-c-tests)?
> +
> # 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
> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
> "Lua code need to be run before tests are started."
> )
>
> +# XXX: Enable testing via CTest. Since CTest expects to find a
> +# test file in the build directory root, this command should be in
Nit: I suggest to mentioning that this command generates "test" target.
Something like the following:
| # XXX: Enable testing via CTest. This generates `test` target.
> +# the source directory root (hence, in this CMakeLists.txt).
> +enable_testing()
Should we generate the `test` target only if the `LUAJIT_USE_TEST`
option is set, like it was done before (see relevant changes in the
<test/CMakeLists.txt>)?
> add_subdirectory(test)
>
> # --- Misc rules ---------------------------------------------------------------
> 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
Nit: the comment line is wider than 66 symbols.
> +# and is used by `test` target.
> +set(TEST_FLAGS
Minor: Maybe it is better to name this variable as `CTEST_FLAGS`, to
avoid confusion that these flags are arguments for some internal test
runner?
Unfortunately, this flag does nothing for `make test` command.
Is there the way to fix it?
BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
> + --output-on-failure
> + --schedule-random
> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
I suppose it will be good to add the `--timeout` option here too.
5 minutes as a timeout for the single test sounds more than enough.
> +)
> +if(CMAKE_VERBOSE_MAKEFILE)
> + list(APPEND TEST_FLAGS --verbose)
> +endif()
> +
> +function(add_test_suite_target target)
> + set(prefix ARG)
> + set(noValues)
> + set(singleValues LABEL)
> + set(multiValues DEPENDS)
> +
> + # FIXME: if we update to CMake >= 3.5, can remove this line.
> + include(CMakeParseArguments)
> + cmake_parse_arguments(${prefix}
> + "${noValues}"
> + "${singleValues}"
> + "${multiValues}"
> + ${ARGN})
> +
> + set(_DEPS_TARGET ${target}-deps)
> +
> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
> +
> + add_test(${_DEPS_TARGET}
Should the NAME parameter be ${target} instead? Or, maybe this line
should be ommitted since it is also used for normal test targets with
several tests.
For now `ctest -N` output looks a little bit inconsistent:
| Test project /home/burii/reviews/luajit/ctest
| Test #1: LuaJIT-tests-deps
| Test #2: test/LuaJIT-tests
| Test #3: PUC-Rio-Lua-5.1-tests-deps
| Test #4: test/PUC-Rio-Lua-5.1
| Test #5: lua-Harness-tests-deps
| Test #6: test/lua-Harness-tests/000-sanity.t
| ...
| Test #56: tarantool-c-tests-deps
| Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
| ...
| Test #67: tarantool-tests-deps
| Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
| ...
If tests like the following are needed to run tests, I suggest naming
them like `LuaJIT-tests-build` to avoid confusion.
| Test #1: LuaJIT-tests-deps
Also, as discussed with Sergey offline:
For now, we observe an error since dependencies (C libraries) for tests
are not built if we build LuaJIT and try to run ctest target like the
following (without using make for a specific test target):
| cmake . && make -j
| ctest -L tarantool-tests --verbose
| 68: /home/burii/reviews/luajit/ctest/src/luajit: .../test/tarantool-tests/arm64-ccall-fp-convention.test.lua:4: libfficcall.so: cannot open shared object file: No such file or directory
> + ${CMAKE_COMMAND}
> + --build ${CMAKE_BINARY_DIR}
> + --target ${_DEPS_TARGET}
> + )
> +
> + message(STATUS "Add test suite ${ARG_LABEL}")
Note: If we don't run this function for multi-test targets after
applying the changes required above, this STATUS message should be
copied to the correponding CMakeLists.txt.
> +
> + add_custom_target(${target}
> + COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
> + DEPENDS ${_DEPS_TARGET}
> + )
> +
> + unset(_DEPS_TARGET)
There is no need in `unset()` since we are using `function()` here,
which opens a new scope.
> +endfunction()
> +
> add_subdirectory(LuaJIT-tests)
> add_subdirectory(PUC-Rio-Lua-5.1-tests)
> add_subdirectory(tarantool-c-tests)
> add_subdirectory(tarantool-tests)
>
> -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}
Why do we need to use custom command for this test?
IINM, all "subtargets" are used the same ${TEST_FLAGS}
See also Igor's comment about this test.
> + DEPENDS tarantool-c-tests-deps
> + tarantool-tests-deps
> + lua-Harness-tests-deps
> + PUC-Rio-Lua-5.1-tests-deps
> + LuaJIT-tests-deps
> )
> -
> -if(LUAJIT_USE_TEST)
Within this change the `test` target is always generated.
See the comment above.
> - if(POLICY CMP0037)
<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}\;\;"
> )
These semicolons look excessive, see also Igor's comment.
Side note: I suppose that double quotes are used to avoid escaping
various special symbols or spaces. But for now, all these special cases
of directory names are just not working, so I see nothing crucial in
removing these quotes.
>
> 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
This is not true. After these changes the environment variable
`LD_PRELOAD` is the following:
| LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
But it should be:
| LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
This is why this quotes is used.
For some reason, with the debug flags, paths are separated via \n:
| 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
| 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
So, I got the following error, when I build LuaJIT with GCC and use the
flag `-DLUAJIT_ENABLE_ASAN`:
| AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
| #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69
> + # 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()
>
> @@ -64,12 +61,25 @@ if(LUAJIT_NO_UNWIND)
<snipped>
> +set(TEST_SUITE_NAME "LuaJIT-tests")
> +
> +# XXX: The call produces both test and target
> +# <LuaJIT-tests-deps> as a side effect.
Nit: It looks like the <LuaJIT-tests-deps> quotation may be placed at
the previous line, fitting 66 symbols of the comment width.
> +add_test_suite_target(LuaJIT-tests
> + LABEL ${TEST_SUITE_NAME}
> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
Why don't use `${LUAJIT_TEST_BINARY}` here?
> +)
> +
> +# The test suite has its own test runner
> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
> +# tests one-by-one by CTest.
> +add_test(NAME "test/LuaJIT-tests"
Why don't use just LuaJIT-tests as a test name here?
The same question to the tests below.
> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
> )
> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
> + LABELS ${TEST_SUITE_NAME}
> + ENVIRONMENT "${LUAJIT_TESTS_ENV}"
> + DEPENDS LuaJIT-tests-deps
> +)
> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
> index 2f90da86..f0e639e1 100644
> --- a/test/LuaJIT-tests/src/CMakeLists.txt
> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
> @@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c)
> enable_language(CXX)
> AddTestLib(libcpptest libcpptest.cpp)
>
> -add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS})
> +add_custom_target(LuaJIT-tests-deps-libs DEPENDS ${TESTLIBS})
Why do you prefer to rename this target? I see no conflict between this
name and the previous version.
> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> index 98277f9a..af89bfe5 100644
> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> @@ -34,17 +34,26 @@ 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
> +set(TEST_SUITE_NAME "PUC-Rio-Lua-5.1-tests")
> +
> +# XXX: The call produces both test and target
> +# <PUC-Rio-Lua-5.1-tests-deps> as a side effect.
> +add_test_suite_target(PUC-Rio-Lua-5.1-tests
> + LABEL ${TEST_SUITE_NAME}
> + DEPENDS ${LUAJIT_TEST_DEPS} PUC-Rio-Lua-5.1-tests-prepare
Why don't use `${LUAJIT_TEST_BINARY}` here?
Here and below.
> )
>
> -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
> +# 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 one-by-one by CTest.
> +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}\;\;"
Typo: These semicolons look excessive.
> + LABELS ${TEST_SUITE_NAME}
> + DEPENDS PUC-Rio-Lua-5.1-tests-deps
> +)
>
> # vim: expandtab tabstop=2 shiftwidth=2
> 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
> @@ -4,12 +4,6 @@
<snipped>
> +# XXX: The call produces both test and target
> +# <lua-Harness-tests-deps> as a side effect.
> +add_test_suite_target(lua-Harness-tests
> + LABEL ${TEST_SUITE_NAME}
> + DEPENDS ${LUAJIT_TEST_DEPS}
> )
>
> +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.
Typo: s/By default/By default,/
> + # 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: s/endif (/endif(/
> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
I suppose, that general part "test/" may be ommited from the title.
> + add_test(NAME ${test_title}
> + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
Minor: Looks like misindent.
> + )
> + set_tests_properties(${test_title} PROPERTIES
> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
Minor: 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
> @@ -1,15 +1,4 @@
<snipped>
> # Build libtest.
>
> @@ -49,20 +38,35 @@ foreach(test_source ${tests})
> LIST(APPEND TESTS_COMPILED ${exe})
> endforeach()
>
> -add_custom_target(tarantool-c-tests
> - DEPENDS libluajit libtest ${TESTS_COMPILED}
> -)
> +set(TEST_SUITE_NAME "tarantool-c-tests")
>
> -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}
> +# XXX: The call produces both test and target
> +# <LuaJIT-tests-deps> as a side effect.
Typo: s/LuaJIT-tests-deps/tarantool-c-tests-deps/
> +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
Nit: Missed dot at the end of the sentence.
> +# To workaround this, we glob source files and generated tests
> +# with paths to executable binaries.
> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".
You may use the cycle above, which adds build targets.
> +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: s/endif (/endif(/
> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
I suppose, that general part "test/" may be ommited from the 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 ${TEST_SUITE_NAME}
> + DEPENDS tarantool-c-tests-deps
> + )
> +endforeach()
> 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
> @@ -4,19 +4,13 @@
<snipped>
> -add_custom_target(tarantool-tests
> +add_custom_target(tarantool-tests-deps-libs
Why do you prefer to rename this target? I see no conflict between this
name and the previous version.
> DEPENDS ${LUAJIT_TEST_BINARY}
> )
>
> macro(BuildTestCLib lib sources)
> AddTestLib(${lib} ${sources})
> - add_dependencies(tarantool-tests ${lib})
> + add_dependencies(tarantool-tests-deps-libs ${lib})
> # Add the directory where the lib is built to the list with
> # entries for LUA_CPATH environment variable, so LuaJIT can find
> # and load it. See the comment about extending the list in the
> @@ -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_SOURCE_DIR}/?.lua.
> )
> +
> # Update LUA_CPATH with the library paths collected within
> # <BuildTestLib> macro.
> make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS})
>
<snipped>
> +set(TEST_SUITE_NAME "tarantool-tests")
> +
> +# XXX: The call produces both test and target
> +# <tarantool-tests-deps> as a side effect.
> +add_test_suite_target(tarantool-tests
> + LABEL ${TEST_SUITE_NAME}
> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
> )
>
> +# 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}")
> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
Why do we need GLOB_RECURSE here, instead of GLOB?
> +foreach(test_path ${tests})
> + get_filename_component(test_name ${test_path} NAME)
> + set(test_title "test/tarantool-tests/${test_name}")
I suppose, that general part "test/" may be ommited from the 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 "${TEST_ENV}"
> + LABELS ${TEST_SUITE_NAME}
> + DEPENDS tarantool-tests-deps
> + )
> +endforeach()
> --
> 2.34.1
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
2024-03-18 15:54 ` Sergey Kaplun via Tarantool-patches
@ 2024-03-20 19:44 ` Sergey Bronnikov via Tarantool-patches
2024-03-21 11:17 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-20 19:44 UTC (permalink / raw)
To: Sergey Kaplun, Sergey Bronnikov; +Cc: max.kokryashkin, tarantool-patches
Sergey,
thanks for review!
see my comments below
On 3/18/24 18:54, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> I'll proceed with a review within the original letter.
>
> I look forward to applying these changes!
> I hope we can use the full power of CTest. It will be nice to configure
> heavy-memory tests to avoid running them together in parallel and
> hanging the system:).
>
> Please, consider my comments 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
>> $ 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.
> Please, remove the excess amount of spaces (9) in the formatting to make
> the commit message fits 72 symbols.
Fixed.
>
> Same for the paragraphs below.
Fixed.
>> $ 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`.
> Is it possible to introduce a new target like test-all to join them?
Discussed target name in a private conversation and decided to introduce
"LuaJIT-check-all".
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -140,3 +140,8 @@ add_custom_target(${PROJECT_NAME}-test
PUC-Rio-Lua-5.1-tests-deps
LuaJIT-tests-deps
)
+
+add_custom_target(${PROJECT_NAME}-check-all
+ DEPENDS ${PROJECT_NAME}-test
+ ${PROJECT_NAME}-lint
+)
>
>> 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
> Typo: s/Lua/Lua,/
>
Added comma.
>> future we could change these test runners to specify tests from outside,
> Typo: s/future/future,/
Added comma.
>
>> and after this we could run tests separately by CTest in these test
> Typo: s/this/this,/
Added comma.
>
>> 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
> Typo: s/workarounded/is workaraounded/
Typo is in "workaraounded".
Fixed.
>
>> for each testsuite that executes a target that builds test dependencies.
>>
>> 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/.gitignore b/.gitignore
>> index dc5ea5fc..dd1f8888 100644
>> --- a/.gitignore
>> +++ b/.gitignore
> <snipped>
>
>> 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."
>> +)
> Why do we use luajit-main as a target dependency for tests?
> Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
> in tarantool-c-tests)?
>
>> +
>> # 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
>> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>> "Lua code need to be run before tests are started."
>> )
>>
>> +# XXX: Enable testing via CTest. Since CTest expects to find a
>> +# test file in the build directory root, this command should be in
> Nit: I suggest to mentioning that this command generates "test" target.
> Something like the following:
> | # XXX: Enable testing via CTest. This generates `test` target.
Added:
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT
"${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
"Lua code need to be run before tests are started."
)
-# XXX: Enable testing via CTest. Since CTest expects to find a
-# test file in the build directory root, this command should be in
-# the source directory root (hence, in this CMakeLists.txt).
+# XXX: Enable testing via CTest. This automatically generates
+# `test` target. Since CTest expects to find a test file in
+# the build directory root, this command should be in the source
+# directory root (hence, in this CMakeLists.txt).
enable_testing()
add_subdirectory(test)
>
>> +# the source directory root (hence, in this CMakeLists.txt).
>> +enable_testing()
> Should we generate the `test` target only if the `LUAJIT_USE_TEST`
> option is set, like it was done before (see relevant changes in the
> <test/CMakeLists.txt>)?
Added option LUAJIT_USE_TEST back:
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -373,7 +373,9 @@ set(LUAJIT_TEST_INIT
"${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
# `test` target. Since CTest expects to find a test file in
# the build directory root, this command should be in the source
# directory root (hence, in this CMakeLists.txt).
-enable_testing()
+if (LUAJIT_USE_TEST)
+ enable_testing()
+endif()
add_subdirectory(test)
# --- Misc rules
---------------------------------------------------------------
>> add_subdirectory(test)
>>
>> # --- Misc rules ---------------------------------------------------------------
>> 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
> Nit: the comment line is wider than 66 symbols.
Already fixed.
>
>> +# and is used by `test` target.
>> +set(TEST_FLAGS
> Minor: Maybe it is better to name this variable as `CTEST_FLAGS`, to
> avoid confusion that these flags are arguments for some internal test
> runner?
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 899bf31a..ca298118 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -77,16 +77,16 @@ 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.
+# CTEST_FLAGS is used by CMake targets in test suites.
# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains
# CTest options and is used by `test` target.
-set(TEST_FLAGS
+set(CTEST_FLAGS
--output-on-failure
--schedule-random
--parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
)
if(CMAKE_VERBOSE_MAKEFILE)
- list(APPEND TEST_FLAGS --verbose)
+ list(APPEND CTEST_FLAGS --verbose)
endif()
function(add_test_suite_target target)
@@ -119,7 +119,7 @@ function(add_test_suite_target target)
message(STATUS "Add test suite ${ARG_LABEL}")
add_custom_target(${target}
- COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
+ COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${CTEST_FLAGS}
DEPENDS ${_DEPS_TARGET}
)
@@ -133,7 +133,7 @@ add_subdirectory(tarantool-c-tests)
add_subdirectory(tarantool-tests)
add_custom_target(${PROJECT_NAME}-test
- COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}
+ COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}
DEPENDS tarantool-c-tests-deps
tarantool-tests-deps
lua-Harness-tests-deps
>
> Unfortunately, this flag does nothing for `make test` command.
> Is there the way to fix it?
Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that
contains
CTest options and is used by `test` target. As a comment says.
BTW you can fix it by using CMake workflows and specify any CTest
arguments you want in CMakePreset.json.
> BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
Sure, these are our own targets and we pass desired CTest options to it.
>> + --output-on-failure
>> + --schedule-random
>> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
> I suppose it will be good to add the `--timeout` option here too.
> 5 minutes as a timeout for the single test sounds more than enough.
>
Added.
>> +)
>> +if(CMAKE_VERBOSE_MAKEFILE)
>> + list(APPEND TEST_FLAGS --verbose)
>> +endif()
>> +
>> +function(add_test_suite_target target)
>> + set(prefix ARG)
>> + set(noValues)
>> + set(singleValues LABEL)
>> + set(multiValues DEPENDS)
>> +
>> + # FIXME: if we update to CMake >= 3.5, can remove this line.
>> + include(CMakeParseArguments)
>> + cmake_parse_arguments(${prefix}
>> + "${noValues}"
>> + "${singleValues}"
>> + "${multiValues}"
>> + ${ARGN})
>> +
>> + set(_DEPS_TARGET ${target}-deps)
>> +
>> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
>> +
>> + add_test(${_DEPS_TARGET}
> Should the NAME parameter be ${target} instead?
Added.
> Or, maybe this line
> should be ommitted since it is also used for normal test targets with
> several tests.
CMake test for a target with dependencies must be generated,
otherwise dependencies will not be built.
>
> For now `ctest -N` output looks a little bit inconsistent:
>
> | Test project /home/burii/reviews/luajit/ctest
> | Test #1: LuaJIT-tests-deps
> | Test #2: test/LuaJIT-tests
> | Test #3: PUC-Rio-Lua-5.1-tests-deps
> | Test #4: test/PUC-Rio-Lua-5.1
> | Test #5: lua-Harness-tests-deps
> | Test #6: test/lua-Harness-tests/000-sanity.t
> | ...
> | Test #56: tarantool-c-tests-deps
> | Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
> | ...
> | Test #67: tarantool-tests-deps
> | Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
> | ...
>
> If tests like the following are needed to run tests, I suggest naming
> them like `LuaJIT-tests-build` to avoid confusion.
AFAIR this naming was suggested by Igor.
What kind of confusion you observe? For me something like
`LuaJIT-tests-build` much more confusing than "xxx-deps".
> | Test #1: LuaJIT-tests-deps
>
> Also, as discussed with Sergey offline:
> For now, we observe an error since dependencies (C libraries) for tests
> are not built if we build LuaJIT and try to run ctest target like the
> following (without using make for a specific test target):
>
> | cmake . && make -j
> | ctest -L tarantool-tests --verbose
> | 68: /home/burii/reviews/luajit/ctest/src/luajit: .../test/tarantool-tests/arm64-ccall-fp-convention.test.lua:4: libfficcall.so: cannot open shared object file: No such file or directory
>
it was fixed
>> + ${CMAKE_COMMAND}
>> + --build ${CMAKE_BINARY_DIR}
>> + --target ${_DEPS_TARGET}
>> + )
>> +
>> + message(STATUS "Add test suite ${ARG_LABEL}")
> Note: If we don't run this function for multi-test targets after
> applying the changes required above, this STATUS message should be
> copied to the correponding CMakeLists.txt.
This macro executed one time for each test suite.
>
>> +
>> + add_custom_target(${target}
>> + COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
>> + DEPENDS ${_DEPS_TARGET}
>> + )
>> +
>> + unset(_DEPS_TARGET)
> There is no need in `unset()` since we are using `function()` here,
> which opens a new scope.
Removed.
>
>> +endfunction()
>> +
>> add_subdirectory(LuaJIT-tests)
>> add_subdirectory(PUC-Rio-Lua-5.1-tests)
>> add_subdirectory(tarantool-c-tests)
>> add_subdirectory(tarantool-tests)
>>
>> -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}
> Why do we need to use custom command for this test?
> IINM, all "subtargets" are used the same ${TEST_FLAGS}
> See also Igor's comment about this test.
Answered to Igor.
because output will be ugly in that case,
just change as you suggest and run LuaJIT-test.
>
>> + DEPENDS tarantool-c-tests-deps
>> + tarantool-tests-deps
>> + lua-Harness-tests-deps
>> + PUC-Rio-Lua-5.1-tests-deps
>> + LuaJIT-tests-deps
>> )
>> -
>> -if(LUAJIT_USE_TEST)
> Within this change the `test` target is always generated.
> See the comment above.
Generated when LUAJIT_USE_TEST is ON.
>
>> - if(POLICY CMP0037)
> <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}\;\;"
>> )
> These semicolons look excessive, see also Igor's comment.
>
> Side note: I suppose that double quotes are used to avoid escaping
> various special symbols or spaces. But for now, all these special cases
> of directory names are just not working, so I see nothing crucial in
> removing these quotes.
already fixed and force-pushed
>>
>> 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
> This is not true. After these changes the environment variable
> `LD_PRELOAD` is the following:
> | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
> But it should be:
> | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
> This is why this quotes is used.
> For some reason, with the debug flags, paths are separated via \n:
> | 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
> | 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
>
> So, I got the following error, when I build LuaJIT with GCC and use the
> flag `-DLUAJIT_ENABLE_ASAN`:
> | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
> | #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69
>
Fixed.
Unfortunately there is a yet another test that will never worked in
configuration GCC + ASAN.
Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to
fix it, but as
it was never worked before we don't need to do it in scope of ctest support.
diff --git a/test/LuaJIT-tests/CMakeLists.txt
b/test/LuaJIT-tests/CMakeLists.txt
index 0c85f15c..4c9cf901 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)
# https://github.com/google/sanitizers/issues/934.
macro(LibRealPath output lib)
execute_process(
- COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
+ COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
OUTPUT_VARIABLE LIB_LINK
OUTPUT_STRIP_TRAILING_WHITESPACE
+ TIMEOUT 5
+ RESULT_VARIABLE RES
)
+ if (${LIB_LINK} STREQUAL ${lib})
+ message(FATAL_ERROR "library ${lib} is not found")
+ endif()
+ if (NOT RES EQUAL 0)
+ message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed")
+ endif()
+
# Fortunately, we are not interested in macOS here, so we can
# use realpath.
+ # Beware, builtin command returns exit code equal to 0,
+ # we cannot check is it failed or not.
execute_process(
COMMAND realpath ${LIB_LINK}
OUTPUT_VARIABLE ${output}
OUTPUT_STRIP_TRAILING_WHITESPACE
+ TIMEOUT 5
)
endmacro()
LibRealPath(LIB_STDCPP libstdc++.so)
- # XXX: GCC requires both. Clang requires only libstdc++.
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}")
+ # XXX: The items of the list in LD_PRELOAD can be separated
+ # by spaces or colons, and there is no support for escaping
+ # either separator, see ld.so(8).
+ 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()
>> + # 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()
>>
>> @@ -64,12 +61,25 @@ if(LUAJIT_NO_UNWIND)
> <snipped>
>
>> +set(TEST_SUITE_NAME "LuaJIT-tests")
>> +
>> +# XXX: The call produces both test and target
>> +# <LuaJIT-tests-deps> as a side effect.
> Nit: It looks like the <LuaJIT-tests-deps> quotation may be placed at
> the previous line, fitting 66 symbols of the comment width.
Ok, added to the previous line
>> +add_test_suite_target(LuaJIT-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
> Why don't use `${LUAJIT_TEST_BINARY}` here?
AFAIR we decided with Igor in a verbal conversation
to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}.
LUAJIT_TEST_BINARY is a single target that can be used as a command,
see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a
list of targets
that should be build before running tests.
>
>> +)
>> +
>> +# The test suite has its own test runner
>> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
>> +# tests one-by-one by CTest.
>> +add_test(NAME "test/LuaJIT-tests"
> Why don't use just LuaJIT-tests as a test name here?
For consistency with other test titles.
> The same question to the tests below.
>
>> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
>> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
>> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
>> )
>> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
>> + LABELS ${TEST_SUITE_NAME}
>> + ENVIRONMENT "${LUAJIT_TESTS_ENV}"
>> + DEPENDS LuaJIT-tests-deps
>> +)
>> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
>> index 2f90da86..f0e639e1 100644
>> --- a/test/LuaJIT-tests/src/CMakeLists.txt
>> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
>> @@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c)
>> enable_language(CXX)
>> AddTestLib(libcpptest libcpptest.cpp)
>>
>> -add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS})
>> +add_custom_target(LuaJIT-tests-deps-libs DEPENDS ${TESTLIBS})
> Why do you prefer to rename this target? I see no conflict between this
> name and the previous version.
Seems this a change made by Igor. I'm totally ok with a previous name.
reverted
>
>> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
>> index 98277f9a..af89bfe5 100644
>> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
>> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
>> @@ -34,17 +34,26 @@ 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
>> +set(TEST_SUITE_NAME "PUC-Rio-Lua-5.1-tests")
>> +
>> +# XXX: The call produces both test and target
>> +# <PUC-Rio-Lua-5.1-tests-deps> as a side effect.
>> +add_test_suite_target(PUC-Rio-Lua-5.1-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} PUC-Rio-Lua-5.1-tests-prepare
> Why don't use `${LUAJIT_TEST_BINARY}` here?
> Here and below.
the same as above
>
>> )
>>
>> -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
>> +# 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 one-by-one by CTest.
>> +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}\;\;"
> Typo: These semicolons look excessive.
Removed.
>
>> + LABELS ${TEST_SUITE_NAME}
>> + DEPENDS PUC-Rio-Lua-5.1-tests-deps
>> +)
>>
>> # vim: expandtab tabstop=2 shiftwidth=2
>> 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
>> @@ -4,12 +4,6 @@
> <snipped>
>
>> +# XXX: The call produces both test and target
>> +# <lua-Harness-tests-deps> as a side effect.
>> +add_test_suite_target(lua-Harness-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS}
>> )
>>
>> +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.
> Typo: s/By default/By default,/
Fixed.
>
>> + # 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: s/endif (/endif(/
Already fixed.
>
>> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
> I suppose, that general part "test/" may be ommited from the title.
I would left as is. The main idea is to map test title to a file/dir on
the filesystem.
>
>> + add_test(NAME ${test_title}
>> + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
>> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> Minor: Looks like misindent.
It is ok in a current version.
>
>> + )
>> + set_tests_properties(${test_title} PROPERTIES
>> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
> Minor: Semicolon looks excess here.
It is ok in a current version.
>
>> + 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
>> @@ -1,15 +1,4 @@
> <snipped>
>
>> # Build libtest.
>>
>> @@ -49,20 +38,35 @@ foreach(test_source ${tests})
>> LIST(APPEND TESTS_COMPILED ${exe})
>> endforeach()
>>
>> -add_custom_target(tarantool-c-tests
>> - DEPENDS libluajit libtest ${TESTS_COMPILED}
>> -)
>> +set(TEST_SUITE_NAME "tarantool-c-tests")
>>
>> -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}
>> +# XXX: The call produces both test and target
>> +# <LuaJIT-tests-deps> as a side effect.
> Typo: s/LuaJIT-tests-deps/tarantool-c-tests-deps/
It is ok in a current version.
>> +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
> Nit: Missed dot at the end of the sentence.
Added a dot to the end of sentence.
>
>> +# To workaround this, we glob source files and generated tests
>> +# with paths to executable binaries.
>> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
> Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".
>
> You may use the cycle above, which adds build targets.
This is a good idea, but anyway we need two loops.
First should build a list TESTS_COMPILED that is used in
add_test_suite_target.
The second one should generate CMake tests.
If you are still want to have a single loop for everything then
we need to build -deps target and CMake test for each tarantool C test.
Left without changes.
>
>> +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: s/endif (/endif(/
this is ok in a current version
>
>> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
> I suppose, that general part "test/" may be ommited from the title.
No, explained above.
>
>> + 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 ${TEST_SUITE_NAME}
>> + DEPENDS tarantool-c-tests-deps
>> + )
>> +endforeach()
>> 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
>> @@ -4,19 +4,13 @@
> <snipped>
>
>> -add_custom_target(tarantool-tests
>> +add_custom_target(tarantool-tests-deps-libs
> Why do you prefer to rename this target? I see no conflict between this
> name and the previous version.
As I said before I'm totally ok with old name.
The change was introduced when "Igor automatically squashed all the
tweaks made to the original commit."
Reverted
>
>> DEPENDS ${LUAJIT_TEST_BINARY}
>> )
>>
>> macro(BuildTestCLib lib sources)
>> AddTestLib(${lib} ${sources})
>> - add_dependencies(tarantool-tests ${lib})
>> + add_dependencies(tarantool-tests-deps-libs ${lib})
>> # Add the directory where the lib is built to the list with
>> # entries for LUA_CPATH environment variable, so LuaJIT can find
>> # and load it. See the comment about extending the list in the
>> @@ -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_SOURCE_DIR}/?.lua.
fixed in a current version
>
>> )
>> +
>> # Update LUA_CPATH with the library paths collected within
>> # <BuildTestLib> macro.
>> make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS})
>>
> <snipped>
>
>> +set(TEST_SUITE_NAME "tarantool-tests")
>> +
>> +# XXX: The call produces both test and target
>> +# <tarantool-tests-deps> as a side effect.
>> +add_test_suite_target(tarantool-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
>> )
>>
>> +# 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}")
>> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> Why do we need GLOB_RECURSE here, instead of GLOB?
it is better to search test files recursively, no one can guarantee that
files always will be in a top testsuite dir.
>
>> +foreach(test_path ${tests})
>> + get_filename_component(test_name ${test_path} NAME)
>> + set(test_title "test/tarantool-tests/${test_name}")
> I suppose, that general part "test/" may be ommited from the title.
answered above
>
>> + 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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
2024-03-20 19:44 ` Sergey Bronnikov via Tarantool-patches
@ 2024-03-21 11:17 ` Sergey Kaplun via Tarantool-patches
2024-03-22 13:06 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-03-21 11:17 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: Sergey Bronnikov, max.kokryashkin, tarantool-patches
Hi, Sergey!
Thanks for the fixes!
Please consider my concerns below.
I suggest moving our conversation forward with the new patch version.
On 20.03.24, Sergey Bronnikov wrote:
> Sergey,
>
> thanks for review!
>
> see my comments below
>
> On 3/18/24 18:54, Sergey Kaplun wrote:
<snipped>
> >> 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/.gitignore b/.gitignore
> >> index dc5ea5fc..dd1f8888 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> > <snipped>
> >
> >> 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."
> >> +)
> > Why do we use luajit-main as a target dependency for tests?
> > Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
> > in tarantool-c-tests)?
See my comment below.
> >
> >> +
> >> # 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
> >> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
> >> "Lua code need to be run before tests are started."
> >> )
> >>
> >> +# XXX: Enable testing via CTest. Since CTest expects to find a
> >> +# test file in the build directory root, this command should be in
> > Nit: I suggest to mentioning that this command generates "test" target.
> > Something like the following:
> > | # XXX: Enable testing via CTest. This generates `test` target.
>
> Added:
>
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT
> "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
> "Lua code need to be run before tests are started."
> )
>
> -# XXX: Enable testing via CTest. Since CTest expects to find a
> -# test file in the build directory root, this command should be in
> -# the source directory root (hence, in this CMakeLists.txt).
> +# XXX: Enable testing via CTest. This automatically generates
> +# `test` target. Since CTest expects to find a test file in
Typo: s/`test`/the `test`/
> +# the build directory root, this command should be in the source
> +# directory root (hence, in this CMakeLists.txt).
> enable_testing()
> add_subdirectory(test)
>
>
> >
<snipped>
> > Unfortunately, this flag does nothing for `make test` command.
> > Is there the way to fix it?
>
> Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that
> contains
> CTest options and is used by `test` target. As a comment says.
>
> BTW you can fix it by using CMake workflows and specify any CTest
> arguments you want in CMakePreset.json.
I got it thanks for the clarification!
May you please add a comment with the "FIXME:" mark and mention the
CMake version and the CMAKE_CTEST_ARGUMENTS variable?
>
> > BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
> Sure, these are our own targets and we pass desired CTest options to it.
> >> + --output-on-failure
> >> + --schedule-random
> >> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
> > I suppose it will be good to add the `--timeout` option here too.
> > 5 minutes as a timeout for the single test sounds more than enough.
> >
> Added.
Unfortunately, as you mentioned offline, for some builds [1], even 5
minutes is not enough:
| 211/211 Test #109: test/tarantool-tests/lj-1034-tabov-error-frame.test.lua ........................***Timeout 300.54 sec
I suggest increasing the timeout to 30 minutes.
> >> +)
> >> +if(CMAKE_VERBOSE_MAKEFILE)
> >> + list(APPEND TEST_FLAGS --verbose)
> >> +endif()
> >> +
> >> +function(add_test_suite_target target)
> >> + set(prefix ARG)
> >> + set(noValues)
> >> + set(singleValues LABEL)
> >> + set(multiValues DEPENDS)
> >> +
> >> + # FIXME: if we update to CMake >= 3.5, can remove this line.
> >> + include(CMakeParseArguments)
> >> + cmake_parse_arguments(${prefix}
> >> + "${noValues}"
> >> + "${singleValues}"
> >> + "${multiValues}"
> >> + ${ARGN})
> >> +
> >> + set(_DEPS_TARGET ${target}-deps)
> >> +
> >> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
> >> +
> >> + add_test(${_DEPS_TARGET}
> > Should the NAME parameter be ${target} instead?
>
> Added.
Sorry, I've misguided you with my comment. I thought that we should name
the test as "${target}", not "${_DEPS_TARGET}" (I hadn't gotten the idea
of proxy targets before, but I'm OK with it now).
Please revert this change.
My bad.
Also, please add the comment that this proxy test allows us to be sure that
all prerequisites must be built or generated before the main test
execution.
>
>
> > Or, maybe this line
> > should be ommitted since it is also used for normal test targets with
> > several tests.
>
> CMake test for a target with dependencies must be generated,
>
> otherwise dependencies will not be built.
>
> >
> > For now `ctest -N` output looks a little bit inconsistent:
> >
> > | Test project /home/burii/reviews/luajit/ctest
> > | Test #1: LuaJIT-tests-deps
> > | Test #2: test/LuaJIT-tests
> > | Test #3: PUC-Rio-Lua-5.1-tests-deps
> > | Test #4: test/PUC-Rio-Lua-5.1
> > | Test #5: lua-Harness-tests-deps
> > | Test #6: test/lua-Harness-tests/000-sanity.t
> > | ...
> > | Test #56: tarantool-c-tests-deps
> > | Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
> > | ...
> > | Test #67: tarantool-tests-deps
> > | Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
> > | ...
> >
> > If tests like the following are needed to run tests, I suggest naming
> > them like `LuaJIT-tests-build` to avoid confusion.
>
> AFAIR this naming was suggested by Igor.
>
> What kind of confusion you observe? For me something like
That we are really testing something with this strange name.
OK, my bad ignore it.
>
> `LuaJIT-tests-build` much more confusing than "xxx-deps".
>
> > | Test #1: LuaJIT-tests-deps
OK, let's leave it as it is.
> >
<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}
> > Why do we need to use custom command for this test?
> > IINM, all "subtargets" are used the same ${TEST_FLAGS}
> > See also Igor's comment about this test.
>
> Answered to Igor.
>
> because output will be ugly in that case,
>
> just change as you suggest and run LuaJIT-test.
Please, mention the desired output format in the comment.
>
> >
<snipped>
> >>
> >> 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
> > This is not true. After these changes the environment variable
> > `LD_PRELOAD` is the following:
> > | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
> > But it should be:
> > | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
> > This is why this quotes is used.
> > For some reason, with the debug flags, paths are separated via \n:
> > | 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
> > | 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
> >
> > So, I got the following error, when I build LuaJIT with GCC and use the
> > flag `-DLUAJIT_ENABLE_ASAN`:
> > | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
> > | #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69
> >
> Fixed.
>
> Unfortunately there is a yet another test that will never worked in
> configuration GCC + ASAN.
>
> Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to
> fix it, but as
>
> it was never worked before we don't need to do it in scope of ctest support.
It works in our CI, where we use the clang build.
For now, we may just disable this test for GCC + clang build with the
link to the issue.
On branch, you're using global LD_PRELOAD from the aforementioned test
(lj-802-panic-at-mcode-protfail.test.lua). As a result, tons of tests
fail with the following error:
| got: PANIC: unprotected error in call to Lua API (runtime code generation failed, restricted kernel?)
May be it is better to proxy parent LD_PRELOAD to the test instead?
> diff --git a/test/LuaJIT-tests/CMakeLists.txt
> b/test/LuaJIT-tests/CMakeLists.txt
> index 0c85f15c..4c9cf901 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)
> # https://github.com/google/sanitizers/issues/934.
> macro(LibRealPath output lib)
> execute_process(
> - COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
> + COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
Please mention in the comment that the C++ and C compilers use the same
tooling, so the returned library path is the same.
> OUTPUT_VARIABLE LIB_LINK
> OUTPUT_STRIP_TRAILING_WHITESPACE
> + TIMEOUT 5
Why do we need timeout here? (*)
> + RESULT_VARIABLE RES
> )
> + if (${LIB_LINK} STREQUAL ${lib})
> + message(FATAL_ERROR "library ${lib} is not found")
> + endif()
> + if (NOT RES EQUAL 0)
> + message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed")
Nit: the message is not very informative. I still don't know how to
rewrite it, since compilers just print the input for unexisting files.
Hence, I am not sure: do we really need this error message, since the
given command is valid?
> + endif()
> +
> # Fortunately, we are not interested in macOS here, so we can
> # use realpath.
> + # Beware, builtin command returns exit code equal to 0,
> + # we cannot check is it failed or not.
> execute_process(
> COMMAND realpath ${LIB_LINK}
> OUTPUT_VARIABLE ${output}
> OUTPUT_STRIP_TRAILING_WHITESPACE
> + TIMEOUT 5
(*) And here?
> )
> endmacro()
> LibRealPath(LIB_STDCPP libstdc++.so)
> - # XXX: GCC requires both. Clang requires only libstdc++.
> 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}")
> + # XXX: The items of the list in LD_PRELOAD can be separated
> + # by spaces or colons, and there is no support for escaping
> + # either separator, see ld.so(8).
> + 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()
Minor: Maybe it is better to move this change to a separate patch.
Feel free to ignore.
<snipped>
> >> +add_test_suite_target(LuaJIT-tests
> >> + LABEL ${TEST_SUITE_NAME}
> >> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
> > Why don't use `${LUAJIT_TEST_BINARY}` here?
>
>
> AFAIR we decided with Igor in a verbal conversation
>
> to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}.
>
> LUAJIT_TEST_BINARY is a single target that can be used as a command,
>
> see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a
> list of targets
>
> that should be build before running tests.
Yes, but we are now setting LUAJIT_TEST_DEPS unconditionally.
Also, as I mentioned above for tarantool-c-tests, there is no need to
build the luajit-main target -- libluajit is enough. Hence, this change
may be dropped.
CC imun@ here.
>
> >
> >> +)
> >> +
> >> +# The test suite has its own test runner
> >> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
> >> +# tests one-by-one by CTest.
> >> +add_test(NAME "test/LuaJIT-tests"
> > Why don't use just LuaJIT-tests as a test name here?
> For consistency with other test titles.
Side note: It looks inconsistent anyway (partially):
| Start 1: LuaJIT-tests-deps
| 1/211 Test #1: LuaJIT-tests-deps .............................................................. Passed 0.29 sec
| Start 2: test/LuaJIT-tests
| 2/211 Test #2: test/LuaJIT-tests .............................................................. Passed 3.56 sec
| Start 3: PUC-Rio-Lua-5.1-tests-deps
| 3/211 Test #3: PUC-Rio-Lua-5.1-tests-deps ..................................................... Passed 0.35 sec
| Start 4: test/PUC-Rio-Lua-5.1
As you can see, *-deps targets have no test in their name.
But I can't come up with something meaningful, so feel free to ignore
it.
> > The same question to the tests below.
> >
> >> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
> >> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
> >> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
> >> )
> >> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
> >> + LABELS ${TEST_SUITE_NAME}
> >> + ENVIRONMENT "${LUAJIT_TESTS_ENV}"
> >> + DEPENDS LuaJIT-tests-deps
> >> +)
> >> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
> >> index 2f90da86..f0e639e1 100644
> >> --- a/test/LuaJIT-tests/src/CMakeLists.txt
> >> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
<snipped>
> >> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
> > I suppose, that general part "test/" may be ommited from the title.
> I would left as is. The main idea is to map test title to a file/dir on
> the filesystem.
> >
It's kind of obvious to me that tests are placed in the directory
"test". I'm not insisting, but the names of the tests are already long
enough, it will be nicer to decrease their length with unnecessary
information. The "test" directory is still the root directory for all
tests.
OTOH, it is more convenient to copy-paste the filenames, but for sure, we
must use the whole filename to be consistent with the prove's output we
are replacing.
<snipped>
> >> +# To workaround this, we glob source files and generated tests
> >> +# with paths to executable binaries.
> >> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
> > Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".
> >
> > You may use the cycle above, which adds build targets.
>
> This is a good idea, but anyway we need two loops.
>
> First should build a list TESTS_COMPILED that is used in
> add_test_suite_target.
>
> The second one should generate CMake tests.
>
> If you are still want to have a single loop for everything then
>
> we need to build -deps target and CMake test for each tarantool C test.
Shouldn't it be enough to set
| DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${exe}
for each test? But, yes, in this way, the order of compilation and
execution isn't sequential...
Maybe we should use the proxy target here. In the loop, we append a
dependency for this target and set it as a dependency for each test
target. May it work?
>
> Left without changes.
May you please add a comment about it to avoid confusion in the future?
>
<snipped>
> >> +set(TEST_SUITE_NAME "tarantool-tests")
> >> +
> >> +# XXX: The call produces both test and target
> >> +# <tarantool-tests-deps> as a side effect.
> >> +add_test_suite_target(tarantool-tests
> >> + LABEL ${TEST_SUITE_NAME}
> >> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
> >> )
> >>
> >> +# 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}")
> >> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> > Why do we need GLOB_RECURSE here, instead of GLOB?
>
>
> it is better to search test files recursively, no one can guarantee that
> files always will be in a top testsuite dir.
Ok, thanks for the clarification.
<snipped>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
2024-03-21 11:17 ` Sergey Kaplun via Tarantool-patches
@ 2024-03-22 13:06 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-22 13:06 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: Sergey Bronnikov, max.kokryashkin, tarantool-patches
Sergey,
thanks for the answers. See my comments.
On 3/21/24 14:17, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> Please consider my concerns below.
> I suggest moving our conversation forward with the new patch version.
>
> On 20.03.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>> thanks for review!
>>
>> see my comments below
>>
>> On 3/18/24 18:54, Sergey Kaplun wrote:
> <snipped>
>
>>>> 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/.gitignore b/.gitignore
>>>> index dc5ea5fc..dd1f8888 100644
>>>> --- a/.gitignore
>>>> +++ b/.gitignore
>>> <snipped>
>>>
>>>> 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."
>>>> +)
>>> Why do we use luajit-main as a target dependency for tests?
>>> Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
>>> in tarantool-c-tests)?
> See my comment below.
>
>>>> +
>>>> # 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
>>>> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>>>> "Lua code need to be run before tests are started."
>>>> )
>>>>
>>>> +# XXX: Enable testing via CTest. Since CTest expects to find a
>>>> +# test file in the build directory root, this command should be in
>>> Nit: I suggest to mentioning that this command generates "test" target.
>>> Something like the following:
>>> | # XXX: Enable testing via CTest. This generates `test` target.
>> Added:
>>
>> --- a/CMakeLists.txt
>> +++ b/CMakeLists.txt
>> @@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT
>> "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>> "Lua code need to be run before tests are started."
>> )
>>
>> -# XXX: Enable testing via CTest. Since CTest expects to find a
>> -# test file in the build directory root, this command should be in
>> -# the source directory root (hence, in this CMakeLists.txt).
>> +# XXX: Enable testing via CTest. This automatically generates
>> +# `test` target. Since CTest expects to find a test file in
> Typo: s/`test`/the `test`/
Fixed.
>
>> +# the build directory root, this command should be in the source
>> +# directory root (hence, in this CMakeLists.txt).
>> enable_testing()
>> add_subdirectory(test)
>>
>>
> <snipped>
>
>>> Unfortunately, this flag does nothing for `make test` command.
>>> Is there the way to fix it?
>> Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that
>> contains
>> CTest options and is used by `test` target. As a comment says.
>>
>> BTW you can fix it by using CMake workflows and specify any CTest
>> arguments you want in CMakePreset.json.
> I got it thanks for the clarification!
> May you please add a comment with the "FIXME:" mark and mention the
> CMake version and the CMAKE_CTEST_ARGUMENTS variable?
Already there, see v4.
>>> BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
>> Sure, these are our own targets and we pass desired CTest options to it.
>>>> + --output-on-failure
>>>> + --schedule-random
>>>> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
>>> I suppose it will be good to add the `--timeout` option here too.
>>> 5 minutes as a timeout for the single test sounds more than enough.
>>>
>> Added.
> Unfortunately, as you mentioned offline, for some builds [1], even 5
> minutes is not enough:
> | 211/211 Test #109: test/tarantool-tests/lj-1034-tabov-error-frame.test.lua ........................***Timeout 300.54 sec
> I suggest increasing the timeout to 30 minutes.
>
Already updated and added comment with explanation.
>>>> +)
>>>> +if(CMAKE_VERBOSE_MAKEFILE)
>>>> + list(APPEND TEST_FLAGS --verbose)
>>>> +endif()
>>>> +
>>>> +function(add_test_suite_target target)
>>>> + set(prefix ARG)
>>>> + set(noValues)
>>>> + set(singleValues LABEL)
>>>> + set(multiValues DEPENDS)
>>>> +
>>>> + # FIXME: if we update to CMake >= 3.5, can remove this line.
>>>> + include(CMakeParseArguments)
>>>> + cmake_parse_arguments(${prefix}
>>>> + "${noValues}"
>>>> + "${singleValues}"
>>>> + "${multiValues}"
>>>> + ${ARGN})
>>>> +
>>>> + set(_DEPS_TARGET ${target}-deps)
>>>> +
>>>> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
>>>> +
>>>> + add_test(${_DEPS_TARGET}
>>> Should the NAME parameter be ${target} instead?
>> Added.
> Sorry, I've misguided you with my comment. I thought that we should name
> the test as "${target}", not "${_DEPS_TARGET}" (I hadn't gotten the idea
> of proxy targets before, but I'm OK with it now).
> Please revert this change.
> My bad.
Actually nothing bad is happen.
Change below was made after your comment:
@@ -107,10 +114,10 @@ function(add_test_suite_target target)
add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
- add_test(${_DEPS_TARGET}
- ${CMAKE_COMMAND}
- --build ${CMAKE_BINARY_DIR}
- --target ${_DEPS_TARGET}
+ add_test(NAME ${_DEPS_TARGET}
+ COMMAND ${CMAKE_COMMAND}
+ --build ${CMAKE_BINARY_DIR}
+ --target ${_DEPS_TARGET}
)
set_tests_properties(${_DEPS_TARGET} PROPERTIES
LABELS ${target}
Seems according to BNF in [1] `NAME` is required before test name in
`add_test`.
But in fact `NAME` can be omitted. I would left `NAME` to be consistent
with CMake documentation.
1. https://cmake.org/cmake/help/latest/command/add_test.html
> Also, please add the comment that this proxy test allows us to be sure that
> all prerequisites must be built or generated before the main test
> execution.
Added.
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -96,6 +96,17 @@ if(CMAKE_VERBOSE_MAKEFILE)
list(APPEND CTEST_FLAGS --verbose)
endif()
+# It is not possible to add dependencies to `add_test()`
+# in CMake, see [1]. CMake 3.7 introduces FIXTURES_REQUIRED [2]
+# and FIXTURES_SETUP [3], but these test properties cannot be
+# used - it is unsupported in a current CMake version.
+# To workaround this a function `add_test_suite_target` is
+# introduced, it adds a CMake target that builds testsuite's
+# prerequisites and CMake test that executes that target.
+#
+# 1. https://gitlab.kitware.com/cmake/cmake/-/issues/8774
+# 2. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html
+# 3. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html
function(add_test_suite_target target)
set(prefix ARG)
set(noValues)
>>
>>> Or, maybe this line
>>> should be ommitted since it is also used for normal test targets with
>>> several tests.
>> CMake test for a target with dependencies must be generated,
>>
>> otherwise dependencies will not be built.
>>
>>> For now `ctest -N` output looks a little bit inconsistent:
>>>
>>> | Test project /home/burii/reviews/luajit/ctest
>>> | Test #1: LuaJIT-tests-deps
>>> | Test #2: test/LuaJIT-tests
>>> | Test #3: PUC-Rio-Lua-5.1-tests-deps
>>> | Test #4: test/PUC-Rio-Lua-5.1
>>> | Test #5: lua-Harness-tests-deps
>>> | Test #6: test/lua-Harness-tests/000-sanity.t
>>> | ...
>>> | Test #56: tarantool-c-tests-deps
>>> | Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
>>> | ...
>>> | Test #67: tarantool-tests-deps
>>> | Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
>>> | ...
>>>
>>> If tests like the following are needed to run tests, I suggest naming
>>> them like `LuaJIT-tests-build` to avoid confusion.
>> AFAIR this naming was suggested by Igor.
>>
>> What kind of confusion you observe? For me something like
> That we are really testing something with this strange name.
> OK, my bad ignore it.
>
>> `LuaJIT-tests-build` much more confusing than "xxx-deps".
>>
>>> | Test #1: LuaJIT-tests-deps
> OK, let's leave it as it is.
>
> <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}
>>> Why do we need to use custom command for this test?
>>> IINM, all "subtargets" are used the same ${TEST_FLAGS}
>>> See also Igor's comment about this test.
>> Answered to Igor.
>>
>> because output will be ugly in that case,
>>
>> just change as you suggest and run LuaJIT-test.
> Please, mention the desired output format in the comment.
@@ -137,6 +148,13 @@ add_subdirectory(lua-Harness-tests)
add_subdirectory(tarantool-c-tests)
add_subdirectory(tarantool-tests)
+# Each testsuite has it's own CMake target, but combining these
+# target to a single one is not desired, because each target
+# runs it's own ctest command, that each time enumerates tests
+# from zero and prints test summary at the end of test run.
+# For common target this output looks inconvenient.
+# Therefore target below executes a single instance of ctest
+# command that runs all generated CMake tests.
add_custom_target(${PROJECT_NAME}-test
COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}
DEPENDS tarantool-c-tests-deps
>
> <snipped>
>
>>>>
>>>> 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
>>> This is not true. After these changes the environment variable
>>> `LD_PRELOAD` is the following:
>>> | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
>>> But it should be:
>>> | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
>>> This is why this quotes is used.
>>> For some reason, with the debug flags, paths are separated via \n:
>>> | 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
>>> | 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
>>>
>>> So, I got the following error, when I build LuaJIT with GCC and use the
>>> flag `-DLUAJIT_ENABLE_ASAN`:
>>> | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
>>> | #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69
>>>
>> Fixed.
>>
>> Unfortunately there is a yet another test that will never worked in
>> configuration GCC + ASAN.
>>
>> Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to
>> fix it, but as
>>
>> it was never worked before we don't need to do it in scope of ctest support.
> It works in our CI, where we use the clang build.
>
> For now, we may just disable this test for GCC + clang build with the
> link to the issue.
Added:
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -123,7 +123,8 @@ foreach(test_path ${tests})
)
endforeach()
-# Test is broken when GCC with enabled ASAN is used.
+# Test is broken when GCC with enabled ASAN is used,
+# see https://github.com/tarantool/tarantool/issues/9856.
if (LUAJIT_USE_ASAN AND (CMAKE_C_COMPILER_ID STREQUAL "GNU"))
set(test_title
test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua)
set_tests_properties(${test_title} PROPERTIES
>
> On branch, you're using global LD_PRELOAD from the aforementioned test
> (lj-802-panic-at-mcode-protfail.test.lua). As a result, tons of tests
> fail with the following error:
> | got: PANIC: unprotected error in call to Lua API (runtime code generation failed, restricted kernel?)
>
> May be it is better to proxy parent LD_PRELOAD to the test instead?
Ok
>
>> diff --git a/test/LuaJIT-tests/CMakeLists.txt
>> b/test/LuaJIT-tests/CMakeLists.txt
>> index 0c85f15c..4c9cf901 100644
>> --- a/test/LuaJIT-tests/CMakeLists.txt
>> +++ b/test/LuaJIT-tests/CMakeLists.txt
>> @@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)
>> # https://github.com/google/sanitizers/issues/934.
>> macro(LibRealPath output lib)
>> execute_process(
>> - COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
>> + COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
> Please mention in the comment that the C++ and C compilers use the same
> tooling, so the returned library path is the same.
Already commented in v4:
# CMAKE_C_COMPILER is used because it has the same behaviour
# as CMAKE_CXX_COMPILER, but CMAKE_CXX_COMPILER is not required
# for building LuaJIT and can be missed in GH Actions.
COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
OUTPUT_VARIABLE LIB_LINK
OUTPUT_STRIP_TRAILING_WHITESPACE
RESULT_VARIABLE RES
>
>> OUTPUT_VARIABLE LIB_LINK
>> OUTPUT_STRIP_TRAILING_WHITESPACE
>> + TIMEOUT 5
> Why do we need timeout here? (*)
Removed in v4.
>
>> + RESULT_VARIABLE RES
>> )
>> + if (${LIB_LINK} STREQUAL ${lib})
>> + message(FATAL_ERROR "library ${lib} is not found")
>> + endif()
>> + if (NOT RES EQUAL 0)
>> + message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed")
> Nit: the message is not very informative. I still don't know how to
> rewrite it, since compilers just print the input for unexisting files.
> Hence, I am not sure: do we really need this error message, since the
> given command is valid?
Discussed verbally and in v4 is the following message:
if (NOT RES EQUAL 0)
message(FATAL_ERROR "Executing '${CMAKE_C_COMPILER} \
-print-file-name=${lib}' has failed")
endif()
>> + endif()
>> +
>> # Fortunately, we are not interested in macOS here, so we can
>> # use realpath.
>> + # Beware, builtin command returns exit code equal to 0,
>> + # we cannot check is it failed or not.
>> execute_process(
>> COMMAND realpath ${LIB_LINK}
>> OUTPUT_VARIABLE ${output}
>> OUTPUT_STRIP_TRAILING_WHITESPACE
>> + TIMEOUT 5
> (*) And here?
Removed in v4.
>
>> )
>> endmacro()
>> LibRealPath(LIB_STDCPP libstdc++.so)
>> - # XXX: GCC requires both. Clang requires only libstdc++.
>> 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}")
>> + # XXX: The items of the list in LD_PRELOAD can be separated
>> + # by spaces or colons, and there is no support for escaping
>> + # either separator, see ld.so(8).
>> + 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()
> Minor: Maybe it is better to move this change to a separate patch.
> Feel free to ignore.
Ignored. Before introducing CTest this worked, changes above fixes
behaviour broken by patch with CTest.
>
> <snipped>
>
>>>> +add_test_suite_target(LuaJIT-tests
>>>> + LABEL ${TEST_SUITE_NAME}
>>>> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
>>> Why don't use `${LUAJIT_TEST_BINARY}` here?
>>
>> AFAIR we decided with Igor in a verbal conversation
>>
>> to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}.
>>
>> LUAJIT_TEST_BINARY is a single target that can be used as a command,
>>
>> see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a
>> list of targets
>>
>> that should be build before running tests.
> Yes, but we are now setting LUAJIT_TEST_DEPS unconditionally.
> Also, as I mentioned above for tarantool-c-tests, there is no need to
> build the luajit-main target -- libluajit is enough. Hence, this change
> may be dropped.
> CC imun@ here.
>>>> +)
>>>> +
>>>> +# The test suite has its own test runner
>>>> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
>>>> +# tests one-by-one by CTest.
>>>> +add_test(NAME "test/LuaJIT-tests"
>>> Why don't use just LuaJIT-tests as a test name here?
>> For consistency with other test titles.
> Side note: It looks inconsistent anyway (partially):
>
> | Start 1: LuaJIT-tests-deps
> | 1/211 Test #1: LuaJIT-tests-deps .............................................................. Passed 0.29 sec
> | Start 2: test/LuaJIT-tests
> | 2/211 Test #2: test/LuaJIT-tests .............................................................. Passed 3.56 sec
> | Start 3: PUC-Rio-Lua-5.1-tests-deps
> | 3/211 Test #3: PUC-Rio-Lua-5.1-tests-deps ..................................................... Passed 0.35 sec
> | Start 4: test/PUC-Rio-Lua-5.1
>
> As you can see, *-deps targets have no test in their name.
> But I can't come up with something meaningful, so feel free to ignore
> it.
Okay, I left as is.
>>> The same question to the tests below.
>>>
>>>> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
>>>> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
>>>> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
>>>> )
>>>> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
>>>> + LABELS ${TEST_SUITE_NAME}
>>>> + ENVIRONMENT "${LUAJIT_TESTS_ENV}"
>>>> + DEPENDS LuaJIT-tests-deps
>>>> +)
>>>> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
>>>> index 2f90da86..f0e639e1 100644
>>>> --- a/test/LuaJIT-tests/src/CMakeLists.txt
>>>> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
> <snipped>
>
>>>> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
>>> I suppose, that general part "test/" may be ommited from the title.
>> I would left as is. The main idea is to map test title to a file/dir on
>> the filesystem.
> It's kind of obvious to me that tests are placed in the directory
> "test". I'm not insisting, but the names of the tests are already long
> enough, it will be nicer to decrease their length with unnecessary
> information. The "test" directory is still the root directory for all
> tests.
It is not clear for me why you are care about test title length.
ctest accepts partial test titles too:
`ctest -R lj-802`
(test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua will be
executed)
> OTOH, it is more convenient to copy-paste the filenames, but for sure, we
> must use the whole filename to be consistent with the prove's output we
> are replacing.
>
> <snipped>
>
>>>> +# To workaround this, we glob source files and generated tests
>>>> +# with paths to executable binaries.
>>>> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
>>> Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".
>>>
>>> You may use the cycle above, which adds build targets.
>> This is a good idea, but anyway we need two loops.
>>
>> First should build a list TESTS_COMPILED that is used in
>> add_test_suite_target.
>>
>> The second one should generate CMake tests.
>>
>> If you are still want to have a single loop for everything then
>>
>> we need to build -deps target and CMake test for each tarantool C test.
> Shouldn't it be enough to set
> | DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${exe}
> for each test? But, yes, in this way, the order of compilation and
> execution isn't sequential...
> Maybe we should use the proxy target here. In the loop, we append a
> dependency for this target and set it as a dependency for each test
> target. May it work?
seems yes
diff --git a/test/tarantool-c-tests/CMakeLists.txt
b/test/tarantool-c-tests/CMakeLists.txt
index a4bc3a76..abf44a46 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -19,6 +19,21 @@ AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
# test binary can be run from any directory.
AppendFlags(TESTS_C_FLAGS
"-D__LJ_TEST_DIR__='\"${CMAKE_CURRENT_SOURCE_DIR}\"'")
+set(TEST_SUITE_NAME "tarantool-c-tests")
+
+# Proxy CMake target with all targets that builds C tests.
+# This is needed because targets for each C test are generated
+# at the same time with CMake tests, and all prerequisites must be
+# already exist at this moment.
+add_custom_target(tarantool-c-tests-build)
+
+# XXX: The call produces both test and target
+# <tarantool-c-tests-deps> as a side effect.
+add_test_suite_target(tarantool-c-tests
+ LABELS ${TEST_SUITE_NAME}
+ DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest tarantool-c-tests-build
+)
+
set(CTEST_SRC_SUFFIX ".test.c")
file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
foreach(test_source ${tests})
@@ -36,27 +51,12 @@ foreach(test_source ${tests})
)
target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
LIST(APPEND TESTS_COMPILED ${exe})
-endforeach()
-
-set(TEST_SUITE_NAME "tarantool-c-tests")
-
-# XXX: The call produces both test and target
-# <tarantool-c-tests-deps> as a side effect.
-add_test_suite_target(tarantool-c-tests
- LABELS ${TEST_SUITE_NAME}
- DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED}
-)
+ add_dependencies(tarantool-c-tests-build ${exe})
-# 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_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${CTEST_SRC_SUFFIX}")
-foreach(test_path ${tests})
- get_filename_component(test_name ${test_path} NAME_WE)
- set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
+ # Generate CMake tests.
+ set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")
add_test(NAME ${test_title}
- COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}
+ COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
)
set_tests_properties(${test_title} PROPERTIES
>
>> Left without changes.
> May you please add a comment about it to avoid confusion in the future?
patch is above
>
> <snipped>
>
>>>> +set(TEST_SUITE_NAME "tarantool-tests")
>>>> +
>>>> +# XXX: The call produces both test and target
>>>> +# <tarantool-tests-deps> as a side effect.
>>>> +add_test_suite_target(tarantool-tests
>>>> + LABEL ${TEST_SUITE_NAME}
>>>> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
>>>> )
>>>>
>>>> +# 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}")
>>>> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
>>> Why do we need GLOB_RECURSE here, instead of GLOB?
>>
>> it is better to search test files recursively, no one can guarantee that
>> files always will be in a top testsuite dir.
> Ok, thanks for the clarification.
>
> <snipped>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-22 13:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-12 7:06 [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches
2024-03-16 17:12 ` Igor Munkin via Tarantool-patches
2024-03-18 14:58 ` Sergey Bronnikov via Tarantool-patches
2024-03-18 15:54 ` Sergey Kaplun via Tarantool-patches
2024-03-20 19:44 ` Sergey Bronnikov via Tarantool-patches
2024-03-21 11:17 ` Sergey Kaplun via Tarantool-patches
2024-03-22 13:06 ` Sergey Bronnikov via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox