<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Sergey,</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 3/26/24 14:03, Sergey Kaplun wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the fixes!
Please consider some minor comments and nits below.
On 26.03.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Sergey Bronnikov <a
class="moz-txt-link-rfc2396E"
href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>
The patch replaces the main test runner `prove(1)` with CTest,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Since we are dropping the usage of `prove`, we should reformulate
related comments about it, shouldn't we? </pre>
</blockquote>
<p>Updated:</p>
<p>--- a/test/tarantool-tests/CMakeLists.txt<br>
+++ b/test/tarantool-tests/CMakeLists.txt<br>
@@ -79,15 +79,15 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")<br>
# since some programs sanitize the environment before they
start<br>
# child processes. Specifically, environment variables starting<br>
# with DYLD_ and LD_ are unset for child process started by<br>
- # other programs (like /usr/bin/prove --exec using for
launching<br>
- # test suite). For more info, see the docs[2] below.<br>
+ # other programs (like `ctest` using for launching tests).<br>
+ # For more info, see the docs[2] below.<br>
#<br>
# These environment variables are used by FFI machinery to find<br>
# the proper shared library, hence we can still tweak testing<br>
# environment before calling <ffi.load>. However, the
value<br>
# can't be passed via the standard environment variable, so we<br>
- # use env call in prove's --exec flag value to get around SIP<br>
- # magic tricks.<br>
+ # use ENVIRONMENT property in `set_tests_properties` to get<br>
+ # around SIP magic tricks.<br>
#<br>
# [1]: <a class="moz-txt-link-freetext"
href="https://support.apple.com/en-us/HT204899">https://support.apple.com/en-us/HT204899</a><br>
# [2]: <a class="moz-txt-link-freetext"
href="https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html">https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html</a><br>
<br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<pre class="moz-quote-pre" wrap="">Also, maybe the
LUA_TEST_ENV_MORE can be removed?</pre>
</blockquote>
I'm not sure. I suspect the same trick is required with CTest on
macOS too.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">see [1] and [2].
Build and test steps looks the same as before:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/looks/look/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">$ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/CMake/The CMake/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/To mitigate this/To mitigate this,/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">`LuaJIT-check-all` is introduced, it has the same behaviour like
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/introduced, it/introduced. It/
Typo: s/like/as/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">an old `test` target: it runs all functional tests and linters.
One could use `ctest` tool:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/use `ctest` tool/use the `ctest` tool/
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">$ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/used CMake version/the currently supported CMake version/
Typo: s/an introduced/the introduced/
</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/introduce/introduces/
Typo: s/a CMake option/the CMake option/
Typo: s/set/sets/
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">dependencies to be used for running and building LuaJIT tests.
1. <a class="moz-txt-link-freetext"
href="https://cmake.org/cmake/help/latest/manual/ctest.1.html">https://cmake.org/cmake/help/latest/manual/ctest.1.html</a>
2. <a class="moz-txt-link-freetext"
href="https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html">https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html</a>
3. <a class="moz-txt-link-freetext"
href="https://gitlab.kitware.com/cmake/cmake/-/issues/8774">https://gitlab.kitware.com/cmake/cmake/-/issues/8774</a>
4. <a class="moz-txt-link-freetext"
href="https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html">https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html</a>
5. <a class="moz-txt-link-freetext"
href="https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html">https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html</a>
---
.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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/to be used/to use/ or s/to be used/are used/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# LuaJIT tests.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+if (LUAJIT_USE_TEST)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/if (/if(/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 62478441..06a1d365 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+set(CTEST_FLAGS
+ --output-on-failure
+ --schedule-random
+ # Timeout in seconds. Most of LuaJIT tests are fast,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/Most of LuaJIT tests are fast, however,/Most LuaJIT tests are fast. However,/
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # however, some tests can hang or execute infinite loop.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/infinite loop/an infinite loop/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # 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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/requires/require/
Fixed.</pre>
</blockquote>
<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # 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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/it is unsupported/this feature is unsupported/
Typo: s/a current/the current/</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.<br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# To workaround this a function `add_test_suite_target` is
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/To workaround this a/To workaround this, the/
Typo: s/is introduced, it/is introduced. It/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# introduced, it adds a CMake target that builds testsuite's
+# prerequisites and CMake test that executes that target.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/CMake test/a CMake test/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+#
+# 1. <a class="moz-txt-link-freetext"
href="https://gitlab.kitware.com/cmake/cmake/-/issues/8774">https://gitlab.kitware.com/cmake/cmake/-/issues/8774</a>
+# 2. <a class="moz-txt-link-freetext"
href="https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html">https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html</a>
+# 3. <a class="moz-txt-link-freetext"
href="https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html">https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">-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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/it's/its/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# target to a single one is not desired, because each target
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/these target to/these targets into/
Typo: s/desired,/desired/
Fixed.</pre>
</blockquote>
<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# runs it's own ctest command, that each time enumerates tests
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/command, that/command, which/
Typo? s/ctest/CTest/</pre>
</blockquote>
replaced with `ctest`<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# from zero and prints test summary at the end of test run.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/prints test summary/prints the test summary/
Typo: s/test run/ the test run/</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.<br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# For common target this output looks inconvenient.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/For common target/For a common target,/
Fixed.</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# Therefore target below executes a single instance of ctest
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/Therefore target/Therefore, the target/
Typo: s/instance of ctest/instance of the CTest/
Fixed.</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# command that runs all generated CMake tests.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# 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"
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.</pre>
</blockquote>
<p>Updated:</p>
<p>diff --git a/test/LuaJIT-tests/CMakeLists.txt
b/test/LuaJIT-tests/CMakeLists.txt<br>
index 6ad05aa6..b8e4dfc4 100644<br>
--- a/test/LuaJIT-tests/CMakeLists.txt<br>
+++ b/test/LuaJIT-tests/CMakeLists.txt<br>
@@ -64,12 +64,13 @@ add_test_suite_target(LuaJIT-tests<br>
# The test suite has its own test runner<br>
# (test/LuaJIT-tests/test.lua), it is not possible to run these<br>
# tests one-by-one by CTest.<br>
-add_test(NAME "test/LuaJIT-tests"<br>
+set(test_title "test/${TEST_SUITE_NAME}")<br>
+add_test(NAME "${test_title}"<br>
COMMAND ${LUAJIT_TEST_COMMAND}
${CMAKE_CURRENT_SOURCE_DIR}/test.lua<br>
+slow +ffi +bit +jit
${LUAJIT_TEST_TAGS_EXTRA}<br>
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}<br>
)<br>
-set_tests_properties("test/LuaJIT-tests" PROPERTIES<br>
+set_tests_properties("${test_title}" PROPERTIES<br>
LABELS ${TEST_SUITE_NAME}<br>
ENVIRONMENT "${LUAJIT_TESTS_ENV}"<br>
DEPENDS LuaJIT-tests-deps<br>
diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt<br>
index a26e38d4..0942c62d 100644<br>
--- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt<br>
+++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt<br>
@@ -46,11 +46,12 @@ add_test_suite_target(PUC-Rio-Lua-5.1-tests<br>
# The test suite has its own test runner<br>
# (test/PUC-Rio-Lua-5.1-tests/all.lua), it is not possible<br>
# to run these tests one-by-one by CTest.<br>
-add_test(NAME "test/PUC-Rio-Lua-5.1-tests"<br>
+set(test_title "test/${TEST_SUITE_NAME}")<br>
+add_test(NAME "${test_title}"<br>
COMMAND ${LUAJIT_TEST_COMMAND}
${CMAKE_CURRENT_SOURCE_DIR}/all.lua<br>
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}<br>
)<br>
-set_tests_properties("test/PUC-Rio-Lua-5.1-tests" PROPERTIES<br>
+set_tests_properties("${test_title}" PROPERTIES<br>
ENVIRONMENT "LUA_PATH=${LUA_PATH}"<br>
LABELS ${TEST_SUITE_NAME}<br>
DEPENDS PUC-Rio-Lua-5.1-tests-deps<br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
+ +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">-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}"
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.</pre>
</blockquote>
<p><br>
</p>
<p>it breaks suite:</p>
<p><br>
</p>
<p> Start 2:
test/PUC-Rio-Lua-5.1-tests
<br>
2/2 Test #2: test/PUC-Rio-Lua-5.1-tests .......***Failed 0.02
sec <br>
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc32/src/luajit:
module '/tmp/lua_joUdrU' not found:<br>
no field package.preload['/tmp/lua_joUdrU']
<br>
no file '"/tmp/lua_joUdrU' <br>
no file
'./build/gc64/test/tarantool-tests/lj-1087-vm-handler-call//tmp/lua_joUdrU.so'<br>
stack traceback:
<br>
[C]: at
0x63fdd720d320
<br>
[C]: at 0x63fdd7145e30
<br>
/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<br>
a:66: assertion failed!
<br>
stack traceback:
<br>
[C]: in function
'assert'
<br>
...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lua:66: in
function 'RUN'<br>
...l/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/main.lua:77: in
function '_dofile' <br>
...ol/third_party/luajit/test/PUC-Rio-Lua-5.1-tests/all.lua:74: in
main chunk <br>
[C]: at 0x5f99fec32e30 <br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+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})
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/if (/if(/
</pre>
</blockquote>
<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<pre class="moz-quote-pre" wrap="">Fixed.</pre>
</blockquote>
<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ continue()
+ endif()
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/Proxy/The proxy/
Typo: s/that builds/that build/
</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# This is needed because targets for each C test are generated
+# at the same time with CMake tests, and all prerequisites must be
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/at the same time with/at the same time as/
Typo: s/must be already exist/must already exist/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+# already exist at this moment.
+add_custom_target(tarantool-c-tests-build)
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">@@ -47,22 +53,16 @@ foreach(test_source ${tests})
)
target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
LIST(APPEND TESTS_COMPILED ${exe})
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Should this line be removed since TESTS_COMPILED is no longer in use?</pre>
</blockquote>
<p>Sure, thanks!</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">-endforeach()
+ add_dependencies(tarantool-c-tests-build ${exe})
-add_custom_target(tarantool-c-tests
- DEPENDS libluajit libtest ${TESTS_COMPILED}
-)
-
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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 @@
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""><snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">-add_custom_target(tarantool-tests
+add_custom_target(tarantool-tests-libs
DEPENDS ${LUAJIT_TEST_BINARY}
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Should it depend on libluajit instead?
We don't need the LuaJIT binary to link with the LuaJIT library.
</pre>
</blockquote>
<br>
<p>Fixed.</p>
<p><br>
</p>
<p>--- a/test/tarantool-tests/CMakeLists.txt<br>
+++ b/test/tarantool-tests/CMakeLists.txt<br>
@@ -5,7 +5,7 @@<br>
cmake_minimum_required(VERSION 3.1 FATAL_ERROR)<br>
<br>
add_custom_target(tarantool-tests-libs<br>
- DEPENDS ${LUAJIT_TEST_BINARY}<br>
+ DEPENDS libluajit<br>
)<br>
<br>
macro(BuildTestCLib lib sources)<br>
<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> )
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: <a class="moz-txt-link-freetext"
href="https://github.com/tarantool/tarantool/issues/9656">https://github.com/tarantool/tarantool/issues/9656</a>.
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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: No dot at the end of the sentence.
Nit: Comment line width is more than 66 symbols.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<pre class="moz-quote-pre" wrap="">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?<span
style="white-space: normal"></span></pre>
</blockquote>
<p><br>
</p>
<p>I propose to address this later. We have an issue to set env
variables specific for test,</p>
<p>lets fix this in scope of that issue too.</p>
<p><br>
</p>
<p>--- a/test/tarantool-tests/CMakeLists.txt<br>
+++ b/test/tarantool-tests/CMakeLists.txt<br>
@@ -118,9 +118,9 @@ endif()<br>
# process.<br>
# See also: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/9656">https://github.com/tarantool/tarantool/issues/9656</a>.<br>
if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU")<br>
- # FIXME: After using CTest instead of `prove` we should set
this<br>
- # environment variable only for the corresponding tests to
avoid<br>
- # warnings from the libc and etc.<br>
+ # FIXME: We should set this environment variable only<br>
+ # for the corresponding tests to avoid warnings from<br>
+ # the GNU libc and other libc implementations.<br>
LibRealPath(LIB_ASAN libasan.so)<br>
list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN})<br>
endif()<br>
<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # With CTest we should set this environment variable only
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Typo: s/With CTest/With CTest,/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # for the corresponding tests to avoid warnings from
+ # the GNU libc and other libc implementations.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Should this change be the part of the commit "test: fix
lj-802-panic-at-mcode-protfail GCC+ASan"?</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.<br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">I prefer to use the old paraphrasing of this paragraph.
Otherwise, the reader may think: "What the heck is TESTLIBS?"</pre>
</blockquote>
<p><br>
</p>
<p>--- a/test/tarantool-tests/CMakeLists.txt<br>
+++ b/test/tarantool-tests/CMakeLists.txt<br>
@@ -114,9 +114,9 @@ foreach(test_path ${tests})<br>
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}<br>
)<br>
set_tests_properties(${test_title} PROPERTIES<br>
- # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS<br>
- # list with dependencies are set in scope of BuildTestLib<br>
- # macro.<br>
+ # LUA_CPATH and LD_LIBRARY_PATH variables and also<br>
+ # dependencies list with libraries are set in scope of<br>
+ # BuildTestLib macro.<br>
ENVIRONMENT
"LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"<br>
LABELS ${TEST_SUITE_NAME}<br>
DEPENDS tarantool-tests-deps<br>
<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+set(TEST_ENV "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Maybe it is better to use `list(APPEND` here for better readability?</pre>
</blockquote>
It was fine before patch for ctest support. Why we need to change
this?<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<pre class="moz-quote-pre" wrap="">Or even don't use the additional variable that is used once and provide
these ENVIROMENT variables directly below.</pre>
</blockquote>
<p>Done. (but for my taste previously was better and</p>
<p>actually it is not related to ctest patch at all)<br>
</p>
<p><br>
</p>
<p>--- a/test/tarantool-tests/CMakeLists.txt<br>
+++ b/test/tarantool-tests/CMakeLists.txt<br>
@@ -121,9 +121,6 @@ add_test_suite_target(tarantool-tests<br>
DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs<br>
)<br>
<br>
-# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list<br>
-# with dependencies are set in scope of BuildTestLib macro.<br>
-set(TEST_ENV
"LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}")<br>
file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR}
"*${LUA_TEST_SUFFIX}")<br>
foreach(test_path ${tests})<br>
get_filename_component(test_name ${test_path} NAME)<br>
@@ -133,7 +130,10 @@ foreach(test_path ${tests})<br>
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}<br>
)<br>
set_tests_properties(${test_title} PROPERTIES<br>
- ENVIRONMENT "${TEST_ENV}"<br>
+ # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS<br>
+ # list with dependencies are set in scope of BuildTestLib<br>
+ # macro.<br>
+ ENVIRONMENT
"LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}"<br>
LABELS ${TEST_SUITE_NAME}<br>
DEPENDS tarantool-tests-deps<br>
)<br>
<br>
</p>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+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}")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Should it be "test/${TEST_SUITE_NAME}/${test_name}" by analogy with
other test suites?</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZgKrgeqlzYLt0tZE@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ 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
</pre>
</blockquote>
</blockquote>
</body>
</html>