* [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest
@ 2024-03-26 8:33 Sergey Bronnikov via Tarantool-patches
2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN Sergey Bronnikov via Tarantool-patches
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw)
To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin
From: Sergey Bronnikov <sergeyb@tarantool.org>
Changes v5:
- applied fixes suggested by Sergey Kaplun
- added patches with prerequisites
PR: https://github.com/tarantool/tarantool/pull/9286
Branch: https://github.com/tarantool/luajit/tree/ligurio/enable_test_target
Sergey Bronnikov (4):
ci: execute LuaJIT tests with GCC 10 and ASAN
test: refactor CMake macro LibRealPath
test: more cautious usage of LD_PRELOAD for ASan
cmake: replace prove with CTest
Sergey Kaplun (2):
test: move LibRealPath to the separate module
test: fix lj-802-panic-at-mcode-protfail GCC+ASan
.github/actions/setup-sanitizers/action.yml | 15 ++-
.github/workflows/sanitizers-testing.yml | 4 +
.gitignore | 2 +
CMakeLists.txt | 14 +++
test/CMakeLists.txt | 108 +++++++++++++++-----
test/LuaJIT-tests/CMakeLists.txt | 61 +++++------
test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++--
test/cmake/LibRealPath.cmake | 41 ++++++++
test/lua-Harness-tests/CMakeLists.txt | 50 +++++----
test/tarantool-c-tests/CMakeLists.txt | 58 +++++------
test/tarantool-tests/CMakeLists.txt | 77 ++++++++------
test/tarantool-tests/utils/exec.lua | 14 +++
12 files changed, 320 insertions(+), 149 deletions(-)
create mode 100644 test/cmake/LibRealPath.cmake
--
2.34.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 9:51 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 2/6] test: refactor CMake macro LibRealPath Sergey Bronnikov via Tarantool-patches ` (5 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin From: Sergey Bronnikov <sergeyb@tarantool.org> The commit fa8e9e9a721e ("test: enable <catch_wrap.lua> LuaJIT test") brings a workaround to CMake for running tests with LuaJIT built by GCC with enabled AddressSanitizer. However, we have no such configuration in CI, which means that sooner or later it will be broken. The patch adds a job to GH Actions that builds LuaJIT with GCC 10 with enabled AddressSanitizer and runs LuaJIT tests. Linux builds are built by runners on Ubuntu Focal, where GCC 10 is the latest version [1] available in packages. Library libstdc++ is needed by the test <catch_wrap.lua> for a workaround with AddressSanitizer. 1. https://packages.ubuntu.com/focal/gcc-10 Follows up tarantool/tarantool#9398 Relates to tarantool/tarantool#9656 --- .github/actions/setup-sanitizers/action.yml | 15 ++++++++++++--- .github/workflows/sanitizers-testing.yml | 4 ++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/.github/actions/setup-sanitizers/action.yml b/.github/actions/setup-sanitizers/action.yml index 4b74f39d..8642d553 100644 --- a/.github/actions/setup-sanitizers/action.yml +++ b/.github/actions/setup-sanitizers/action.yml @@ -1,5 +1,10 @@ name: Setup CI environment for testing with sanitizers on Linux description: Common part to tweak Linux CI runner environment for sanitizers +inputs: + cc_name: + description: C compiler name (for example, gcc-12) + required: false + default: clang-11 runs: using: composite steps: @@ -15,9 +20,13 @@ runs: - name: Install build and test dependencies run: | apt -y update - apt -y install clang-11 cmake ninja-build make perl + apt -y install ${CC_NAME} libstdc++-10-dev cmake ninja-build make perl shell: bash - - name: Set Clang as a default toolchain + env: + CC_NAME: ${{ inputs.cc_name }} + - name: Set specific C compiler as a default toolchain run: | - echo CC=clang-11 | tee -a $GITHUB_ENV + echo CC=${CC_NAME} | tee -a $GITHUB_ENV shell: bash + env: + CC_NAME: ${{ inputs.cc_name }} diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml index 4bccfcef..a06398ea 100644 --- a/.github/workflows/sanitizers-testing.yml +++ b/.github/workflows/sanitizers-testing.yml @@ -33,6 +33,7 @@ jobs: matrix: # XXX: Let's start with only Linux/x86_64 BUILDTYPE: [Debug, Release] + CC: [gcc-10, clang-11] include: - BUILDTYPE: Debug CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON @@ -42,6 +43,7 @@ jobs: name: > LuaJIT with ASan (Linux/x86_64) ${{ matrix.BUILDTYPE }} + CC:${{ matrix.CC }} GC64:ON SYSMALLOC:ON steps: - uses: actions/checkout@v3 @@ -50,6 +52,8 @@ jobs: submodules: recursive - name: setup Linux for sanitizers uses: ./.github/actions/setup-sanitizers + with: + cc_name: ${{ matrix.CC }} - name: configure # XXX: LuaJIT configuration requires a couple of tweaks: # LUAJIT_USE_SYSMALLOC=ON: Unfortunately, internal LuaJIT -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN Sergey Bronnikov via Tarantool-patches @ 2024-03-26 9:51 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-03-26 9:51 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Hi, Sergey! Thanks for the patch! LGTM, but please replace it after all related fixes (PATCH 5/6), when the aforementioned build works correctly. -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Tarantool-patches] [PATCH luajit 2/6] test: refactor CMake macro LibRealPath 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 9:57 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 3/6] test: move LibRealPath to the separate module Sergey Bronnikov via Tarantool-patches ` (4 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin From: Sergey Bronnikov <sergeyb@tarantool.org> The patch refactors the CMake macro LibRealPath: - Replace CMAKE_CXX_COMPILER with CMAKE_C_COMPILER because LuaJIT doesn't require the CXX compiler, and thus it can be unavailable on a build system. - Add a check for a code returned by a command with the compiler. - Add a check for a value returned by the compiler. A value equal to a passed one means that the library was not found in the system. - Unset local variables to avoid namespace pollution. Needed for tarantool/tarantool#9656 --- test/LuaJIT-tests/CMakeLists.txt | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index a0fb5440..badec91d 100644 --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -33,18 +33,41 @@ 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} + # 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 ) + + if(NOT RES EQUAL 0) + message(FATAL_ERROR + "Executing '${CMAKE_C_COMPILER} -print-file-name=${lib}' has failed" + ) + endif() + + # GCC and Clang return a passed filename when a library is + # not found. + if(LIB_LINK STREQUAL ${lib}) + message(FATAL_ERROR "Library '${lib}' is not found") + endif() + # Fortunately, we are not interested in macOS here, so we can - # use realpath. + # use realpath. Beware, `realpath` always returns an exit code + # equal to 0, so we cannot check if it fails. execute_process( COMMAND realpath ${LIB_LINK} OUTPUT_VARIABLE ${output} OUTPUT_STRIP_TRAILING_WHITESPACE ) + + unset(LIB_LINK) + unset(RES) endmacro() + LibRealPath(LIB_STDCPP libstdc++.so) # XXX: GCC requires both. Clang requires only libstdc++. if(CMAKE_C_COMPILER_ID STREQUAL "GNU") -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/6] test: refactor CMake macro LibRealPath 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 2/6] test: refactor CMake macro LibRealPath Sergey Bronnikov via Tarantool-patches @ 2024-03-26 9:57 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-03-26 9:57 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Hi, Sergey! Thanks for the patch! LGTM! -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Tarantool-patches] [PATCH luajit 3/6] test: move LibRealPath to the separate module 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN Sergey Bronnikov via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 2/6] test: refactor CMake macro LibRealPath Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 10:01 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 4/6] test: more cautious usage of LD_PRELOAD for ASan Sergey Bronnikov via Tarantool-patches ` (3 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin From: Sergey Kaplun <skaplun@tarantool.org> This patch moves the `LibRealPath` macro to a separate module to be used in other test suites. Needed for tarantool/tarantool#9656 --- test/CMakeLists.txt | 1 + test/LuaJIT-tests/CMakeLists.txt | 37 ---------------------------- test/cmake/LibRealPath.cmake | 41 ++++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 37 deletions(-) create mode 100644 test/cmake/LibRealPath.cmake diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 3ad5d15f..62478441 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -76,6 +76,7 @@ separate_arguments(LUAJIT_TEST_COMMAND) set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") include(AddTestLib) +include(LibRealPath) add_subdirectory(LuaJIT-tests) add_subdirectory(PUC-Rio-Lua-5.1-tests) diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index badec91d..e8361e97 100644 --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -31,43 +31,6 @@ if(LUAJIT_USE_ASAN) # "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) # This is a workaround suggested at # https://github.com/google/sanitizers/issues/934. - macro(LibRealPath output lib) - execute_process( - # 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 - ) - - if(NOT RES EQUAL 0) - message(FATAL_ERROR - "Executing '${CMAKE_C_COMPILER} -print-file-name=${lib}' has failed" - ) - endif() - - # GCC and Clang return a passed filename when a library is - # not found. - if(LIB_LINK STREQUAL ${lib}) - message(FATAL_ERROR "Library '${lib}' is not found") - endif() - - # Fortunately, we are not interested in macOS here, so we can - # use realpath. Beware, `realpath` always returns an exit code - # equal to 0, so we cannot check if it fails. - execute_process( - COMMAND realpath ${LIB_LINK} - OUTPUT_VARIABLE ${output} - OUTPUT_STRIP_TRAILING_WHITESPACE - ) - - unset(LIB_LINK) - unset(RES) - endmacro() - LibRealPath(LIB_STDCPP libstdc++.so) # XXX: GCC requires both. Clang requires only libstdc++. if(CMAKE_C_COMPILER_ID STREQUAL "GNU") diff --git a/test/cmake/LibRealPath.cmake b/test/cmake/LibRealPath.cmake new file mode 100644 index 00000000..70d43c50 --- /dev/null +++ b/test/cmake/LibRealPath.cmake @@ -0,0 +1,41 @@ +# Find a full path of a library used by a compiler and set it to +# the given variable. +macro(LibRealPath output lib) + if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") + message(FATAL_ERROR "LibRealPath macro is unsupported for OSX") + endif() + execute_process( + # 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 + ) + + if(NOT RES EQUAL 0) + message(FATAL_ERROR + "Executing '${CMAKE_C_COMPILER} -print-file-name=${lib}' has failed" + ) + endif() + + # GCC and Clang return a passed filename when a library is + # not found. + if(LIB_LINK STREQUAL ${lib}) + message(FATAL_ERROR "Library '${lib}' is not found") + endif() + + # Fortunately, we are not interested in macOS here, so we can + # use realpath. Beware, `realpath` always returns an exit code + # equal to 0, so we cannot check if it fails. + execute_process( + COMMAND realpath ${LIB_LINK} + OUTPUT_VARIABLE ${output} + OUTPUT_STRIP_TRAILING_WHITESPACE + ) + + unset(LIB_LINK) + unset(RES) +endmacro() -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 3/6] test: move LibRealPath to the separate module 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 3/6] test: move LibRealPath to the separate module Sergey Bronnikov via Tarantool-patches @ 2024-03-26 10:01 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-03-26 10:01 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Hi, Sergey! Thanks for the patch! LGTM! -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Tarantool-patches] [PATCH luajit 4/6] test: more cautious usage of LD_PRELOAD for ASan 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches ` (2 preceding siblings ...) 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 3/6] test: move LibRealPath to the separate module Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 10:04 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan Sergey Bronnikov via Tarantool-patches ` (2 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin From: Sergey Bronnikov <sergeyb@tarantool.org> This patch adds a comment for clarification about the usage of different versions of ASan implementations for different compilers. Also, it replaces ' ' with a ':' as a separator in `LD_PRELOAD` to make it more robust to the CMake list semantics and removes unnecessary double quotes. Needed for tarantool/tarantool#9656 Co-authored-by: Sergey Bronnikov <sergeyb@tarantool.org> --- test/LuaJIT-tests/CMakeLists.txt | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index e8361e97..003f8de6 100644 --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -33,13 +33,19 @@ if(LUAJIT_USE_ASAN) # https://github.com/google/sanitizers/issues/934. LibRealPath(LIB_STDCPP libstdc++.so) # XXX: GCC requires both. Clang requires only libstdc++. + # Be aware, ASAN runtime in Clang on Linux was offered only in + # the form of a library with differ name from libasan.so (e.g. + # libclang_rt.asan-x86_64.a). + # See also: + # https://github.com/google/sanitizers/wiki/AddressSanitizerAsDso. 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() -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 4/6] test: more cautious usage of LD_PRELOAD for ASan 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 4/6] test: more cautious usage of LD_PRELOAD for ASan Sergey Bronnikov via Tarantool-patches @ 2024-03-26 10:04 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-03-26 10:04 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Hi, Sergey! Thanks for the patch! LGTM! -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches ` (3 preceding siblings ...) 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 4/6] test: more cautious usage of LD_PRELOAD for ASan Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 10:08 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches 2024-04-11 16:47 ` [Tarantool-patches] [PATCH luajit 0/6][v5] " Sergey Kaplun via Tarantool-patches 6 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin From: Sergey Kaplun <skaplun@tarantool.org> The aforementioned test uses `LD_PRELOAD` to mock the system call to `mprotect()`. The GCC implementation of libasan requires that the ASan library go first in the `LD_PRELOAD` list. This patch tweaks the behaviour to avoid failure of the test. OTOH, this patch intorduces warnings related to the libc leaks for a couple of tests, but they are not treated as test failures. This should be fixed by target setting LD_PRELOAD only for necessary tests when we start using CTest instead of `prove`. Part of tarantool/tarantool#9656 Co-authored-by: Sergey Bronnikov <sergeyb@tarantool.org> --- test/tarantool-tests/CMakeLists.txt | 15 +++++++++++++++ test/tarantool-tests/utils/exec.lua | 14 ++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt index e6d12984..35bcc5ef 100644 --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -110,6 +110,21 @@ else() list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH}) endif() +# Some tests use `LD_PRELOAD` to mock system calls (like +# <lj-802-panic-at-mcode-protfail.test.lua> overwrites +# `mprotect()`. When compiling with ASan support under GCC, it is +# required that the ASan library go first in the `LD_PRELOAD` +# list. Set it manually, test will append it to the executed +# process. +# See also: https://github.com/tarantool/tarantool/issues/9656. +if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") + # FIXME: After using CTest instead of `prove` we should set this + # environment variable only for the corresponding tests to avoid + # warnings from the libc and etc. + LibRealPath(LIB_ASAN libasan.so) + list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN}) +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 diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua index 48a08ba5..49f8ef06 100644 --- a/test/tarantool-tests/utils/exec.lua +++ b/test/tarantool-tests/utils/exec.lua @@ -24,6 +24,20 @@ local function makeenv(tabenv) if tabenv == nil then return '' end local flatenv = {} for var, value in pairs(tabenv) do + if var == 'LD_PRELOAD' then + -- XXX: For ASan used by GCC, the ASan library should go + -- first in the `LD_PRELOAD` list, or we get the error: + -- "ASan runtime does not come first in initial library + -- list;". + -- See https://github.com/tarantool/tarantool/issues/9656 + -- for details. + -- So, prepend the given library from `LD_PRELOAD` to the + -- start of the list. The separator may be ':' or ' ', see + -- man ld.so(8).so. Use ':' as the most robust. + local ld_preload = os.getenv('LD_PRELOAD') + local ld_preload_prefix = ld_preload and ld_preload .. ':' or '' + value = ld_preload_prefix .. value + end table.insert(flatenv, ('%s=%s'):format(var, value)) end return table.concat(flatenv, ' ') -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan Sergey Bronnikov via Tarantool-patches @ 2024-03-26 10:08 ` Sergey Kaplun via Tarantool-patches 2024-03-26 15:22 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-03-26 10:08 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Hi, Sergey! Thanks for the patch! LGTM, except 2 typos regarding the commit message and the comment. On 26.03.24, Sergey Bronnikov wrote: > From: Sergey Kaplun <skaplun@tarantool.org> > > The aforementioned test uses `LD_PRELOAD` to mock the system call to > `mprotect()`. The GCC implementation of libasan requires that the ASan > library go first in the `LD_PRELOAD` list. This patch tweaks the > behaviour to avoid failure of the test. OTOH, this patch intorduces Typo: s/intorduces/introduces/ > warnings related to the libc leaks for a couple of tests, but they are > not treated as test failures. This should be fixed by target setting > LD_PRELOAD only for necessary tests when we start using CTest instead of > `prove`. > > Part of tarantool/tarantool#9656 > > Co-authored-by: Sergey Bronnikov <sergeyb@tarantool.org> > --- > test/tarantool-tests/CMakeLists.txt | 15 +++++++++++++++ > test/tarantool-tests/utils/exec.lua | 14 ++++++++++++++ > 2 files changed, 29 insertions(+) > > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > index e6d12984..35bcc5ef 100644 > --- a/test/tarantool-tests/CMakeLists.txt > +++ b/test/tarantool-tests/CMakeLists.txt > @@ -110,6 +110,21 @@ else() > list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH}) > endif() > > +# Some tests use `LD_PRELOAD` to mock system calls (like > +# <lj-802-panic-at-mcode-protfail.test.lua> overwrites > +# `mprotect()`. When compiling with ASan support under GCC, it is > +# required that the ASan library go first in the `LD_PRELOAD` > +# list. Set it manually, test will append it to the executed Typo: s/manually, test/manually. The test/ > +# process. > +# See also: https://github.com/tarantool/tarantool/issues/9656. <snipped> > -- > 2.34.1 > -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan 2024-03-26 10:08 ` Sergey Kaplun via Tarantool-patches @ 2024-03-26 15:22 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 0 replies; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 15:22 UTC (permalink / raw) To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches Sergey, On 3/26/24 13:08, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the patch! > LGTM, except 2 typos regarding the commit message and the comment. > > On 26.03.24, Sergey Bronnikov wrote: >> From: Sergey Kaplun <skaplun@tarantool.org> >> >> The aforementioned test uses `LD_PRELOAD` to mock the system call to >> `mprotect()`. The GCC implementation of libasan requires that the ASan >> library go first in the `LD_PRELOAD` list. This patch tweaks the >> behaviour to avoid failure of the test. OTOH, this patch intorduces > Typo: s/intorduces/introduces/ > Fixed. >> warnings related to the libc leaks for a couple of tests, but they are >> not treated as test failures. This should be fixed by target setting >> LD_PRELOAD only for necessary tests when we start using CTest instead of >> `prove`. >> >> Part of tarantool/tarantool#9656 >> >> Co-authored-by: Sergey Bronnikov <sergeyb@tarantool.org> >> --- >> test/tarantool-tests/CMakeLists.txt | 15 +++++++++++++++ >> test/tarantool-tests/utils/exec.lua | 14 ++++++++++++++ >> 2 files changed, 29 insertions(+) >> >> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt >> index e6d12984..35bcc5ef 100644 >> --- a/test/tarantool-tests/CMakeLists.txt >> +++ b/test/tarantool-tests/CMakeLists.txt >> @@ -110,6 +110,21 @@ else() >> list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH}) >> endif() >> >> +# Some tests use `LD_PRELOAD` to mock system calls (like >> +# <lj-802-panic-at-mcode-protfail.test.lua> overwrites >> +# `mprotect()`. When compiling with ASan support under GCC, it is >> +# required that the ASan library go first in the `LD_PRELOAD` >> +# list. Set it manually, test will append it to the executed > Typo: s/manually, test/manually. The test/ > Fixed. >> +# process. >> +# See also: https://github.com/tarantool/tarantool/issues/9656. > <snipped> > >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches ` (4 preceding siblings ...) 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 11:03 ` Sergey Kaplun via Tarantool-patches 2024-04-11 16:47 ` [Tarantool-patches] [PATCH luajit 0/6][v5] " Sergey Kaplun via Tarantool-patches 6 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-03-26 8:33 UTC (permalink / raw) To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin 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 operating. CMake custom target `test` was replaced by a target with the same name that is automatically generated by CMake. It is not possible to attach targets to this automatically generated `test` target. It means that target `test` is now executing `LuaJIT-test`, but not `LuaJIT-lint`. To mitigate this a new target `LuaJIT-check-all` is introduced, it has the same behaviour like an old `test` target: it runs all functional tests and linters. One could use `ctest` tool: $ 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-tests # Run tests by label. $ ctest -L tarantool-c-tests # Ditto. $ ctest -L lua-Harness-tests # Ditto. $ ctest -L LuaJIT-tests # Ditto. $ ctest -L PUC-Rio-Lua-5.1-tests # Ditto. $ ctest --rerun-fail # Run only the tests # that failed previously. $ ctest --verbose # Print details to stdout. $ ctest --output-on-failure # Print details to stdout # on test failure only. 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 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 is workarounded by an introduced macro `add_test_suite_target` that adds a CMake-test for each testsuite that executes a target that builds test dependencies. The commit introduce a CMake option LUAJIT_TEST_DEPS that set dependencies to be used for running and building LuaJIT tests. 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 --- .gitignore | 2 + CMakeLists.txt | 14 +++ test/CMakeLists.txt | 107 +++++++++++++++++----- test/LuaJIT-tests/CMakeLists.txt | 33 ++++--- test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++-- test/lua-Harness-tests/CMakeLists.txt | 50 +++++----- test/tarantool-c-tests/CMakeLists.txt | 58 ++++++------ test/tarantool-tests/CMakeLists.txt | 68 +++++++------- 8 files changed, 226 insertions(+), 131 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..96f85285 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,13 @@ 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. This automatically generates +# the `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). +if (LUAJIT_USE_TEST) + enable_testing() +endif() add_subdirectory(test) # --- Misc rules --------------------------------------------------------------- diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 62478441..06a1d365 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -78,35 +78,94 @@ set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") include(AddTestLib) include(LibRealPath) +# 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(CTEST_FLAGS + --output-on-failure + --schedule-random + # Timeout in seconds. Most of LuaJIT tests are fast, + # however, some tests can hang or execute infinite loop. + # See <tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c>, + # commit caa99865c206 ("test: rewrite sysprof test using managed execution"). + # Moreover, tests for instrumented LuaJIT requires more time. + # 30 minutes is a sane timeout. + --timeout 1800 + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL} +) +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) + set(singleValues LABELS) + 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(NAME ${_DEPS_TARGET} + COMMAND ${CMAKE_COMMAND} + --build ${CMAKE_BINARY_DIR} + --target ${_DEPS_TARGET} + ) + set_tests_properties(${_DEPS_TARGET} PROPERTIES + LABELS ${ARG_LABELS} + ) + + message(STATUS "Add test suite ${ARG_LABELS}") + + add_custom_target(${target} + COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABELS} ${CTEST_FLAGS} + DEPENDS ${_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 +# 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 + 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() +add_custom_target(${PROJECT_NAME}-check-all + DEPENDS ${PROJECT_NAME}-test + ${PROJECT_NAME}-lint +) diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index 003f8de6..6ad05aa6 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/:") @@ -56,12 +52,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 + LABELS ${TEST_SUITE_NAME} + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-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/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt index 98277f9a..a26e38d4 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 + LABELS ${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-tests" + 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-tests" 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..6f7b5697 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,37 @@ 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 + LABELS ${TEST_SUITE_NAME} + DEPENDS ${LUAJIT_TEST_DEPS} ) +set(LUA_TEST_SUFFIX .t) + +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") +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() + 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..5affbb9e 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. @@ -30,6 +19,23 @@ 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} + # XXX: LUAJIT_TEST_DEPS is not needed, we know dependencies + # in advance. + DEPENDS 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}) @@ -47,22 +53,16 @@ foreach(test_source ${tests}) ) target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY}) LIST(APPEND TESTS_COMPILED ${exe}) -endforeach() + add_dependencies(tarantool-c-tests-build ${exe}) -add_custom_target(tarantool-c-tests - DEPENDS libluajit libtest ${TESTS_COMPILED} -) - -add_custom_command(TARGET tarantool-c-tests - COMMENT "Running Tarantool C tests" - COMMAND - ${PROVE} - ${CMAKE_CURRENT_BINARY_DIR} - --ext ${C_TEST_SUFFIX} - --jobs ${CMAKE_BUILD_PARALLEL_LEVEL} - # Report any TAP parse errors, if any, since test module is - # maintained by us. - --parse - ${C_TEST_FLAGS} - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} -) + # Generate CMake tests. + set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}") + add_test(NAME ${test_title} + COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${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 35bcc5ef..9086a890 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-libs DEPENDS ${LUAJIT_TEST_BINARY} ) macro(BuildTestCLib lib sources) AddTestLib(${lib} ${sources}) - add_dependencies(tarantool-tests ${lib}) + add_dependencies(tarantool-tests-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 @@ -61,20 +55,12 @@ make_lua_path(LUA_PATH ${LUAJIT_SOURCE_DIR}/?.lua ${LUAJIT_BINARY_DIR}/?.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 @@ -118,29 +104,37 @@ endif() # process. # See also: https://github.com/tarantool/tarantool/issues/9656. if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") - # FIXME: After using CTest instead of `prove` we should set this - # environment variable only for the corresponding tests to avoid - # warnings from the libc and etc. + # FIXME: test_title test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua + # With CTest we should set this environment variable only + # for the corresponding tests to avoid warnings from + # the GNU libc and other libc implementations. LibRealPath(LIB_ASAN libasan.so) list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN}) 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 + LABELS ${TEST_SUITE_NAME} + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-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] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches @ 2024-03-26 11:03 ` Sergey Kaplun via Tarantool-patches 2024-04-01 14:28 ` Sergey Bronnikov via Tarantool-patches 0 siblings, 1 reply; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-03-26 11:03 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Hi, Sergey! Thanks for the fixes! Please consider some minor comments and nits below. On 26.03.24, Sergey Bronnikov wrote: > From: Sergey Bronnikov <sergeyb@tarantool.org> > > The patch replaces the main test runner `prove(1)` with CTest, Since we are dropping the usage of `prove`, we should reformulate related comments about it, shouldn't we? Also, maybe the LUA_TEST_ENV_MORE can be removed? > see [1] and [2]. > > Build and test steps looks the same as before: Typo: s/looks/look/ > > $ 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 operating. > > CMake custom target `test` was replaced by a target with the same Typo: s/CMake/The CMake/ > name that is automatically generated by CMake. It is not possible > to attach targets to this automatically generated `test` target. > It means that target `test` is now executing `LuaJIT-test`, > but not `LuaJIT-lint`. To mitigate this a new target Typo: s/To mitigate this/To mitigate this,/ > `LuaJIT-check-all` is introduced, it has the same behaviour like Typo: s/introduced, it/introduced. It/ Typo: s/like/as/ > an old `test` target: it runs all functional tests and linters. > > One could use `ctest` tool: Typo: s/use `ctest` tool/use the `ctest` tool/ > > $ 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-tests # Run tests by label. > $ ctest -L tarantool-c-tests # Ditto. > $ ctest -L lua-Harness-tests # Ditto. > $ ctest -L LuaJIT-tests # Ditto. > $ ctest -L PUC-Rio-Lua-5.1-tests # Ditto. > $ ctest --rerun-fail # Run only the tests > # that failed previously. > $ ctest --verbose # Print details to stdout. > $ ctest --output-on-failure # Print details to stdout > # on test failure only. > > 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 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 is workarounded by an introduced Typo: s/used CMake version/the currently supported CMake version/ Typo: s/an introduced/the introduced/ > macro `add_test_suite_target` that adds a CMake-test for each > testsuite that executes a target that builds test dependencies. > > The commit introduce a CMake option LUAJIT_TEST_DEPS that set Typo: s/introduce/introduces/ Typo: s/a CMake option/the CMake option/ Typo: s/set/sets/ > dependencies to be used for running and building LuaJIT tests. > > 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 > --- > .gitignore | 2 + > CMakeLists.txt | 14 +++ > test/CMakeLists.txt | 107 +++++++++++++++++----- > test/LuaJIT-tests/CMakeLists.txt | 33 ++++--- > test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++-- > test/lua-Harness-tests/CMakeLists.txt | 50 +++++----- > test/tarantool-c-tests/CMakeLists.txt | 58 ++++++------ > test/tarantool-tests/CMakeLists.txt | 68 +++++++------- > 8 files changed, 226 insertions(+), 131 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..96f85285 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 Typo: s/to be used/to use/ or s/to be used/are used/ > +# LuaJIT tests. <snipped> > @@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR <snipped> > +if (LUAJIT_USE_TEST) Typo: s/if (/if(/ <snipped> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 62478441..06a1d365 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt <snipped> > +set(CTEST_FLAGS > + --output-on-failure > + --schedule-random > + # Timeout in seconds. Most of LuaJIT tests are fast, Typo: s/Most of LuaJIT tests are fast, however,/Most LuaJIT tests are fast. However,/ > + # however, some tests can hang or execute infinite loop. Typo: s/infinite loop/an infinite loop/ > + # See <tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c>, > + # commit caa99865c206 ("test: rewrite sysprof test using managed execution"). > + # Moreover, tests for instrumented LuaJIT requires more time. Typo: s/requires/require/ > + # 30 minutes is a sane timeout. > + --timeout 1800 > + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL} > +) > +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. Typo: s/it is unsupported/this feature is unsupported/ Typo: s/a current/the current/ > +# To workaround this a function `add_test_suite_target` is Typo: s/To workaround this a/To workaround this, the/ Typo: s/is introduced, it/is introduced. It/ > +# introduced, it adds a CMake target that builds testsuite's > +# prerequisites and CMake test that executes that target. Typo: s/CMake test/a CMake test/ > +# > +# 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 <snipped> > -add_custom_target(${PROJECT_NAME}-test DEPENDS > - LuaJIT-tests > - PUC-Rio-Lua-5.1-tests > - lua-Harness-tests > - tarantool-c-tests > - tarantool-tests > +# Each testsuite has it's own CMake target, but combining these Typo: s/it's/its/ > +# target to a single one is not desired, because each target Typo: s/these target to/these targets into/ Typo: s/desired,/desired/ > +# runs it's own ctest command, that each time enumerates tests Typo: s/command, that/command, which/ Typo? s/ctest/CTest/ > +# from zero and prints test summary at the end of test run. Typo: s/prints test summary/prints the test summary/ Typo: s/test run/ the test run/ > +# For common target this output looks inconvenient. Typo: s/For common target/For a common target,/ > +# Therefore target below executes a single instance of ctest Typo: s/Therefore target/Therefore, the target/ Typo: s/instance of ctest/instance of the CTest/ > +# command that runs all generated CMake tests. <snipped> > diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt > index 003f8de6..6ad05aa6 100644 > --- a/test/LuaJIT-tests/CMakeLists.txt > +++ b/test/LuaJIT-tests/CMakeLists.txt <snipped> > +# 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" Maybe it is better to use | add_test(NAME "test/${TEST_SUITE_NAME}" for consistency for all suites? Also, it avoids confusing (for me, this is some kind of typo opposed to the `add_test_suite_target()` command above). Feel free to ignore, since it is subjectivity. > + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua > + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA} > WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} > ) <snipped> > diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt > index 98277f9a..a26e38d4 100644 > --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt > +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt <snipped> > -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-tests" > + 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-tests" PROPERTIES > + ENVIRONMENT "LUA_PATH=${LUA_PATH}" Nit: Do we need to move the " back to the start of the declaration? If it is possible, please use LUA_PATH="${LUA_PATH}" instead. > + 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..6f7b5697 100644 > --- a/test/lua-Harness-tests/CMakeLists.txt > +++ b/test/lua-Harness-tests/CMakeLists.txt <snipped> > +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}) Typo: s/if (/if(/ > + continue() > + endif() <snipped> > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt > index 4b1d8832..5affbb9e 100644 > --- a/test/tarantool-c-tests/CMakeLists.txt > +++ b/test/tarantool-c-tests/CMakeLists.txt <snipped> > @@ -30,6 +19,23 @@ 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. Typo: s/Proxy/The proxy/ Typo: s/that builds/that build/ > +# This is needed because targets for each C test are generated > +# at the same time with CMake tests, and all prerequisites must be Typo: s/at the same time with/at the same time as/ Typo: s/must be already exist/must already exist/ > +# already exist at this moment. > +add_custom_target(tarantool-c-tests-build) <snipped> > @@ -47,22 +53,16 @@ foreach(test_source ${tests}) > ) > target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY}) > LIST(APPEND TESTS_COMPILED ${exe}) Should this line be removed since TESTS_COMPILED is no longer in use? > -endforeach() > + add_dependencies(tarantool-c-tests-build ${exe}) > > -add_custom_target(tarantool-c-tests > - DEPENDS libluajit libtest ${TESTS_COMPILED} > -) > - <snipped> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > index 35bcc5ef..9086a890 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-libs > DEPENDS ${LUAJIT_TEST_BINARY} Should it depend on libluajit instead? We don't need the LuaJIT binary to link with the LuaJIT library. > ) > > macro(BuildTestCLib lib sources) > AddTestLib(${lib} ${sources}) > - add_dependencies(tarantool-tests ${lib}) > + add_dependencies(tarantool-tests-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 > @@ -61,20 +55,12 @@ make_lua_path(LUA_PATH > ${LUAJIT_SOURCE_DIR}/?.lua > ${LUAJIT_BINARY_DIR}/?.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 > @@ -118,29 +104,37 @@ endif() > # process. > # See also: https://github.com/tarantool/tarantool/issues/9656. > if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") > - # FIXME: After using CTest instead of `prove` we should set this > - # environment variable only for the corresponding tests to avoid > - # warnings from the libc and etc. > + # FIXME: test_title test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua Typo: No dot at the end of the sentence. Nit: Comment line width is more than 66 symbols. Also, I didn't get the idea of the first sentence. Also, will we add an additional patch that fixes this comment in this patch series (as an additional commit) or will we do it later? > + # With CTest we should set this environment variable only Typo: s/With CTest/With CTest,/ > + # for the corresponding tests to avoid warnings from > + # the GNU libc and other libc implementations. Should this change be the part of the commit "test: fix lj-802-panic-at-mcode-protfail GCC+ASan"? > LibRealPath(LIB_ASAN libasan.so) > list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN}) > 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 > + LABELS ${TEST_SUITE_NAME} > + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs > ) > > +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list > +# with dependencies are set in scope of BuildTestLib macro. I prefer to use the old paraphrasing of this paragraph. Otherwise, the reader may think: "What the heck is TESTLIBS?" > +set(TEST_ENV "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}") Maybe it is better to use `list(APPEND` here for better readability? Or even don't use the additional variable that is used once and provide these ENVIROMENT variables directly below. > +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}") Should it be "test/${TEST_SUITE_NAME}/${test_name}" by analogy with other test suites? > + 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] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest 2024-03-26 11:03 ` Sergey Kaplun via Tarantool-patches @ 2024-04-01 14:28 ` Sergey Bronnikov via Tarantool-patches 2024-04-01 18:46 ` Sergey Kaplun via Tarantool-patches 0 siblings, 1 reply; 17+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2024-04-01 14:28 UTC (permalink / raw) To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 25093 bytes --] Sergey, On 3/26/24 14:03, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > Please consider some minor comments and nits below. > > On 26.03.24, Sergey Bronnikov wrote: >> From: Sergey Bronnikov<sergeyb@tarantool.org> >> >> The patch replaces the main test runner `prove(1)` with CTest, > Since we are dropping the usage of `prove`, we should reformulate > related comments about it, shouldn't we? Updated: --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -79,15 +79,15 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") # since some programs sanitize the environment before they start # child processes. Specifically, environment variables starting # with DYLD_ and LD_ are unset for child process started by - # other programs (like /usr/bin/prove --exec using for launching - # test suite). For more info, see the docs[2] below. + # other programs (like `ctest` using for launching tests). + # For more info, see the docs[2] below. # # These environment variables are used by FFI machinery to find # the proper shared library, hence we can still tweak testing # environment before calling <ffi.load>. However, the value # can't be passed via the standard environment variable, so we - # use env call in prove's --exec flag value to get around SIP - # magic tricks. + # use ENVIRONMENT property in `set_tests_properties` to get + # around SIP magic tricks. # # [1]: https://support.apple.com/en-us/HT204899 # [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html > Also, maybe the > LUA_TEST_ENV_MORE can be removed? I'm not sure. I suspect the same trick is required with CTest on macOS too. >> see [1] and [2]. >> >> Build and test steps looks the same as before: > Typo: s/looks/look/ Fixed. >> $ 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 operating. >> >> CMake custom target `test` was replaced by a target with the same > Typo: s/CMake/The CMake/ Fixed. >> name that is automatically generated by CMake. It is not possible >> to attach targets to this automatically generated `test` target. >> It means that target `test` is now executing `LuaJIT-test`, >> but not `LuaJIT-lint`. To mitigate this a new target > Typo: s/To mitigate this/To mitigate this,/ Fixed. >> `LuaJIT-check-all` is introduced, it has the same behaviour like > Typo: s/introduced, it/introduced. It/ > Typo: s/like/as/ Fixed. >> an old `test` target: it runs all functional tests and linters. >> >> One could use `ctest` tool: > Typo: s/use `ctest` tool/use the `ctest` tool/ Fixed. >> $ 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-tests # Run tests by label. >> $ ctest -L tarantool-c-tests # Ditto. >> $ ctest -L lua-Harness-tests # Ditto. >> $ ctest -L LuaJIT-tests # Ditto. >> $ ctest -L PUC-Rio-Lua-5.1-tests # Ditto. >> $ ctest --rerun-fail # Run only the tests >> # that failed previously. >> $ ctest --verbose # Print details to stdout. >> $ ctest --output-on-failure # Print details to stdout >> # on test failure only. >> >> 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 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 is workarounded by an introduced > Typo: s/used CMake version/the currently supported CMake version/ > Typo: s/an introduced/the introduced/ Fixed. >> macro `add_test_suite_target` that adds a CMake-test for each >> testsuite that executes a target that builds test dependencies. >> >> The commit introduce a CMake option LUAJIT_TEST_DEPS that set > Typo: s/introduce/introduces/ > Typo: s/a CMake option/the CMake option/ > Typo: s/set/sets/ Fixed. >> dependencies to be used for running and building LuaJIT tests. >> >> 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 >> --- >> .gitignore | 2 + >> CMakeLists.txt | 14 +++ >> test/CMakeLists.txt | 107 +++++++++++++++++----- >> test/LuaJIT-tests/CMakeLists.txt | 33 ++++--- >> test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++-- >> test/lua-Harness-tests/CMakeLists.txt | 50 +++++----- >> test/tarantool-c-tests/CMakeLists.txt | 58 ++++++------ >> test/tarantool-tests/CMakeLists.txt | 68 +++++++------- >> 8 files changed, 226 insertions(+), 131 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..96f85285 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 > Typo: s/to be used/to use/ or s/to be used/are used/ Fixed. >> +# LuaJIT tests. > <snipped> > >> @@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR > <snipped> > >> +if (LUAJIT_USE_TEST) > Typo: s/if (/if(/ Fixed. > <snipped> > >> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt >> index 62478441..06a1d365 100644 >> --- a/test/CMakeLists.txt >> +++ b/test/CMakeLists.txt > <snipped> > >> +set(CTEST_FLAGS >> + --output-on-failure >> + --schedule-random >> + # Timeout in seconds. Most of LuaJIT tests are fast, > Typo: s/Most of LuaJIT tests are fast, however,/Most LuaJIT tests are fast. However,/ > >> + # however, some tests can hang or execute infinite loop. > Typo: s/infinite loop/an infinite loop/ Fixed. >> + # See <tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c>, >> + # commit caa99865c206 ("test: rewrite sysprof test using managed execution"). >> + # Moreover, tests for instrumented LuaJIT requires more time. > Typo: s/requires/require/ > Fixed. >> + # 30 minutes is a sane timeout. >> + --timeout 1800 >> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL} >> +) >> +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. > Typo: s/it is unsupported/this feature is unsupported/ > Typo: s/a current/the current/ Fixed. >> +# To workaround this a function `add_test_suite_target` is > Typo: s/To workaround this a/To workaround this, the/ > Typo: s/is introduced, it/is introduced. It/ Fixed. >> +# introduced, it adds a CMake target that builds testsuite's >> +# prerequisites and CMake test that executes that target. > Typo: s/CMake test/a CMake test/ Fixed. >> +# >> +# 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 > <snipped> > >> -add_custom_target(${PROJECT_NAME}-test DEPENDS >> - LuaJIT-tests >> - PUC-Rio-Lua-5.1-tests >> - lua-Harness-tests >> - tarantool-c-tests >> - tarantool-tests >> +# Each testsuite has it's own CMake target, but combining these > Typo: s/it's/its/ Fixed. >> +# target to a single one is not desired, because each target > Typo: s/these target to/these targets into/ > Typo: s/desired,/desired/ > > Fixed. >> +# runs it's own ctest command, that each time enumerates tests > Typo: s/command, that/command, which/ > Typo? s/ctest/CTest/ replaced with `ctest` >> +# from zero and prints test summary at the end of test run. > Typo: s/prints test summary/prints the test summary/ > Typo: s/test run/ the test run/ Fixed. >> +# For common target this output looks inconvenient. > Typo: s/For common target/For a common target,/ > > Fixed. >> +# Therefore target below executes a single instance of ctest > Typo: s/Therefore target/Therefore, the target/ > Typo: s/instance of ctest/instance of the CTest/ > > Fixed. >> +# command that runs all generated CMake tests. > <snipped> > >> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt >> index 003f8de6..6ad05aa6 100644 >> --- a/test/LuaJIT-tests/CMakeLists.txt >> +++ b/test/LuaJIT-tests/CMakeLists.txt > <snipped> > >> +# 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" > Maybe it is better to use > | add_test(NAME "test/${TEST_SUITE_NAME}" > for consistency for all suites? > Also, it avoids confusing (for me, this is some kind of typo opposed to > the `add_test_suite_target()` command above). > Feel free to ignore, since it is subjectivity. Updated: diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index 6ad05aa6..b8e4dfc4 100644 --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -64,12 +64,13 @@ add_test_suite_target(LuaJIT-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" +set(test_title "test/${TEST_SUITE_NAME}") +add_test(NAME "${test_title}" 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 +set_tests_properties("${test_title}" PROPERTIES LABELS ${TEST_SUITE_NAME} ENVIRONMENT "${LUAJIT_TESTS_ENV}" DEPENDS LuaJIT-tests-deps diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt index a26e38d4..0942c62d 100644 --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt @@ -46,11 +46,12 @@ add_test_suite_target(PUC-Rio-Lua-5.1-tests # 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-tests" +set(test_title "test/${TEST_SUITE_NAME}") +add_test(NAME "${test_title}" 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-tests" PROPERTIES +set_tests_properties("${test_title}" PROPERTIES ENVIRONMENT "LUA_PATH=${LUA_PATH}" LABELS ${TEST_SUITE_NAME} DEPENDS PUC-Rio-Lua-5.1-tests-deps >> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua >> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA} >> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} >> ) > <snipped> > >> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> index 98277f9a..a26e38d4 100644 >> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt > <snipped> > >> -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-tests" >> + 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-tests" PROPERTIES >> + ENVIRONMENT "LUA_PATH=${LUA_PATH}" > Nit: Do we need to move the " back to the start of the declaration? > If it is possible, please use LUA_PATH="${LUA_PATH}" instead. it breaks suite: Start 2: test/PUC-Rio-Lua-5.1-tests 2/2 Test #2: test/PUC-Rio-Lua-5.1-tests .......***Failed 0.02 sec /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc32/src/luajit: module '/tmp/lua_joUdrU' not found: no field package.preload['/tmp/lua_joUdrU'] no file '"/tmp/lua_joUdrU' no file './build/gc64/test/tarantool-tests/lj-1087-vm-handler-call//tmp/lua_joUdrU.so' stack traceback: [C]: at 0x63fdd720d320 [C]: at 0x63fdd7145e30 /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc32/src/luajit: ...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lu a:66: assertion failed! stack traceback: [C]: in function 'assert' ...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lua:66: in function 'RUN' ...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lua:77: in function '_dofile' ...ol/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/all.lua:74: in main chunk [C]: at 0x5f99fec32e30 >> + 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..6f7b5697 100644 >> --- a/test/lua-Harness-tests/CMakeLists.txt >> +++ b/test/lua-Harness-tests/CMakeLists.txt > <snipped> > >> +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}) > Typo: s/if (/if(/ > Fixed. >> + continue() >> + endif() > <snipped> > >> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt >> index 4b1d8832..5affbb9e 100644 >> --- a/test/tarantool-c-tests/CMakeLists.txt >> +++ b/test/tarantool-c-tests/CMakeLists.txt > <snipped> > >> @@ -30,6 +19,23 @@ 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. > Typo: s/Proxy/The proxy/ > Typo: s/that builds/that build/ Fixed. >> +# This is needed because targets for each C test are generated >> +# at the same time with CMake tests, and all prerequisites must be > Typo: s/at the same time with/at the same time as/ > Typo: s/must be already exist/must already exist/ Fixed. >> +# already exist at this moment. >> +add_custom_target(tarantool-c-tests-build) > <snipped> > >> @@ -47,22 +53,16 @@ foreach(test_source ${tests}) >> ) >> target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY}) >> LIST(APPEND TESTS_COMPILED ${exe}) > Should this line be removed since TESTS_COMPILED is no longer in use? Sure, thanks! >> -endforeach() >> + add_dependencies(tarantool-c-tests-build ${exe}) >> >> -add_custom_target(tarantool-c-tests >> - DEPENDS libluajit libtest ${TESTS_COMPILED} >> -) >> - > <snipped> > >> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt >> index 35bcc5ef..9086a890 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-libs >> DEPENDS ${LUAJIT_TEST_BINARY} > Should it depend on libluajit instead? > We don't need the LuaJIT binary to link with the LuaJIT library. Fixed. --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -5,7 +5,7 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) add_custom_target(tarantool-tests-libs - DEPENDS ${LUAJIT_TEST_BINARY} + DEPENDS libluajit ) macro(BuildTestCLib lib sources) >> ) >> >> macro(BuildTestCLib lib sources) >> AddTestLib(${lib} ${sources}) >> - add_dependencies(tarantool-tests ${lib}) >> + add_dependencies(tarantool-tests-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 >> @@ -61,20 +55,12 @@ make_lua_path(LUA_PATH >> ${LUAJIT_SOURCE_DIR}/?.lua >> ${LUAJIT_BINARY_DIR}/?.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 >> @@ -118,29 +104,37 @@ endif() >> # process. >> # See also:https://github.com/tarantool/tarantool/issues/9656. >> if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") >> - # FIXME: After using CTest instead of `prove` we should set this >> - # environment variable only for the corresponding tests to avoid >> - # warnings from the libc and etc. >> + # FIXME: test_title test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua > Typo: No dot at the end of the sentence. > Nit: Comment line width is more than 66 symbols. Fixed. > Also, I didn't get the idea of the first sentence. > Also, will we add an additional patch that fixes this comment in this > patch series (as an additional commit) or will we do it later? I propose to address this later. We have an issue to set env variables specific for test, lets fix this in scope of that issue too. --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -118,9 +118,9 @@ endif() # process. # See also: https://github.com/tarantool/tarantool/issues/9656. if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU") - # FIXME: After using CTest instead of `prove` we should set this - # environment variable only for the corresponding tests to avoid - # warnings from the libc and etc. + # FIXME: We should set this environment variable only + # for the corresponding tests to avoid warnings from + # the GNU libc and other libc implementations. LibRealPath(LIB_ASAN libasan.so) list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN}) endif() >> + # With CTest we should set this environment variable only > Typo: s/With CTest/With CTest,/ Fixed. >> + # for the corresponding tests to avoid warnings from >> + # the GNU libc and other libc implementations. > Should this change be the part of the commit "test: fix > lj-802-panic-at-mcode-protfail GCC+ASan"? Fixed. >> LibRealPath(LIB_ASAN libasan.so) >> list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN}) >> 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 >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs >> ) >> >> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list >> +# with dependencies are set in scope of BuildTestLib macro. > I prefer to use the old paraphrasing of this paragraph. > Otherwise, the reader may think: "What the heck is TESTLIBS?" --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -114,9 +114,9 @@ foreach(test_path ${tests}) WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) set_tests_properties(${test_title} PROPERTIES - # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS - # list with dependencies are set in scope of BuildTestLib - # macro. + # LUA_CPATH and LD_LIBRARY_PATH variables and also + # dependencies list with libraries are set in scope of + # BuildTestLib macro. ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}" LABELS ${TEST_SUITE_NAME} DEPENDS tarantool-tests-deps >> +set(TEST_ENV "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}") > Maybe it is better to use `list(APPEND` here for better readability? It was fine before patch for ctest support. Why we need to change this? > Or even don't use the additional variable that is used once and provide > these ENVIROMENT variables directly below. Done. (but for my taste previously was better and actually it is not related to ctest patch at all) --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -121,9 +121,6 @@ add_test_suite_target(tarantool-tests DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-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) @@ -133,7 +130,10 @@ foreach(test_path ${tests}) WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) set_tests_properties(${test_title} PROPERTIES - ENVIRONMENT "${TEST_ENV}" + # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS + # list with dependencies are set in scope of BuildTestLib + # macro. + ENVIRONMENT "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}" LABELS ${TEST_SUITE_NAME} DEPENDS tarantool-tests-deps ) >> +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}") > Should it be "test/${TEST_SUITE_NAME}/${test_name}" by analogy with > other test suites? Fixed. >> + 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 >> [-- Attachment #2: Type: text/html, Size: 40115 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest 2024-04-01 14:28 ` Sergey Bronnikov via Tarantool-patches @ 2024-04-01 18:46 ` Sergey Kaplun via Tarantool-patches 0 siblings, 0 replies; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-04-01 18:46 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches Hi, Sergey! Thanks for the fixes! LGTM! <snipped> > > > Also, maybe the > > LUA_TEST_ENV_MORE can be removed? > I'm not sure. I suspect the same trick is required with CTest on macOS too. So, let it be the part of 9898[1] too. <snipped> [1]: https://github.com/tarantool/tarantool/issues/9898 -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches ` (5 preceding siblings ...) 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches @ 2024-04-11 16:47 ` Sergey Kaplun via Tarantool-patches 6 siblings, 0 replies; 17+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2024-04-11 16:47 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: tarantool-patches Sergey, I've checked the patchset into all long-term branches in tarantool/luajit and bumped a new version in master [1], release/3.0 [2] and release/2.11 [3]. [1]: https://github.com/tarantool/tarantool/pull/9938 [2]: https://github.com/tarantool/tarantool/pull/9939 [3]: https://github.com/tarantool/tarantool/pull/9940 -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-04-11 16:51 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-26 8:33 [Tarantool-patches] [PATCH luajit 0/6][v5] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 1/6] ci: execute LuaJIT tests with GCC 10 and ASAN Sergey Bronnikov via Tarantool-patches 2024-03-26 9:51 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 2/6] test: refactor CMake macro LibRealPath Sergey Bronnikov via Tarantool-patches 2024-03-26 9:57 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 3/6] test: move LibRealPath to the separate module Sergey Bronnikov via Tarantool-patches 2024-03-26 10:01 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 4/6] test: more cautious usage of LD_PRELOAD for ASan Sergey Bronnikov via Tarantool-patches 2024-03-26 10:04 ` Sergey Kaplun via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 5/6] test: fix lj-802-panic-at-mcode-protfail GCC+ASan Sergey Bronnikov via Tarantool-patches 2024-03-26 10:08 ` Sergey Kaplun via Tarantool-patches 2024-03-26 15:22 ` Sergey Bronnikov via Tarantool-patches 2024-03-26 8:33 ` [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest Sergey Bronnikov via Tarantool-patches 2024-03-26 11:03 ` Sergey Kaplun via Tarantool-patches 2024-04-01 14:28 ` Sergey Bronnikov via Tarantool-patches 2024-04-01 18:46 ` Sergey Kaplun via Tarantool-patches 2024-04-11 16:47 ` [Tarantool-patches] [PATCH luajit 0/6][v5] " Sergey Kaplun 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