Re: [Tarantool-patches] [PATCH luajit 6/6] cmake: replace prove with CTest

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the fixes!
Please consider some minor comments and nits below.

On 26.03.24, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> The patch replaces the main test runner `prove(1)` with CTest,

Since we are dropping the usage of `prove`, we should reformulate
related comments about it, shouldn't we? Also, maybe the
LUA_TEST_ENV_MORE can be removed?

> see [1] and [2].
> 
> Build and test steps looks the same as before:

Typo: s/looks/look/

> 
> $ cmake -S . -B build
> $ cmake --build build --parallel
> $ cmake --build build --target [LuaJIT-test, test]
> 
> CMake targets lua-Harness-tests, LuaJIT-tests,
> PUC-Rio-Lua-5.1-tests, tarantool-tests and tarantool-c-tests
> are still operating.
> 
> CMake custom target `test` was replaced by a target with the same

Typo: s/CMake/The CMake/

> name that is automatically generated by CMake. It is not possible
> to attach targets to this automatically generated `test` target.
> It means that target `test` is now executing `LuaJIT-test`,
> but not `LuaJIT-lint`. To mitigate this a new target

Typo: s/To mitigate this/To mitigate this,/

> `LuaJIT-check-all` is introduced, it has the same behaviour like

Typo: s/introduced, it/introduced. It/
Typo: s/like/as/

> an old `test` target: it runs all functional tests and linters.
> 
> One could use `ctest` tool:

Typo: s/use `ctest` tool/use the `ctest` tool/

> 
> $ ctest --print-labels
> Test project <snipped>
> All Labels:
>   LuaJIT-tests
>   PUC-Rio-Lua-5.1-tests
>   lua-Harness-tests
>   tarantool-c-tests
>   tarantool-tests
> $ ctest                          # Run all tests.
> $ ctest -L tarantool-tests       # Run tests by label.
> $ ctest -L tarantool-c-tests     # Ditto.
> $ ctest -L lua-Harness-tests     # Ditto.
> $ ctest -L LuaJIT-tests          # Ditto.
> $ ctest -L PUC-Rio-Lua-5.1-tests # Ditto.
> $ ctest --rerun-fail             # Run only the tests
>                                  # that failed previously.
> $ ctest --verbose                # Print details to stdout.
> $ ctest --output-on-failure      # Print details to stdout
>                                  # on test failure only.
> 
> The benefits are:
> 
> - less external dependencies, `prove(1)` is gone, `ctest`
>   is a part of CMake
> - running tests by test title
> - extended test running options in comparison to prove(1)
> - unified test output (finally!)
> 
> Note that the testsuites in `test/LuaJIT-tests` and
> `test/PUC-Rio-Lua-5.1` directories have their own test runners
> written in Lua, and currently CTest runs these testsuites
> as single tests. In the future, we could change these test runners
> to specify tests from outside, and after this, we could run tests
> separately by CTest in these test suites.
> 
> Note that it is not possible to add dependencies to `add_test()`
> in CMake, see [3]. CMake 3.7 introduces FIXTURES_REQUIRED [4] and
> FIXTURES_SETUP [5], but these test properties cannot be used -
> used CMake version is lower. This is workarounded by an introduced

Typo: s/used CMake version/the currently supported CMake version/
Typo: s/an introduced/the introduced/

> macro `add_test_suite_target` that adds a CMake-test for each
> testsuite that executes a target that builds test dependencies.
> 
> The commit introduce a CMake option LUAJIT_TEST_DEPS that set

Typo: s/introduce/introduces/
Typo: s/a CMake option/the CMake option/
Typo: s/set/sets/

> dependencies to be used for running and building LuaJIT tests.
> 
> 1. https://cmake.org/cmake/help/latest/manual/ctest.1.html
> 2. https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html
> 3. https://gitlab.kitware.com/cmake/cmake/-/issues/8774
> 4. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html
> 5. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html
> ---
>  .gitignore                                |   2 +
>  CMakeLists.txt                            |  14 +++
>  test/CMakeLists.txt                       | 107 +++++++++++++++++-----
>  test/LuaJIT-tests/CMakeLists.txt          |  33 ++++---
>  test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt |  25 +++--
>  test/lua-Harness-tests/CMakeLists.txt     |  50 +++++-----
>  test/tarantool-c-tests/CMakeLists.txt     |  58 ++++++------
>  test/tarantool-tests/CMakeLists.txt       |  68 +++++++-------
>  8 files changed, 226 insertions(+), 131 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index dc5ea5fc..dd1f8888 100644
> --- a/.gitignore
> +++ b/.gitignore

