From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A3D9EBC3C07; Fri, 22 Mar 2024 16:06:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A3D9EBC3C07 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1711112808; bh=ojbf7fJXDXqxVkgNdMTrzuoJaCzKO1bjImPb/kaVhKw=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=KyIofX0r81OUisTwle/GU4bZwXOOxoaulAvSl04rLwqWrBlQCY4Y3g0QyDiFdOWMI 4E8wL5zSBesY97XYSl6pqZa5ezoQoslP58+NTLc90xl8CS/rAafSKD6fgY+oo6zNf1 hAB4i+X9SrvF+YUqjcccz1bwYPQlRnv5tAULUQmE= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [95.163.41.84]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 91038BC3C07 for ; Fri, 22 Mar 2024 16:06:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 91038BC3C07 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1rnebe-0000000DnBP-2Eqw; Fri, 22 Mar 2024 16:06:47 +0300 Message-ID: Date: Fri, 22 Mar 2024 16:06:46 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun References: <62428a61c98eee87dfc91f2d8972988d80589d3e.1710226958.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92CB91DAA594FB6BF71C75EE959EE1452C6A7FC4172EA4E06182A05F53808504065F3DAFBE1CF8B0BAC8EDD30083ED68EB98C08B3605DEAAF2D09D3CDEA5E914E28006A18B89A7667 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F16C4DE526EFCC04EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D919B403F35891598638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8D1244AF87CE3C07AD54FCCEEB858E7EF74459CFF391234E2CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC82FFDA4F57982C5F4F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C706E30CA523186196136E347CC761E074AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C38142C5555699C654BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF27ED053E960B195E1DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C36174550A02D153F535872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A57D4C59BF9D872CD15002B1117B3ED6961911DFA4B0B6DFF530E4A65F242F5898823CB91A9FED034534781492E4B8EEAD3F23D99D50CED82FBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34181D1E89D5A0B42F130AE9DC16A72F17E86237B096F87E4F2996B71A5E4DFCF2E29F9B946AE9C6D01D7E09C32AA3244C93BAADD02C58D8E3459F46583F07963C0758893A80FDC548EA455F16B58544A2D06CB91D864A7BD2A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxpsVEd/A9aNOF/8kZ2ixdA== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59B23A65AE2B0037ADAC8EDD30083ED68EB98C08B3605DEAAFB7CBEF92542CD7C8795FA72BAB74744FC77752E0C033A69EA16A481184E8BB1C9B38E6EA4F046BE03A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: Sergey Bronnikov , max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, thanks for the answers. See my comments. On 3/21/24 14:17, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > Please consider my concerns below. > I suggest moving our conversation forward with the new patch version. > > On 20.03.24, Sergey Bronnikov wrote: >> Sergey, >> >> thanks for review! >> >> see my comments below >> >> On 3/18/24 18:54, Sergey Kaplun wrote: > > >>>> 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 >>> >>> >>>> diff --git a/CMakeLists.txt b/CMakeLists.txt >>>> index 7f5e2afb..6b2f855f 100644 >>>> --- a/CMakeLists.txt >>>> +++ b/CMakeLists.txt >>>> @@ -345,6 +345,13 @@ set(LUAJIT_TEST_BINARY ${LUAJIT_BINARY} CACHE STRING >>>> "Lua implementation to be used for tests. Default is 'luajit'." >>>> ) >>>> >>>> +# If LuaJIT is used in a parent project, provide an option >>>> +# to choose what dependencies to be used for running and building >>>> +# LuaJIT tests. >>>> +set(LUAJIT_TEST_DEPS "luajit-main" CACHE STRING >>>> + "A list of target dependencies to be used for tests." >>>> +) >>> Why do we use luajit-main as a target dependency for tests? >>> Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not >>> in tarantool-c-tests)? > See my comment below. > >>>> + >>>> # FIXME: If LuaJIT is used in parent project, provide an option >>>> # to pass Lua code to be run before tests are started. >>>> # XXX: Attentive reader might point to LUA_INIT environment >>>> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR >>>> "Lua code need to be run before tests are started." >>>> ) >>>> >>>> +# XXX: Enable testing via CTest. Since CTest expects to find a >>>> +# test file in the build directory root, this command should be in >>> Nit: I suggest to mentioning that this command generates "test" target. >>> Something like the following: >>> | # XXX: Enable testing via CTest. This generates `test` target. >> Added: >> >> --- a/CMakeLists.txt >> +++ b/CMakeLists.txt >> @@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT >> "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR >>    "Lua code need to be run before tests are started." >>  ) >> >> -# XXX: Enable testing via CTest. Since CTest expects to find a >> -# test file in the build directory root, this command should be in >> -# the source directory root (hence, in this CMakeLists.txt). >> +# XXX: Enable testing via CTest. This automatically generates >> +# `test` target. Since CTest expects to find a test file in > Typo: s/`test`/the `test`/ Fixed. > >> +# the build directory root, this command should be in the source >> +# directory root (hence, in this CMakeLists.txt). >>  enable_testing() >>  add_subdirectory(test) >> >> > > >>> Unfortunately, this flag does nothing for `make test` command. >>> Is there the way to fix it? >> Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that >> contains >> CTest options and is used by `test` target. As a comment says. >> >> BTW you can fix it by using CMake workflows and specify any CTest >> arguments you want in CMakePreset.json. > I got it thanks for the clarification! > May you please add a comment with the "FIXME:" mark and mention the > CMake version and the CMAKE_CTEST_ARGUMENTS variable? Already there, see v4. >>> BTW `make LuaJIT-tests` or `make tarantool-tests` work fine. >> Sure, these are our own targets and we pass desired CTest options to it. >>>> + --output-on-failure >>>> + --schedule-random >>>> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL} >>> I suppose it will be good to add the `--timeout` option here too. >>> 5 minutes as a timeout for the single test sounds more than enough. >>> >> Added. > Unfortunately, as you mentioned offline, for some builds [1], even 5 > minutes is not enough: > | 211/211 Test #109: test/tarantool-tests/lj-1034-tabov-error-frame.test.lua ........................***Timeout 300.54 sec > I suggest increasing the timeout to 30 minutes. > Already updated and added comment with explanation. >>>> +) >>>> +if(CMAKE_VERBOSE_MAKEFILE) >>>> + list(APPEND TEST_FLAGS --verbose) >>>> +endif() >>>> + >>>> +function(add_test_suite_target target) >>>> + set(prefix ARG) >>>> + set(noValues) >>>> + set(singleValues LABEL) >>>> + set(multiValues DEPENDS) >>>> + >>>> + # FIXME: if we update to CMake >= 3.5, can remove this line. >>>> + include(CMakeParseArguments) >>>> + cmake_parse_arguments(${prefix} >>>> + "${noValues}" >>>> + "${singleValues}" >>>> + "${multiValues}" >>>> + ${ARGN}) >>>> + >>>> + set(_DEPS_TARGET ${target}-deps) >>>> + >>>> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS}) >>>> + >>>> + add_test(${_DEPS_TARGET} >>> Should the NAME parameter be ${target} instead? >> Added. > Sorry, I've misguided you with my comment. I thought that we should name > the test as "${target}", not "${_DEPS_TARGET}" (I hadn't gotten the idea > of proxy targets before, but I'm OK with it now). > Please revert this change. > My bad. Actually nothing bad is happen. Change below was made after your comment: @@ -107,10 +114,10 @@ function(add_test_suite_target target)    add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS}) -  add_test(${_DEPS_TARGET} -           ${CMAKE_COMMAND} -             --build ${CMAKE_BINARY_DIR} -             --target ${_DEPS_TARGET} +  add_test(NAME ${_DEPS_TARGET} +    COMMAND ${CMAKE_COMMAND} +            --build ${CMAKE_BINARY_DIR} +            --target ${_DEPS_TARGET}    )    set_tests_properties(${_DEPS_TARGET} PROPERTIES      LABELS ${target} Seems according to BNF in [1] `NAME` is required before test name  in `add_test`. But in fact `NAME` can be omitted. I would left `NAME` to be consistent with CMake documentation. 1. https://cmake.org/cmake/help/latest/command/add_test.html > Also, please add the comment that this proxy test allows us to be sure that > all prerequisites must be built or generated before the main test > execution. Added. --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -96,6 +96,17 @@ if(CMAKE_VERBOSE_MAKEFILE)    list(APPEND CTEST_FLAGS --verbose)  endif() +# It is not possible to add dependencies to `add_test()` +# in CMake, see [1]. CMake 3.7 introduces FIXTURES_REQUIRED [2] +# and FIXTURES_SETUP [3], but these test properties cannot be +# used - it is unsupported in a current CMake version. +# To workaround this a function `add_test_suite_target` is +# introduced, it adds a CMake target that builds testsuite's +# prerequisites and CMake test that executes that target. +# +# 1. https://gitlab.kitware.com/cmake/cmake/-/issues/8774 +# 2. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html +# 3. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html  function(add_test_suite_target target)    set(prefix ARG)    set(noValues) >> >>> Or, maybe this line >>> should be ommitted since it is also used for normal test targets with >>> several tests. >> CMake test for a target with dependencies must be generated, >> >> otherwise dependencies will not be built. >> >>> For now `ctest -N` output looks a little bit inconsistent: >>> >>> | Test project /home/burii/reviews/luajit/ctest >>> | Test #1: LuaJIT-tests-deps >>> | Test #2: test/LuaJIT-tests >>> | Test #3: PUC-Rio-Lua-5.1-tests-deps >>> | Test #4: test/PUC-Rio-Lua-5.1 >>> | Test #5: lua-Harness-tests-deps >>> | Test #6: test/lua-Harness-tests/000-sanity.t >>> | ... >>> | Test #56: tarantool-c-tests-deps >>> | Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test >>> | ... >>> | Test #67: tarantool-tests-deps >>> | Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua >>> | ... >>> >>> If tests like the following are needed to run tests, I suggest naming >>> them like `LuaJIT-tests-build` to avoid confusion. >> AFAIR this naming was suggested by Igor. >> >> What kind of confusion you observe? For me something like > That we are really testing something with this strange name. > OK, my bad ignore it. > >> `LuaJIT-tests-build`  much more confusing than "xxx-deps". >> >>> | Test #1: LuaJIT-tests-deps > OK, let's leave it as it is. > > > >>>> -add_custom_target(${PROJECT_NAME}-test DEPENDS >>>> - LuaJIT-tests >>>> - PUC-Rio-Lua-5.1-tests >>>> - lua-Harness-tests >>>> - tarantool-c-tests >>>> - tarantool-tests >>>> +add_custom_target(${PROJECT_NAME}-test >>>> + COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS} >>> Why do we need to use custom command for this test? >>> IINM, all "subtargets" are used the same ${TEST_FLAGS} >>> See also Igor's comment about this test. >> Answered to Igor. >> >> because output will be ugly in that case, >> >> just change as you suggest and run LuaJIT-test. > Please, mention the desired output format in the comment. @@ -137,6 +148,13 @@ add_subdirectory(lua-Harness-tests)  add_subdirectory(tarantool-c-tests)  add_subdirectory(tarantool-tests) +# Each testsuite has it's own CMake target, but combining these +# target to a single one is not desired, because each target +# runs it's own ctest command, that each time enumerates tests +# from zero and prints test summary at the end of test run. +# For common target this output looks inconvenient. +# Therefore target below executes a single instance of ctest +# command that runs all generated CMake tests.  add_custom_target(${PROJECT_NAME}-test    COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}    DEPENDS tarantool-c-tests-deps > > > >>>> >>>> set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:") >>>> @@ -50,10 +46,11 @@ if(LUAJIT_USE_ASAN) >>>> if(CMAKE_C_COMPILER_ID STREQUAL "GNU") >>>> LibRealPath(LIB_ASAN libasan.so) >>>> # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`. >>>> - # ";" will expand to " " as we want. >>>> - list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}") >>>> + # ";" will expand to " " as we want after wrapping >>> This is not true. After these changes the environment variable >>> `LD_PRELOAD` is the following: >>> | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32 >>> But it should be: >>> | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32" >>> This is why this quotes is used. >>> For some reason, with the debug flags, paths are separated via \n: >>> | 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 >>> | 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32 >>> >>> So, I got the following error, when I build LuaJIT with GCC and use the >>> flag `-DLUAJIT_ENABLE_ASAN`: >>> | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264) >>> | #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69 >>> >> Fixed. >> >> Unfortunately there is a yet another test that will never worked in >> configuration GCC + ASAN. >> >> Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to >> fix it, but as >> >> it was never worked before we don't need to do it in scope of ctest support. > It works in our CI, where we use the clang build. > > For now, we may just disable this test for GCC + clang build with the > link to the issue. Added: --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -123,7 +123,8 @@ foreach(test_path ${tests})    )  endforeach() -# Test is broken when GCC with enabled ASAN is used. +# Test is broken when GCC with enabled ASAN is used, +# see https://github.com/tarantool/tarantool/issues/9856.  if (LUAJIT_USE_ASAN AND (CMAKE_C_COMPILER_ID STREQUAL "GNU"))    set(test_title test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua)    set_tests_properties(${test_title} PROPERTIES > > On branch, you're using global LD_PRELOAD from the aforementioned test > (lj-802-panic-at-mcode-protfail.test.lua). As a result, tons of tests > fail with the following error: > | got: PANIC: unprotected error in call to Lua API (runtime code generation failed, restricted kernel?) > > May be it is better to proxy parent LD_PRELOAD to the test instead? Ok > >> diff --git a/test/LuaJIT-tests/CMakeLists.txt >> b/test/LuaJIT-tests/CMakeLists.txt >> index 0c85f15c..4c9cf901 100644 >> --- a/test/LuaJIT-tests/CMakeLists.txt >> +++ b/test/LuaJIT-tests/CMakeLists.txt >> @@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN) >>    # https://github.com/google/sanitizers/issues/934. >>    macro(LibRealPath output lib) >>      execute_process( >> -      COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib} >> +      COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib} > Please mention in the comment that the C++ and C compilers use the same > tooling, so the returned library path is the same. Already commented in v4:        # CMAKE_C_COMPILER is used because it has the same behaviour         # as CMAKE_CXX_COMPILER, but CMAKE_CXX_COMPILER is not required         # for building LuaJIT and can be missed in GH Actions.         COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}         OUTPUT_VARIABLE LIB_LINK OUTPUT_STRIP_TRAILING_WHITESPACE         RESULT_VARIABLE RES > >>        OUTPUT_VARIABLE LIB_LINK >>        OUTPUT_STRIP_TRAILING_WHITESPACE >> +      TIMEOUT 5 > Why do we need timeout here? (*) Removed in v4. > >> +      RESULT_VARIABLE RES >>      ) >> +    if (${LIB_LINK} STREQUAL ${lib}) >> +      message(FATAL_ERROR "library ${lib} is not found") >> +    endif() >> +    if (NOT RES EQUAL 0) >> +      message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed") > Nit: the message is not very informative. I still don't know how to > rewrite it, since compilers just print the input for unexisting files. > Hence, I am not sure: do we really need this error message, since the > given command is valid? Discussed verbally and in v4 is the following message:       if (NOT RES EQUAL 0)         message(FATAL_ERROR "Executing '${CMAKE_C_COMPILER} \                              -print-file-name=${lib}' has failed")       endif() >> +    endif() >> + >>      # Fortunately, we are not interested in macOS here, so we can >>      # use realpath. >> +    # Beware, builtin command returns exit code equal to 0, >> +    # we cannot check is it failed or not. >>      execute_process( >>        COMMAND realpath ${LIB_LINK} >>        OUTPUT_VARIABLE ${output} >>        OUTPUT_STRIP_TRAILING_WHITESPACE >> +      TIMEOUT 5 > (*) And here? Removed in v4. > >>      ) >>    endmacro() >>    LibRealPath(LIB_STDCPP libstdc++.so) >> -  # XXX: GCC requires both. Clang requires only libstdc++. >>    if(CMAKE_C_COMPILER_ID STREQUAL "GNU") >>      LibRealPath(LIB_ASAN libasan.so) >> -    # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`. >> -    # ";" will expand to " " as we want. >> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}") >> +    # XXX: The items of the list in LD_PRELOAD can be separated >> +    # by spaces or colons, and there is no support for escaping >> +    # either separator, see ld.so(8). >> +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN}:${LIB_STDCPP}) >>    else() >> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}") >> +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP}) >>    endif() >>  endif() > Minor: Maybe it is better to move this change to a separate patch. > Feel free to ignore. Ignored. Before introducing CTest this worked, changes above fixes behaviour broken by patch with CTest. > > > >>>> +add_test_suite_target(LuaJIT-tests >>>> + LABEL ${TEST_SUITE_NAME} >>>> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs >>> Why don't use `${LUAJIT_TEST_BINARY}` here? >> >> AFAIR we decided with Igor in a verbal conversation >> >> to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}. >> >> LUAJIT_TEST_BINARY is a single target that can be used as a command, >> >> see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a >> list of targets >> >> that should be build before running tests. > Yes, but we are now setting LUAJIT_TEST_DEPS unconditionally. > Also, as I mentioned above for tarantool-c-tests, there is no need to > build the luajit-main target -- libluajit is enough. Hence, this change > may be dropped. > CC imun@ here. >>>> +) >>>> + >>>> +# The test suite has its own test runner >>>> +# (test/LuaJIT-tests/test.lua), it is not possible to run these >>>> +# tests one-by-one by CTest. >>>> +add_test(NAME "test/LuaJIT-tests" >>> Why don't use just LuaJIT-tests as a test name here? >> For consistency with other test titles. > Side note: It looks inconsistent anyway (partially): > > | Start 1: LuaJIT-tests-deps > | 1/211 Test #1: LuaJIT-tests-deps .............................................................. Passed 0.29 sec > | Start 2: test/LuaJIT-tests > | 2/211 Test #2: test/LuaJIT-tests .............................................................. Passed 3.56 sec > | Start 3: PUC-Rio-Lua-5.1-tests-deps > | 3/211 Test #3: PUC-Rio-Lua-5.1-tests-deps ..................................................... Passed 0.35 sec > | Start 4: test/PUC-Rio-Lua-5.1 > > As you can see, *-deps targets have no test in their name. > But I can't come up with something meaningful, so feel free to ignore > it. Okay, I left as is. >>> The same question to the tests below. >>> >>>> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua >>>> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA} >>>> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} >>>> ) >>>> +set_tests_properties("test/LuaJIT-tests" PROPERTIES >>>> + LABELS ${TEST_SUITE_NAME} >>>> + ENVIRONMENT "${LUAJIT_TESTS_ENV}" >>>> + DEPENDS LuaJIT-tests-deps >>>> +) >>>> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt >>>> index 2f90da86..f0e639e1 100644 >>>> --- a/test/LuaJIT-tests/src/CMakeLists.txt >>>> +++ b/test/LuaJIT-tests/src/CMakeLists.txt > > >>>> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}") >>> I suppose, that general part "test/" may be ommited from the title. >> I would left as is. The main idea is to map test title to a file/dir on >> the filesystem. > It's kind of obvious to me that tests are placed in the directory > "test". I'm not insisting, but the names of the tests are already long > enough, it will be nicer to decrease their length with unnecessary > information. The "test" directory is still the root directory for all > tests. It is not clear for me why you are care about test title length. ctest accepts partial test titles too: `ctest -R lj-802` (test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua will be executed) > OTOH, it is more convenient to copy-paste the filenames, but for sure, we > must use the whole filename to be consistent with the prove's output we > are replacing. > > > >>>> +# To workaround this, we glob source files and generated tests >>>> +# with paths to executable binaries. >>>> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c") >>> Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c". >>> >>> You may use the cycle above, which adds build targets. >> This is a good idea, but anyway we need two loops. >> >> First should build a list TESTS_COMPILED that is used in >> add_test_suite_target. >> >> The second one should generate  CMake tests. >> >> If you are still want to have a single loop for everything then >> >> we need to build -deps target and CMake test for each tarantool C test. > Shouldn't it be enough to set > | DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${exe} > for each test? But, yes, in this way, the order of compilation and > execution isn't sequential... > Maybe we should use the proxy target here. In the loop, we append a > dependency for this target and set it as a dependency for each test > target. May it work? seems yes diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt index a4bc3a76..abf44a46 100644 --- a/test/tarantool-c-tests/CMakeLists.txt +++ b/test/tarantool-c-tests/CMakeLists.txt @@ -19,6 +19,21 @@ AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})  # test binary can be run from any directory.  AppendFlags(TESTS_C_FLAGS "-D__LJ_TEST_DIR__='\"${CMAKE_CURRENT_SOURCE_DIR}\"'") +set(TEST_SUITE_NAME "tarantool-c-tests") + +# Proxy CMake target with all targets that builds C tests. +# This is needed because targets for each C test are generated +# at the same time with CMake tests, and all prerequisites must be +# already exist at this moment. +add_custom_target(tarantool-c-tests-build) + +# XXX: The call produces both test and target +# as a side effect. +add_test_suite_target(tarantool-c-tests +  LABELS ${TEST_SUITE_NAME} +  DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest tarantool-c-tests-build +) +  set(CTEST_SRC_SUFFIX ".test.c")  file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")  foreach(test_source ${tests}) @@ -36,27 +51,12 @@ foreach(test_source ${tests})    )    target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})    LIST(APPEND TESTS_COMPILED ${exe}) -endforeach() - -set(TEST_SUITE_NAME "tarantool-c-tests") - -# XXX: The call produces both test and target -# as a side effect. -add_test_suite_target(tarantool-c-tests -  LABELS ${TEST_SUITE_NAME} -  DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED} -) +  add_dependencies(tarantool-c-tests-build ${exe}) -# XXX: Note that we cannot glob generated files in CMake, see -# https://stackoverflow.com/questions/44076307/cmake-globbing-generated-files. -# To workaround this, we glob source files and generated tests -# with paths to executable binaries. -file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${CTEST_SRC_SUFFIX}") -foreach(test_path ${tests}) -  get_filename_component(test_name ${test_path} NAME_WE) -  set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}") +  # Generate CMake tests. +  set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")    add_test(NAME ${test_title} -    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX} +    COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX}      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}    )    set_tests_properties(${test_title} PROPERTIES > >> Left without changes. > May you please add a comment about it to avoid confusion in the future? patch is above > > > >>>> +set(TEST_SUITE_NAME "tarantool-tests") >>>> + >>>> +# XXX: The call produces both test and target >>>> +# as a side effect. >>>> +add_test_suite_target(tarantool-tests >>>> + LABEL ${TEST_SUITE_NAME} >>>> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs >>>> ) >>>> >>>> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list >>>> +# with dependencies are set in scope of BuildTestLib macro. >>>> +set(TEST_ENV "LUA_PATH=${LUA_PATH};\;\;;LUA_CPATH=${LUA_CPATH}\;\;;${LUA_TEST_ENV_MORE}") >>>> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") >>> Why do we need GLOB_RECURSE here, instead of GLOB? >> >> it is better to search test files recursively, no one can guarantee that >> files always will be in a top testsuite dir. > Ok, thanks for the clarification. > > >