[Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest
Sergey Bronnikov
sergeyb at tarantool.org
Wed Mar 20 22:44:02 MSK 2024
Sergey,
thanks for review!
see my comments below
On 3/18/24 18:54, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> I'll proceed with a review within the original letter.
>
> I look forward to applying these changes!
> I hope we can use the full power of CTest. It will be nice to configure
> heavy-memory tests to avoid running them together in parallel and
> hanging the system:).
>
> Please, consider my comments below.
>
> On 12.03.24, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb at tarantool.org>
>>
>> The patch replaces the main test runner `prove(1)` with CTest,
>> see [1] and [2].
>>
>> Build and test steps looks the same as before:
>>
>> $ cmake -S . -B build
>> $ cmake --build build --parallel
>> $ cmake --build build --target [LuaJIT-test, test]
>>
>> CMake targets lua-Harness-tests, LuaJIT-tests, PUC-Rio-Lua-5.1-tests,
>> tarantool-tests and tarantool-c-tests are still functional.
>>
>> One could use `ctest` tool:
>>
>> $ ctest --print-labels
>> Test project <snipped>
>> All Labels:
>> LuaJIT
>> PUC-Rio-Lua-5.1
>> lua-Harness
>> tarantool
>> tarantool-c
>> $ ctest # Run all tests.
>> $ ctest -L tarantool # Run tests in tarantool-tests dir.
>> $ ctest -L tarantool-c # Run tests in tarantool-c-tests dir.
>> $ ctest -L lua-Harness # Run tests in lua-Harness-tests dir.
>> $ ctest -L luajit # Run tests in LuaJIT-tests dir.
>> $ ctest -L PUC-Rio-Lua-5.1 # Run tests in PUC-Rio-Lua-5.1-tests dir.
> Please, remove the excess amount of spaces (9) in the formatting to make
> the commit message fits 72 symbols.
Fixed.
>
> Same for the paragraphs below.
Fixed.
>> $ ctest --rerun-fail
>> $ ctest --output-on-failure
>>
>> The benefits are:
>>
>> - less external dependencies, `prove(1)` is gone, `ctest` is a part of CMake
>> - running tests by test title
>> - extended test running options in comparison to prove(1)
>> - unified test output (finally!)
>>
>> Note that it is not possible to attach targets to the automatically
>> generated `test` target. It means that target `test` is now executing
>> `LuaJIT-test`, but not `LuaJIT-lint`.
> Is it possible to introduce a new target like test-all to join them?
Discussed target name in a private conversation and decided to introduce
"LuaJIT-check-all".
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -140,3 +140,8 @@ add_custom_target(${PROJECT_NAME}-test
PUC-Rio-Lua-5.1-tests-deps
LuaJIT-tests-deps
)
+
+add_custom_target(${PROJECT_NAME}-check-all
+ DEPENDS ${PROJECT_NAME}-test
+ ${PROJECT_NAME}-lint
+)
>
>> Note that the testsuites in `test/LuaJIT-tests` and
>> `test/PUC-Rio-Lua-5.1` directories have their own test runners written
>> in Lua and currently CTest runs these testsuites as single tests. In the
> Typo: s/Lua/Lua,/
>
Added comma.
>> future we could change these test runners to specify tests from outside,
> Typo: s/future/future,/
Added comma.
>
>> and after this we could run tests separately by CTest in these test
> Typo: s/this/this,/
Added comma.
>
>> suites.
>>
>> Note that it is not possible to add dependencies to `add_test()` in
>> CMake, see [3]. CMake 3.7 introduces FIXTURES_REQUIRED [4] and
>> FIXTURES_SETUP [5], but these test properties cannot be used - used
>> CMake version is lower. This workarounded by introducing a special test
> Typo: s/workarounded/is workaraounded/
Typo is in "workaraounded".
Fixed.
>
>> for each testsuite that executes a target that builds test dependencies.
>>
>> 1. https://cmake.org/cmake/help/latest/manual/ctest.1.html
>> 2. https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html
>> 3. https://gitlab.kitware.com/cmake/cmake/-/issues/8774
>> 4. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html
>> 5. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html
>> ---
>> Changes v3:
>> - rebased to master
>> - applied fixes suggested by Igor Munkin
>>
>> PR: https://github.com/tarantool/tarantool/pull/9286
>> Branch: https://github.com/tarantool/luajit/tree/ligurio/enable_test_target
>>
>> .gitignore | 2 +
>> CMakeLists.txt | 11 ++++
>> test/CMakeLists.txt | 78 +++++++++++++++--------
>> test/LuaJIT-tests/CMakeLists.txt | 40 +++++++-----
>> test/LuaJIT-tests/src/CMakeLists.txt | 2 +-
>> test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++++---
>> test/lua-Harness-tests/CMakeLists.txt | 48 ++++++++------
>> test/tarantool-c-tests/CMakeLists.txt | 56 ++++++++--------
>> test/tarantool-tests/CMakeLists.txt | 62 ++++++++----------
>> 9 files changed, 194 insertions(+), 130 deletions(-)
>>
>> diff --git a/.gitignore b/.gitignore
>> index dc5ea5fc..dd1f8888 100644
>> --- a/.gitignore
>> +++ b/.gitignore
> <snipped>
>
>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>> index 7f5e2afb..6b2f855f 100644
>> --- a/CMakeLists.txt
>> +++ b/CMakeLists.txt
>> @@ -345,6 +345,13 @@ set(LUAJIT_TEST_BINARY ${LUAJIT_BINARY} CACHE STRING
>> "Lua implementation to be used for tests. Default is 'luajit'."
>> )
>>
>> +# If LuaJIT is used in a parent project, provide an option
>> +# to choose what dependencies to be used for running and building
>> +# LuaJIT tests.
>> +set(LUAJIT_TEST_DEPS "luajit-main" CACHE STRING
>> + "A list of target dependencies to be used for tests."
>> +)
> Why do we use luajit-main as a target dependency for tests?
> Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
> in tarantool-c-tests)?
>
>> +
>> # FIXME: If LuaJIT is used in parent project, provide an option
>> # to pass Lua code to be run before tests are started.
>> # XXX: Attentive reader might point to LUA_INIT environment
>> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>> "Lua code need to be run before tests are started."
>> )
>>
>> +# XXX: Enable testing via CTest. Since CTest expects to find a
>> +# test file in the build directory root, this command should be in
> Nit: I suggest to mentioning that this command generates "test" target.
> Something like the following:
> | # XXX: Enable testing via CTest. This generates `test` target.
Added:
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT
"${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
"Lua code need to be run before tests are started."
)
-# XXX: Enable testing via CTest. Since CTest expects to find a
-# test file in the build directory root, this command should be in
-# the source directory root (hence, in this CMakeLists.txt).
+# XXX: Enable testing via CTest. This automatically generates
+# `test` target. Since CTest expects to find a test file in
+# the build directory root, this command should be in the source
+# directory root (hence, in this CMakeLists.txt).
enable_testing()
add_subdirectory(test)
>
>> +# the source directory root (hence, in this CMakeLists.txt).
>> +enable_testing()
> Should we generate the `test` target only if the `LUAJIT_USE_TEST`
> option is set, like it was done before (see relevant changes in the
> <test/CMakeLists.txt>)?
Added option LUAJIT_USE_TEST back:
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -373,7 +373,9 @@ set(LUAJIT_TEST_INIT
"${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
# `test` target. Since CTest expects to find a test file in
# the build directory root, this command should be in the source
# directory root (hence, in this CMakeLists.txt).
-enable_testing()
+if (LUAJIT_USE_TEST)
+ enable_testing()
+endif()
add_subdirectory(test)
# --- Misc rules
---------------------------------------------------------------
>> add_subdirectory(test)
>>
>> # --- Misc rules ---------------------------------------------------------------
>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>> index 3ad5d15f..90aaead2 100644
>> --- a/test/CMakeLists.txt
>> +++ b/test/CMakeLists.txt
>> @@ -77,35 +77,63 @@ separate_arguments(LUAJIT_TEST_COMMAND)
>> set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
>> include(AddTestLib)
>>
>> +# TEST_FLAGS is used by CMake targets in test suites.
>> +# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains CTest options
> Nit: the comment line is wider than 66 symbols.
Already fixed.
>
>> +# and is used by `test` target.
>> +set(TEST_FLAGS
> Minor: Maybe it is better to name this variable as `CTEST_FLAGS`, to
> avoid confusion that these flags are arguments for some internal test
> runner?
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 899bf31a..ca298118 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -77,16 +77,16 @@ separate_arguments(LUAJIT_TEST_COMMAND)
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
include(AddTestLib)
-# TEST_FLAGS is used by CMake targets in test suites.
+# CTEST_FLAGS is used by CMake targets in test suites.
# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains
# CTest options and is used by `test` target.
-set(TEST_FLAGS
+set(CTEST_FLAGS
--output-on-failure
--schedule-random
--parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
)
if(CMAKE_VERBOSE_MAKEFILE)
- list(APPEND TEST_FLAGS --verbose)
+ list(APPEND CTEST_FLAGS --verbose)
endif()
function(add_test_suite_target target)
@@ -119,7 +119,7 @@ function(add_test_suite_target target)
message(STATUS "Add test suite ${ARG_LABEL}")
add_custom_target(${target}
- COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
+ COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${CTEST_FLAGS}
DEPENDS ${_DEPS_TARGET}
)
@@ -133,7 +133,7 @@ add_subdirectory(tarantool-c-tests)
add_subdirectory(tarantool-tests)
add_custom_target(${PROJECT_NAME}-test
- COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}
+ COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}
DEPENDS tarantool-c-tests-deps
tarantool-tests-deps
lua-Harness-tests-deps
>
> Unfortunately, this flag does nothing for `make test` command.
> Is there the way to fix it?
Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that
contains
CTest options and is used by `test` target. As a comment says.
BTW you can fix it by using CMake workflows and specify any CTest
arguments you want in CMakePreset.json.
> BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
Sure, these are our own targets and we pass desired CTest options to it.
>> + --output-on-failure
>> + --schedule-random
>> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
> I suppose it will be good to add the `--timeout` option here too.
> 5 minutes as a timeout for the single test sounds more than enough.
>
Added.
>> +)
>> +if(CMAKE_VERBOSE_MAKEFILE)
>> + list(APPEND TEST_FLAGS --verbose)
>> +endif()
>> +
>> +function(add_test_suite_target target)
>> + set(prefix ARG)
>> + set(noValues)
>> + set(singleValues LABEL)
>> + set(multiValues DEPENDS)
>> +
>> + # FIXME: if we update to CMake >= 3.5, can remove this line.
>> + include(CMakeParseArguments)
>> + cmake_parse_arguments(${prefix}
>> + "${noValues}"
>> + "${singleValues}"
>> + "${multiValues}"
>> + ${ARGN})
>> +
>> + set(_DEPS_TARGET ${target}-deps)
>> +
>> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
>> +
>> + add_test(${_DEPS_TARGET}
> Should the NAME parameter be ${target} instead?
Added.
> Or, maybe this line
> should be ommitted since it is also used for normal test targets with
> several tests.
CMake test for a target with dependencies must be generated,
otherwise dependencies will not be built.
>
> For now `ctest -N` output looks a little bit inconsistent:
>
> | Test project /home/burii/reviews/luajit/ctest
> | Test #1: LuaJIT-tests-deps
> | Test #2: test/LuaJIT-tests
> | Test #3: PUC-Rio-Lua-5.1-tests-deps
> | Test #4: test/PUC-Rio-Lua-5.1
> | Test #5: lua-Harness-tests-deps
> | Test #6: test/lua-Harness-tests/000-sanity.t
> | ...
> | Test #56: tarantool-c-tests-deps
> | Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
> | ...
> | Test #67: tarantool-tests-deps
> | Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
> | ...
>
> If tests like the following are needed to run tests, I suggest naming
> them like `LuaJIT-tests-build` to avoid confusion.
AFAIR this naming was suggested by Igor.
What kind of confusion you observe? For me something like
`LuaJIT-tests-build` much more confusing than "xxx-deps".
> | Test #1: LuaJIT-tests-deps
>
> Also, as discussed with Sergey offline:
> For now, we observe an error since dependencies (C libraries) for tests
> are not built if we build LuaJIT and try to run ctest target like the
> following (without using make for a specific test target):
>
> | cmake . && make -j
> | ctest -L tarantool-tests --verbose
> | 68: /home/burii/reviews/luajit/ctest/src/luajit: .../test/tarantool-tests/arm64-ccall-fp-convention.test.lua:4: libfficcall.so: cannot open shared object file: No such file or directory
>
it was fixed
>> + ${CMAKE_COMMAND}
>> + --build ${CMAKE_BINARY_DIR}
>> + --target ${_DEPS_TARGET}
>> + )
>> +
>> + message(STATUS "Add test suite ${ARG_LABEL}")
> Note: If we don't run this function for multi-test targets after
> applying the changes required above, this STATUS message should be
> copied to the correponding CMakeLists.txt.
This macro executed one time for each test suite.
>
>> +
>> + add_custom_target(${target}
>> + COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS}
>> + DEPENDS ${_DEPS_TARGET}
>> + )
>> +
>> + unset(_DEPS_TARGET)
> There is no need in `unset()` since we are using `function()` here,
> which opens a new scope.
Removed.
>
>> +endfunction()
>> +
>> add_subdirectory(LuaJIT-tests)
>> add_subdirectory(PUC-Rio-Lua-5.1-tests)
>> add_subdirectory(tarantool-c-tests)
>> add_subdirectory(tarantool-tests)
>>
>> -add_custom_target(${PROJECT_NAME}-test DEPENDS
>> - LuaJIT-tests
>> - PUC-Rio-Lua-5.1-tests
>> - lua-Harness-tests
>> - tarantool-c-tests
>> - tarantool-tests
>> +add_custom_target(${PROJECT_NAME}-test
>> + COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}
> Why do we need to use custom command for this test?
> IINM, all "subtargets" are used the same ${TEST_FLAGS}
> See also Igor's comment about this test.
Answered to Igor.
because output will be ugly in that case,
just change as you suggest and run LuaJIT-test.
>
>> + DEPENDS tarantool-c-tests-deps
>> + tarantool-tests-deps
>> + lua-Harness-tests-deps
>> + PUC-Rio-Lua-5.1-tests-deps
>> + LuaJIT-tests-deps
>> )
>> -
>> -if(LUAJIT_USE_TEST)
> Within this change the `test` target is always generated.
> See the comment above.
Generated when LUAJIT_USE_TEST is ON.
>
>> - if(POLICY CMP0037)
> <snipped>
>
>> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
>> index a0fb5440..1080987a 100644
>> --- a/test/LuaJIT-tests/CMakeLists.txt
>> +++ b/test/LuaJIT-tests/CMakeLists.txt
>> @@ -3,17 +3,13 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>>
>> add_subdirectory(src)
>>
>> -add_custom_target(LuaJIT-tests
>> - DEPENDS ${LUAJIT_TEST_BINARY} LuaJIT-tests-libs
>> -)
>> -
>> make_lua_path(LUA_CPATH
>> PATHS
>> ${CMAKE_CURRENT_BINARY_DIR}/src/?${CMAKE_SHARED_LIBRARY_SUFFIX}
>> )
>>
>> set(LUAJIT_TESTS_ENV
>> - "LUA_CPATH=\"${LUA_CPATH}\""
>> + "LUA_CPATH=${LUA_CPATH}\;\;"
>> )
> These semicolons look excessive, see also Igor's comment.
>
> Side note: I suppose that double quotes are used to avoid escaping
> various special symbols or spaces. But for now, all these special cases
> of directory names are just not working, so I see nothing crucial in
> removing these quotes.
already fixed and force-pushed
>>
>> set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:")
>> @@ -50,10 +46,11 @@ if(LUAJIT_USE_ASAN)
>> if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
>> LibRealPath(LIB_ASAN libasan.so)
>> # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`.
>> - # ";" will expand to " " as we want.
>> - list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}")
>> + # ";" will expand to " " as we want after wrapping
> This is not true. After these changes the environment variable
> `LD_PRELOAD` is the following:
> | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
> But it should be:
> | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
> This is why this quotes is used.
> For some reason, with the debug flags, paths are separated via \n:
> | 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
> | 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
>
> So, I got the following error, when I build LuaJIT with GCC and use the
> flag `-DLUAJIT_ENABLE_ASAN`:
> | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
> | #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69
>
Fixed.
Unfortunately there is a yet another test that will never worked in
configuration GCC + ASAN.
Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to
fix it, but as
it was never worked before we don't need to do it in scope of ctest support.
diff --git a/test/LuaJIT-tests/CMakeLists.txt
b/test/LuaJIT-tests/CMakeLists.txt
index 0c85f15c..4c9cf901 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)
# https://github.com/google/sanitizers/issues/934.
macro(LibRealPath output lib)
execute_process(
- COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
+ COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
OUTPUT_VARIABLE LIB_LINK
OUTPUT_STRIP_TRAILING_WHITESPACE
+ TIMEOUT 5
+ RESULT_VARIABLE RES
)
+ if (${LIB_LINK} STREQUAL ${lib})
+ message(FATAL_ERROR "library ${lib} is not found")
+ endif()
+ if (NOT RES EQUAL 0)
+ message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed")
+ endif()
+
# Fortunately, we are not interested in macOS here, so we can
# use realpath.
+ # Beware, builtin command returns exit code equal to 0,
+ # we cannot check is it failed or not.
execute_process(
COMMAND realpath ${LIB_LINK}
OUTPUT_VARIABLE ${output}
OUTPUT_STRIP_TRAILING_WHITESPACE
+ TIMEOUT 5
)
endmacro()
LibRealPath(LIB_STDCPP libstdc++.so)
- # XXX: GCC requires both. Clang requires only libstdc++.
if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
LibRealPath(LIB_ASAN libasan.so)
- # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`.
- # ";" will expand to " " as we want.
- list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}")
+ # XXX: The items of the list in LD_PRELOAD can be separated
+ # by spaces or colons, and there is no support for escaping
+ # either separator, see ld.so(8).
+ list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN}:${LIB_STDCPP})
else()
- list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}")
+ list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP})
endif()
endif()
>> + # LUAJIT_TESTS_ENV by double quotes.
>> + list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN};${LIB_STDCPP})
>> else()
>> - list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}")
>> + list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP})
>> endif()
>> endif()
>>
>> @@ -64,12 +61,25 @@ if(LUAJIT_NO_UNWIND)
> <snipped>
>
>> +set(TEST_SUITE_NAME "LuaJIT-tests")
>> +
>> +# XXX: The call produces both test and target
>> +# <LuaJIT-tests-deps> as a side effect.
> Nit: It looks like the <LuaJIT-tests-deps> quotation may be placed at
> the previous line, fitting 66 symbols of the comment width.
Ok, added to the previous line
>> +add_test_suite_target(LuaJIT-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
> Why don't use `${LUAJIT_TEST_BINARY}` here?
AFAIR we decided with Igor in a verbal conversation
to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}.
LUAJIT_TEST_BINARY is a single target that can be used as a command,
see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a
list of targets
that should be build before running tests.
>
>> +)
>> +
>> +# The test suite has its own test runner
>> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
>> +# tests one-by-one by CTest.
>> +add_test(NAME "test/LuaJIT-tests"
> Why don't use just LuaJIT-tests as a test name here?
For consistency with other test titles.
> The same question to the tests below.
>
>> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
>> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
>> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
>> )
>> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
>> + LABELS ${TEST_SUITE_NAME}
>> + ENVIRONMENT "${LUAJIT_TESTS_ENV}"
>> + DEPENDS LuaJIT-tests-deps
>> +)
>> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
>> index 2f90da86..f0e639e1 100644
>> --- a/test/LuaJIT-tests/src/CMakeLists.txt
>> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
>> @@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c)
>> enable_language(CXX)
>> AddTestLib(libcpptest libcpptest.cpp)
>>
>> -add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS})
>> +add_custom_target(LuaJIT-tests-deps-libs DEPENDS ${TESTLIBS})
> Why do you prefer to rename this target? I see no conflict between this
> name and the previous version.
Seems this a change made by Igor. I'm totally ok with a previous name.
reverted
>
>> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
>> index 98277f9a..af89bfe5 100644
>> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
>> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
>> @@ -34,17 +34,26 @@ add_subdirectory(libs)
>> # But, unfortunately, <ltests.c> depends on specific PUC-Rio
>> # Lua 5.1 internal headers and should be adapted for LuaJIT.
>>
>> -add_custom_target(PUC-Rio-Lua-5.1-tests
>> - DEPENDS ${LUAJIT_TEST_BINARY} PUC-Rio-Lua-5.1-tests-prepare
>> +set(TEST_SUITE_NAME "PUC-Rio-Lua-5.1-tests")
>> +
>> +# XXX: The call produces both test and target
>> +# <PUC-Rio-Lua-5.1-tests-deps> as a side effect.
>> +add_test_suite_target(PUC-Rio-Lua-5.1-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} PUC-Rio-Lua-5.1-tests-prepare
> Why don't use `${LUAJIT_TEST_BINARY}` here?
> Here and below.
the same as above
>
>> )
>>
>> -add_custom_command(TARGET PUC-Rio-Lua-5.1-tests
>> - COMMENT "Running PUC-Rio Lua 5.1 tests"
>> - COMMAND
>> - env
>> - LUA_PATH="${LUA_PATH}"
>> - ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua
>> +# The test suite has its own test runner
>> +# (test/PUC-Rio-Lua-5.1-tests/all.lua), it is not possible
>> +# to run these tests one-by-one by CTest.
>> +add_test(NAME "test/PUC-Rio-Lua-5.1"
>> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua
>> WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>> )
>> +set_tests_properties("test/PUC-Rio-Lua-5.1" PROPERTIES
>> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;\;"
> Typo: These semicolons look excessive.
Removed.
>
>> + LABELS ${TEST_SUITE_NAME}
>> + DEPENDS PUC-Rio-Lua-5.1-tests-deps
>> +)
>>
>> # vim: expandtab tabstop=2 shiftwidth=2
>> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
>> index f748a8fd..d454ba41 100644
>> --- a/test/lua-Harness-tests/CMakeLists.txt
>> +++ b/test/lua-Harness-tests/CMakeLists.txt
>> @@ -4,12 +4,6 @@
> <snipped>
>
>> +# XXX: The call produces both test and target
>> +# <lua-Harness-tests-deps> as a side effect.
>> +add_test_suite_target(lua-Harness-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS}
>> )
>>
>> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.t")
>> +foreach(test_path ${tests})
>> + get_filename_component(test_name ${test_path} NAME)
>> + # FIXME: By default GLOB lists directories.
> Typo: s/By default/By default,/
Fixed.
>
>> + # Directories are omitted in the result if LIST_DIRECTORIES
>> + # is set to false. New in version CMake 3.3.
>> + if (${test_name} STREQUAL ${TEST_SUITE_NAME})
>> + continue()
>> + endif (${test_name} STREQUAL ${TEST_SUITE_NAME})
> Typo: s/endif (/endif(/
Already fixed.
>
>> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
> I suppose, that general part "test/" may be ommited from the title.
I would left as is. The main idea is to map test title to a file/dir on
the filesystem.
>
>> + add_test(NAME ${test_title}
>> + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path}
>> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> Minor: Looks like misindent.
It is ok in a current version.
>
>> + )
>> + set_tests_properties(${test_title} PROPERTIES
>> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;"
> Minor: Semicolon looks excess here.
It is ok in a current version.
>
>> + LABELS ${TEST_SUITE_NAME}
>> + DEPENDS lua-Harness-tests-deps
>> + )
>> +endforeach()
>> +
>> # vim: expandtab tabstop=2 shiftwidth=2
>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
>> index 4b1d8832..2de4a0c8 100644
>> --- a/test/tarantool-c-tests/CMakeLists.txt
>> +++ b/test/tarantool-c-tests/CMakeLists.txt
>> @@ -1,15 +1,4 @@
> <snipped>
>
>> # Build libtest.
>>
>> @@ -49,20 +38,35 @@ foreach(test_source ${tests})
>> LIST(APPEND TESTS_COMPILED ${exe})
>> endforeach()
>>
>> -add_custom_target(tarantool-c-tests
>> - DEPENDS libluajit libtest ${TESTS_COMPILED}
>> -)
>> +set(TEST_SUITE_NAME "tarantool-c-tests")
>>
>> -add_custom_command(TARGET tarantool-c-tests
>> - COMMENT "Running Tarantool C tests"
>> - COMMAND
>> - ${PROVE}
>> - ${CMAKE_CURRENT_BINARY_DIR}
>> - --ext ${C_TEST_SUFFIX}
>> - --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
>> - # Report any TAP parse errors, if any, since test module is
>> - # maintained by us.
>> - --parse
>> - ${C_TEST_FLAGS}
>> - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>> +# XXX: The call produces both test and target
>> +# <LuaJIT-tests-deps> as a side effect.
> Typo: s/LuaJIT-tests-deps/tarantool-c-tests-deps/
It is ok in a current version.
>> +add_test_suite_target(tarantool-c-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED}
>> )
>> +
>> +# XXX: Note that we cannot glob generated files in CMake, see
>> +# https://stackoverflow.com/questions/44076307/cmake-globbing-generated-files
> Nit: Missed dot at the end of the sentence.
Added a dot to the end of sentence.
>
>> +# To workaround this, we glob source files and generated tests
>> +# with paths to executable binaries.
>> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
> Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".
>
> You may use the cycle above, which adds build targets.
This is a good idea, but anyway we need two loops.
First should build a list TESTS_COMPILED that is used in
add_test_suite_target.
The second one should generate CMake tests.
If you are still want to have a single loop for everything then
we need to build -deps target and CMake test for each tarantool C test.
Left without changes.
>
>> +foreach(test_path ${tests})
>> + get_filename_component(test_name ${test_path} NAME_WE)
>> + # FIXME: By default GLOB lists directories.
>> + # Directories are omitted in the result if LIST_DIRECTORIES
>> + # is set to false. New in version CMake 3.3.
>> + if (${test_name} STREQUAL ${TEST_SUITE_NAME})
>> + continue()
>> + endif (${test_name} STREQUAL ${TEST_SUITE_NAME})
> Typo: s/endif (/endif(/
this is ok in a current version
>
>> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}")
> I suppose, that general part "test/" may be ommited from the title.
No, explained above.
>
>> + add_test(NAME ${test_title}
>> + COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX}
>> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>> + )
>> + set_tests_properties(${test_title} PROPERTIES
>> + LABELS ${TEST_SUITE_NAME}
>> + DEPENDS tarantool-c-tests-deps
>> + )
>> +endforeach()
>> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>> index e6d12984..bb41e53b 100644
>> --- a/test/tarantool-tests/CMakeLists.txt
>> +++ b/test/tarantool-tests/CMakeLists.txt
>> @@ -4,19 +4,13 @@
> <snipped>
>
>> -add_custom_target(tarantool-tests
>> +add_custom_target(tarantool-tests-deps-libs
> Why do you prefer to rename this target? I see no conflict between this
> name and the previous version.
As I said before I'm totally ok with old name.
The change was introduced when "Igor automatically squashed all the
tweaks made to the original commit."
Reverted
>
>> DEPENDS ${LUAJIT_TEST_BINARY}
>> )
>>
>> macro(BuildTestCLib lib sources)
>> AddTestLib(${lib} ${sources})
>> - add_dependencies(tarantool-tests ${lib})
>> + add_dependencies(tarantool-tests-deps-libs ${lib})
>> # Add the directory where the lib is built to the list with
>> # entries for LUA_CPATH environment variable, so LuaJIT can find
>> # and load it. See the comment about extending the list in the
>> @@ -60,21 +54,14 @@ make_lua_path(LUA_PATH
>> ${PROJECT_SOURCE_DIR}/tools/?.lua
>> ${LUAJIT_SOURCE_DIR}/?.lua
>> ${LUAJIT_BINARY_DIR}/?.lua
>> + ${PROJECT_BINARY_DIR}/src/?.lua
> This is the same directory as a ${LUAJIT_SOURCE_DIR}/?.lua.
fixed in a current version
>
>> )
>> +
>> # Update LUA_CPATH with the library paths collected within
>> # <BuildTestLib> macro.
>> make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS})
>>
> <snipped>
>
>> +set(TEST_SUITE_NAME "tarantool-tests")
>> +
>> +# XXX: The call produces both test and target
>> +# <tarantool-tests-deps> as a side effect.
>> +add_test_suite_target(tarantool-tests
>> + LABEL ${TEST_SUITE_NAME}
>> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
>> )
>>
>> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
>> +# with dependencies are set in scope of BuildTestLib macro.
>> +set(TEST_ENV "LUA_PATH=${LUA_PATH};\;\;;LUA_CPATH=${LUA_CPATH}\;\;;${LUA_TEST_ENV_MORE}")
>> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> Why do we need GLOB_RECURSE here, instead of GLOB?
it is better to search test files recursively, no one can guarantee that
files always will be in a top testsuite dir.
>
>> +foreach(test_path ${tests})
>> + get_filename_component(test_name ${test_path} NAME)
>> + set(test_title "test/tarantool-tests/${test_name}")
> I suppose, that general part "test/" may be ommited from the title.
answered above
>
>> + add_test(NAME ${test_title}
>> + COMMAND ${LUAJIT_TEST_COMMAND} ${test_path}
>> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>> + )
>> + set_tests_properties(${test_title} PROPERTIES
>> + ENVIRONMENT "${TEST_ENV}"
>> + LABELS ${TEST_SUITE_NAME}
>> + DEPENDS tarantool-tests-deps
>> + )
>> +endforeach()
>> --
>> 2.34.1
>>
More information about the Tarantool-patches
mailing list