<snipped>

> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 7f5e2afb..96f85285 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -345,6 +345,13 @@ set(LUAJIT_TEST_BINARY ${LUAJIT_BINARY} CACHE STRING
>    "Lua implementation to be used for tests. Default is 'luajit'."
>  )
>  
> +# If LuaJIT is used in a parent project, provide an option
> +# to choose what dependencies to be used for running and building

Typo: s/to be used/to use/ or s/to be used/are used/

> +# LuaJIT tests.

<snipped>

> @@ -362,6 +369,13 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR

<snipped>

> +if (LUAJIT_USE_TEST)

Typo: s/if (/if(/


<snipped>

> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 62478441..06a1d365 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt

<snipped>

> +set(CTEST_FLAGS
> +  --output-on-failure
> +  --schedule-random
> +  # Timeout in seconds. Most of LuaJIT tests are fast,

Typo: s/Most of LuaJIT tests are fast, however,/Most LuaJIT tests are fast. However,/

> +  # however, some tests can hang or execute infinite loop.

Typo: s/infinite loop/an infinite loop/

> +  # See <tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c>,
> +  # commit caa99865c206 ("test: rewrite sysprof test using managed execution").
> +  # Moreover, tests for instrumented LuaJIT requires more time.

Typo: s/requires/require/

> +  # 30 minutes is a sane timeout.
> +  --timeout 1800
> +  --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
> +)
> +if(CMAKE_VERBOSE_MAKEFILE)
> +  list(APPEND CTEST_FLAGS --verbose)
> +endif()
> +
> +# It is not possible to add dependencies to `add_test()`
> +# in CMake, see [1]. CMake 3.7 introduces FIXTURES_REQUIRED [2]
> +# and FIXTURES_SETUP [3], but these test properties cannot be
> +# used - it is unsupported in a current CMake version.

Typo: s/it is unsupported/this feature is unsupported/
Typo: s/a current/the current/

> +# To workaround this a function `add_test_suite_target` is

Typo: s/To workaround this a/To workaround this, the/
Typo: s/is introduced, it/is introduced. It/

> +# introduced, it adds a CMake target that builds testsuite's
> +# prerequisites and CMake test that executes that target.

Typo: s/CMake test/a CMake test/

> +#
> +# 1. https://gitlab.kitware.com/cmake/cmake/-/issues/8774
> +# 2. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html
> +# 3. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html

<snipped>

> -add_custom_target(${PROJECT_NAME}-test DEPENDS
> -  LuaJIT-tests
> -  PUC-Rio-Lua-5.1-tests
> -  lua-Harness-tests
> -  tarantool-c-tests
> -  tarantool-tests
> +# Each testsuite has it's own CMake target, but combining these

Typo: s/it's/its/

> +# target to a single one is not desired, because each target

Typo: s/these target to/these targets into/
Typo: s/desired,/desired/

> +# runs it's own ctest command, that each time enumerates tests

Typo: s/command, that/command, which/
Typo? s/ctest/CTest/

> +# from zero and prints test summary at the end of test run.

Typo: s/prints test summary/prints the test summary/
Typo: s/test run/ the test run/

> +# For common target this output looks inconvenient.

Typo: s/For common target/For a common target,/

> +# Therefore target below executes a single instance of ctest

Typo: s/Therefore target/Therefore, the target/
Typo: s/instance of ctest/instance of the CTest/

> +# command that runs all generated CMake tests.

<snipped>

> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index 003f8de6..6ad05aa6 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt

<snipped>

> +# The test suite has its own test runner
> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
> +# tests one-by-one by CTest.
> +add_test(NAME "test/LuaJIT-tests"

Maybe it is better to use
| add_test(NAME "test/${TEST_SUITE_NAME}"
for consistency for all suites?
Also, it avoids confusing (for me, this is some kind of typo opposed to
the `add_test_suite_target()` command above).
Feel free to ignore, since it is subjectivity.

> +  COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
> +                                 +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
>    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
>  )

<snipped>

> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> index 98277f9a..a26e38d4 100644
> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt

<snipped>

> -add_custom_command(TARGET PUC-Rio-Lua-5.1-tests
> -  COMMENT "Running PUC-Rio Lua 5.1 tests"
> -  COMMAND
> -  env
> -    LUA_PATH="${LUA_PATH}"
> -    ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua
> +# The test suite has its own test runner
> +# (test/PUC-Rio-Lua-5.1-tests/all.lua), it is not possible
> +# to run these tests one-by-one by CTest.
> +add_test(NAME "test/PUC-Rio-Lua-5.1-tests"
> +  COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua
>    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>  )
> +set_tests_properties("test/PUC-Rio-Lua-5.1-tests" PROPERTIES
> +  ENVIRONMENT "LUA_PATH=${LUA_PATH}"

Nit: Do we need to move the " back to the start of the declaration?
If it is possible, please use LUA_PATH="${LUA_PATH}" instead.

> +  LABELS ${TEST_SUITE_NAME}
> +  DEPENDS PUC-Rio-Lua-5.1-tests-deps
> +)
>  
>  # vim: expandtab tabstop=2 shiftwidth=2
> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
> index f748a8fd..6f7b5697 100644
> --- a/test/lua-Harness-tests/CMakeLists.txt
> +++ b/test/lua-Harness-tests/CMakeLists.txt

<snipped>

> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME)
> +  # FIXME: By default, GLOB lists directories.
> +  # Directories are omitted in the result if LIST_DIRECTORIES
> +  # is set to false. New in version CMake 3.3.
> +  if (${test_name} STREQUAL ${TEST_SUITE_NAME})

Typo: s/if (/if(/

> +    continue()
> +  endif()

<snipped>

> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index 4b1d8832..5affbb9e 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt

<snipped>

> @@ -30,6 +19,23 @@ AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
>  # test binary can be run from any directory.
>  AppendFlags(TESTS_C_FLAGS "-D__LJ_TEST_DIR__='\"${CMAKE_CURRENT_SOURCE_DIR}\"'")
>  
> +set(TEST_SUITE_NAME "tarantool-c-tests")
> +
> +# Proxy CMake target with all targets that builds C tests.

Typo: s/Proxy/The proxy/
Typo: s/that builds/that build/

> +# This is needed because targets for each C test are generated
> +# at the same time with CMake tests, and all prerequisites must be

Typo: s/at the same time with/at the same time as/
Typo: s/must be already exist/must already exist/

> +# already exist at this moment.
> +add_custom_target(tarantool-c-tests-build)

<snipped>

> @@ -47,22 +53,16 @@ foreach(test_source ${tests})
>    )
>    target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
>    LIST(APPEND TESTS_COMPILED ${exe})

Should this line be removed since TESTS_COMPILED is no longer in use?

> -endforeach()
> +  add_dependencies(tarantool-c-tests-build ${exe})
>  
> -add_custom_target(tarantool-c-tests
> -  DEPENDS libluajit libtest ${TESTS_COMPILED}
> -)
> -

<snipped>

> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 35bcc5ef..9086a890 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -4,19 +4,13 @@

<snipped>

> -add_custom_target(tarantool-tests
> +add_custom_target(tarantool-tests-libs
>    DEPENDS ${LUAJIT_TEST_BINARY}

Should it depend on libluajit instead?
We don't need the LuaJIT binary to link with the LuaJIT library.

>  )
>  
>  macro(BuildTestCLib lib sources)
>    AddTestLib(${lib} ${sources})
> -  add_dependencies(tarantool-tests ${lib})
> +  add_dependencies(tarantool-tests-libs ${lib})
>    # Add the directory where the lib is built to the list with
>    # entries for LUA_CPATH environment variable, so LuaJIT can find
>    # and load it. See the comment about extending the list in the
> @@ -61,20 +55,12 @@ make_lua_path(LUA_PATH
>      ${LUAJIT_SOURCE_DIR}/?.lua
>      ${LUAJIT_BINARY_DIR}/?.lua
>  )
> +
>  # Update LUA_CPATH with the library paths collected within
>  # <BuildTestLib> macro.
>  make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS})
>  
>  set(LUA_TEST_SUFFIX .test.lua)
> -set(LUA_TEST_FLAGS --failures --shuffle)
> -set(LUA_TEST_ENV
> -  "LUA_PATH=\"${LUA_PATH}\""
> -  "LUA_CPATH=\"${LUA_CPATH}\""
> -)
> -
> -if(CMAKE_VERBOSE_MAKEFILE)
> -  list(APPEND LUA_TEST_FLAGS --verbose)
> -endif()
>  
>  # XXX: Since the auxiliary libraries are built as a dynamically
>  # loaded modules on MacOS instead of shared libraries as it is
> @@ -118,29 +104,37 @@ endif()
>  # process.
>  # See also: https://github.com/tarantool/tarantool/issues/9656.
>  if(LUAJIT_USE_ASAN AND CMAKE_C_COMPILER_ID STREQUAL "GNU")
> -  # FIXME: After using CTest instead of `prove` we should set this
> -  # environment variable only for the corresponding tests to avoid
> -  # warnings from the libc and etc.
> +  # FIXME: test_title test/tarantool-tests/lj-802-panic-at-mcode-protfail.test.lua

Typo: No dot at the end of the sentence.
Nit: Comment line width is more than 66 symbols.

Also, I didn't get the idea of the first sentence.
Also, will we add an additional patch that fixes this comment in this
patch series (as an additional commit) or will we do it later?

> +  # With CTest we should set this environment variable only

Typo: s/With CTest/With CTest,/

> +  # for the corresponding tests to avoid warnings from
> +  # the GNU libc and other libc implementations.

Should this change be the part of the commit "test: fix
lj-802-panic-at-mcode-protfail GCC+ASan"?

>    LibRealPath(LIB_ASAN libasan.so)
>    list(APPEND LUA_TEST_ENV_MORE LD_PRELOAD=${LIB_ASAN})
>  endif()
>  
> -# LUA_CPATH and LD_LIBRARY_PATH variables and also dependencies
> -# list with libraries are set in scope of BuildTestLib macro.
> -add_custom_command(TARGET tarantool-tests
> -  COMMENT "Running Tarantool tests"
> -  COMMAND
> -  # XXX: We can't move everything to the "inner" env, since there
> -  # are some issues with escaping ';' for different shells. As
> -  # a result LUA_PATH/LUA_CPATH variables are set via the "outer"
> -  # env, since they are not stripped by SIP like LD_*/DYLD_* are.
> -  env
> -    ${LUA_TEST_ENV}
> -    ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
> -      --exec 'env ${LUA_TEST_ENV_MORE} ${LUAJIT_TEST_COMMAND}'
> -      --ext ${LUA_TEST_SUFFIX}
> -      --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> -      ${LUA_TEST_FLAGS}
> -  WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +set(TEST_SUITE_NAME "tarantool-tests")
> +
> +# XXX: The call produces both test and target
> +# <tarantool-tests-deps> as a side effect.
> +add_test_suite_target(tarantool-tests
> +  LABELS ${TEST_SUITE_NAME}
> +  DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs
>  )
>  
> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> +# with dependencies are set in scope of BuildTestLib macro.

I prefer to use the old paraphrasing of this paragraph.
Otherwise, the reader may think: "What the heck is TESTLIBS?"

> +set(TEST_ENV "LUA_PATH=${LUA_PATH};LUA_CPATH=${LUA_CPATH};${LUA_TEST_ENV_MORE}")

Maybe it is better to use `list(APPEND` here for better readability?

Or even don't use the additional variable that is used once and provide
these ENVIROMENT variables directly below.

> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> +foreach(test_path ${tests})
> +  get_filename_component(test_name ${test_path} NAME)
> +  set(test_title "test/tarantool-tests/${test_name}")

Should it be "test/${TEST_SUITE_NAME}/${test_name}" by analogy with
other test suites?

> +  add_test(NAME ${test_title}
> +    COMMAND ${LUAJIT_TEST_COMMAND} ${test_path}
> +    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +  )
> +  set_tests_properties(${test_title} PROPERTIES
> +    ENVIRONMENT "${TEST_ENV}"
> +    LABELS ${TEST_SUITE_NAME}
> +    DEPENDS tarantool-tests-deps
> +  )
> +endforeach()
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun