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 9BDE2AFDDDD; Mon, 1 Apr 2024 17:28:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9BDE2AFDDDD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1711981710; bh=S5KdgLU/nS9GN6yQjir5ueCylagvxZiSxrepND688wc=; 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=OvzPicaIzYSTnLvMrmKTo816rx7ZipCpUZ25r63EuYdLmMaRee8XWLSQghVqWfvy1 Xhxu9oIoirDmjNQAYXf7VOLbemm2AwG/HQXptNb4Hkqm82V5e1ITPWon72m7Y1b5TZ 3RtBorALV8GIJu5kgaOlFP3ESAnXNilCO2spML4I= Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [95.163.41.85]) (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 418BDAFDDDB for ; Mon, 1 Apr 2024 17:28:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 418BDAFDDDB Received: by smtp47.i.mail.ru with esmtpa (envelope-from ) id 1rrIeB-0000000HLgR-3YHZ; Mon, 01 Apr 2024 17:28:28 +0300 Content-Type: multipart/alternative; boundary="------------DYQZsOd3yPDqaISEkvPhz0Sg" Message-ID: <38e7e06a-1575-4062-8a4b-f97e16edb222@tarantool.org> Date: Mon, 1 Apr 2024 17:28:27 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun , Sergey Bronnikov References: <1d967a53d2d8783ef9f66c0b9d56f64a87a318cb.1711441864.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD99EF44B444080F1C54DE452A310190A1D51661F2982CEA489182A05F5380850404C228DA9ACA6FE27C5F0631F1FD9DECF89CEA9C911D0999582F0F5540DFBC2F5E5F96EE45BBCDCD1CAD80AB92358092F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F2919D563845004AEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063714A07403F1CB524B8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A4908F4A465B9AE2DBBA8F1EF2677A8E4AB90DD0A537DBB0CC7F00164DA146DAFE8445B8C89999728AA50765F79006375FFD5C25497261569FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C353FA85A707D24CADCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C22494A19EED482128B833AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F7900637AABC089B4CD4981CEC76A7562686271ED91E3A1F190DE8FD2E808ACE2090B5E14AD6D5ED66289B5278DA827A17800CE7A03E8F3C2D3812562EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE73CC03873634D8B89731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A557A6D74FB5D5501F5002B1117B3ED696CC3205CAE82A9B315D145BB8EF0DE66B823CB91A9FED034534781492E4B8EEAD572B23B2DFF1AC44BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFD4487F5D2A9A13A8E59C2CE0A3F8AA5B744B809DF663A37BA522F00D7D4D074085EFCC6ECD45420258CF1FE2B37DBE10303F8FA9C190CB09E231FF43EA83A2D680BDE161E15E3F21C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojZkLXUwYT7t8wNUX1iyZiTg== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59EDC2E8D68A448EF189CEA9C911D0999582F0F5540DFBC2F5B7CBEF92542CD7C8795FA72BAB74744FC77752E0C033A69EA16A481184E8BB1C9B38E6EA4F046BE03A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 6/6] 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: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------DYQZsOd3yPDqaISEkvPhz0Sg Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >> >> 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 . 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 >> 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 > > >> 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. > > >> @@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR > > >> +if (LUAJIT_USE_TEST) > Typo: s/if (/if(/ Fixed. > > >> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt >> index 62478441..06a1d365 100644 >> --- a/test/CMakeLists.txt >> +++ b/test/CMakeLists.txt > > >> +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 , >> + # 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 > > >> -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. > > >> 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 > > >> +# 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} >> ) > > >> 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 > > >> -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 > > >> +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() > > >> 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 > > >> @@ -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) > > >> @@ -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} >> -) >> - > > >> 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 @@ > > >> -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 >> # 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 >> +# 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 >> --------------DYQZsOd3yPDqaISEkvPhz0Sg Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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
--------------DYQZsOd3yPDqaISEkvPhz0Sg--