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

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

Hi, Sergey!
Thanks for the fixes!
Please consider my concerns below.
I suggest moving our conversation forward with the new patch version.

On 20.03.24, Sergey Bronnikov wrote:
> Sergey,
> 
> thanks for review!
> 
> see my comments below
> 
> On 3/18/24 18:54, Sergey Kaplun wrote:

<snipped>

> >> 1. https://cmake.org/cmake/help/latest/manual/ctest.1.html
> >> 2. https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html
> >> 3. https://gitlab.kitware.com/cmake/cmake/-/issues/8774
> >> 4. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html
> >> 5. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html
> >> ---
> >> Changes v3:
> >> - rebased to master
> >> - applied fixes suggested by Igor Munkin
> >>
> >> PR: https://github.com/tarantool/tarantool/pull/9286
> >> Branch: https://github.com/tarantool/luajit/tree/ligurio/enable_test_target
> >>
> >>   .gitignore                                |  2 +
> >>   CMakeLists.txt                            | 11 ++++
> >>   test/CMakeLists.txt                       | 78 +++++++++++++++--------
> >>   test/LuaJIT-tests/CMakeLists.txt          | 40 +++++++-----
> >>   test/LuaJIT-tests/src/CMakeLists.txt      |  2 +-
> >>   test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++++---
> >>   test/lua-Harness-tests/CMakeLists.txt     | 48 ++++++++------
> >>   test/tarantool-c-tests/CMakeLists.txt     | 56 ++++++++--------
> >>   test/tarantool-tests/CMakeLists.txt       | 62 ++++++++----------
> >>   9 files changed, 194 insertions(+), 130 deletions(-)
> >>
> >> diff --git a/.gitignore b/.gitignore
> >> index dc5ea5fc..dd1f8888 100644
> >> --- a/.gitignore
> >> +++ b/.gitignore
> > <snipped>
> >
> >> diff --git a/CMakeLists.txt b/CMakeLists.txt
> >> index 7f5e2afb..6b2f855f 100644
> >> --- a/CMakeLists.txt
> >> +++ b/CMakeLists.txt
> >> @@ -345,6 +345,13 @@ set(LUAJIT_TEST_BINARY ${LUAJIT_BINARY} CACHE STRING
> >>     "Lua implementation to be used for tests. Default is 'luajit'."
> >>   )
> >>   
> >> +# If LuaJIT is used in a parent project, provide an option
> >> +# to choose what dependencies to be used for running and building
> >> +# LuaJIT tests.
> >> +set(LUAJIT_TEST_DEPS "luajit-main" CACHE STRING
> >> +  "A list of target dependencies to be used for tests."
> >> +)
> > Why do we use luajit-main as a target dependency for tests?
> > Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not
> > in tarantool-c-tests)?

See my comment below.

> >
> >> +
> >>   # FIXME: If LuaJIT is used in parent project, provide an option
> >>   # to pass Lua code to be run before tests are started.
> >>   # XXX: Attentive reader might point to LUA_INIT environment
> >> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
> >>     "Lua code need to be run before tests are started."
> >>   )
> >>   
> >> +# XXX: Enable testing via CTest. Since CTest expects to find a
> >> +# test file in the build directory root, this command should be in
> > Nit: I suggest to mentioning that this command generates "test" target.
> > Something like the following:
> > | # XXX: Enable testing via CTest. This generates `test` target.
> 
> Added:
> 
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT 
> "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR
>     "Lua code need to be run before tests are started."
>   )
> 
> -# XXX: Enable testing via CTest. Since CTest expects to find a
> -# test file in the build directory root, this command should be in
> -# the source directory root (hence, in this CMakeLists.txt).
> +# XXX: Enable testing via CTest. This automatically generates
> +# `test` target. Since CTest expects to find a test file in

Typo: s/`test`/the `test`/

> +# the build directory root, this command should be in the source
> +# directory root (hence, in this CMakeLists.txt).
>   enable_testing()
>   add_subdirectory(test)
> 
> 
> >

<snipped>

> > Unfortunately, this flag does nothing for `make test` command.
> > Is there the way to fix it?
> 
> Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that 
> contains
> CTest options and is used by `test` target. As a comment says.
> 
> BTW you can fix it by using CMake workflows and specify any CTest 
> arguments you want in CMakePreset.json.

I got it thanks for the clarification!
May you please add a comment with the "FIXME:" mark and mention the
CMake version and the CMAKE_CTEST_ARGUMENTS variable?

> 
> > BTW `make LuaJIT-tests` or `make tarantool-tests` work fine.
> Sure, these are our own targets and we pass desired CTest options to it.
> >> +  --output-on-failure
> >> +  --schedule-random
> >> +  --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}
> > I suppose it will be good to add the `--timeout` option here too.
> > 5 minutes as a timeout for the single test sounds more than enough.
> >
> Added.

Unfortunately, as you mentioned offline, for some builds [1], even 5
minutes is not enough:
| 211/211 Test #109: test/tarantool-tests/lj-1034-tabov-error-frame.test.lua ........................***Timeout 300.54 sec
I suggest increasing the timeout to 30 minutes.

> >> +)
> >> +if(CMAKE_VERBOSE_MAKEFILE)
> >> +  list(APPEND TEST_FLAGS --verbose)
> >> +endif()
> >> +
> >> +function(add_test_suite_target target)
> >> +  set(prefix ARG)
> >> +  set(noValues)
> >> +  set(singleValues LABEL)
> >> +  set(multiValues DEPENDS)
> >> +
> >> +  # FIXME: if we update to CMake >= 3.5, can remove this line.
> >> +  include(CMakeParseArguments)
> >> +  cmake_parse_arguments(${prefix}
> >> +                        "${noValues}"
> >> +                        "${singleValues}"
> >> +                        "${multiValues}"
> >> +                        ${ARGN})
> >> +
> >> +  set(_DEPS_TARGET ${target}-deps)
> >> +
> >> +  add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS})
> >> +
> >> +  add_test(${_DEPS_TARGET}
> > Should the NAME parameter be ${target} instead?
> 
> Added.

Sorry, I've misguided you with my comment. I thought that we should name
the test as "${target}", not "${_DEPS_TARGET}" (I hadn't gotten the idea
of proxy targets before, but I'm OK with it now).
Please revert this change.
My bad.

Also, please add the comment that this proxy test allows us to be sure that
all prerequisites must be built or generated before the main test
execution.

> 
> 
> > Or, maybe this line
> > should be ommitted since it is also used for normal test targets with
> > several tests.
> 
> CMake test for a target with dependencies must be generated,
> 
> otherwise dependencies will not be built.
> 
> >
> > For now `ctest -N` output looks a little bit inconsistent:
> >
> > | Test project /home/burii/reviews/luajit/ctest
> > |   Test   #1: LuaJIT-tests-deps
> > |   Test   #2: test/LuaJIT-tests
> > |   Test   #3: PUC-Rio-Lua-5.1-tests-deps
> > |   Test   #4: test/PUC-Rio-Lua-5.1
> > |   Test   #5: lua-Harness-tests-deps
> > |   Test   #6: test/lua-Harness-tests/000-sanity.t
> > |   ...
> > |   Test  #56: tarantool-c-tests-deps
> > |   Test  #57: test/tarantool-c-tests/fix-yield-c-hook.c_test
> > |   ...
> > |   Test  #67: tarantool-tests-deps
> > |   Test  #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua
> > |   ...
> >
> > If tests like the following are needed to run tests, I suggest naming
> > them like `LuaJIT-tests-build` to avoid confusion.
> 
> AFAIR this naming was suggested by Igor.
> 
> What kind of confusion you observe? For me something like

That we are really testing something with this strange name.
OK, my bad ignore it.

> 
> `LuaJIT-tests-build`  much more confusing than "xxx-deps".
> 
> > |   Test   #1: LuaJIT-tests-deps

OK, let's leave it as it is.

> >

<snipped>

> >> -add_custom_target(${PROJECT_NAME}-test DEPENDS
> >> -  LuaJIT-tests
> >> -  PUC-Rio-Lua-5.1-tests
> >> -  lua-Harness-tests
> >> -  tarantool-c-tests
> >> -  tarantool-tests
> >> +add_custom_target(${PROJECT_NAME}-test
> >> +  COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS}
> > Why do we need to use custom command for this test?
> > IINM, all "subtargets" are used the same ${TEST_FLAGS}
> > See also Igor's comment about this test.
> 
> Answered to Igor.
> 
> because output will be ugly in that case,
> 
> just change as you suggest and run LuaJIT-test.

Please, mention the desired output format in the comment.

> 
> >

<snipped>

> >>   
> >>   set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:")
> >> @@ -50,10 +46,11 @@ if(LUAJIT_USE_ASAN)
> >>     if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
> >>       LibRealPath(LIB_ASAN libasan.so)
> >>       # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`.
> >> -    # ";" will expand to " " as we want.
> >> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}")
> >> +    # ";" will expand to " " as we want after wrapping
> > This is not true. After these changes the environment variable
> > `LD_PRELOAD` is the following:
> > | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
> > But it should be:
> > | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32"
> > This is why this quotes is used.
> > For some reason, with the debug flags, paths are separated via \n:
> > | 2:  LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0
> > | 2:  /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32
> >
> > So, I got the following error, when I build LuaJIT with GCC and use the
> > flag `-DLUAJIT_ENABLE_ASAN`:
> > | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264)
> > |   #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69
> >
> Fixed.
> 
> Unfortunately there is a yet another test that will never worked in 
> configuration GCC + ASAN.
> 
> Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to 
> fix it, but as
> 
> it was never worked before we don't need to do it in scope of ctest support.

It works in our CI, where we use the clang build.

For now, we may just disable this test for GCC + clang build with the
link to the issue.

On branch, you're using global LD_PRELOAD from the aforementioned test
(lj-802-panic-at-mcode-protfail.test.lua). As a result, tons of tests
fail with the following error:
| got:  PANIC: unprotected error in call to Lua API (runtime code generation failed, restricted kernel?)

May be it is better to proxy parent LD_PRELOAD to the test instead?

> diff --git a/test/LuaJIT-tests/CMakeLists.txt 
> b/test/LuaJIT-tests/CMakeLists.txt
> index 0c85f15c..4c9cf901 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)
>     # https://github.com/google/sanitizers/issues/934.
>     macro(LibRealPath output lib)
>       execute_process(
> -      COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
> +      COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}

Please mention in the comment that the C++ and C compilers use the same
tooling, so the returned library path is the same.

>         OUTPUT_VARIABLE LIB_LINK
>         OUTPUT_STRIP_TRAILING_WHITESPACE
> +      TIMEOUT 5

Why do we need timeout here? (*)

> +      RESULT_VARIABLE RES
>       )
> +    if (${LIB_LINK} STREQUAL ${lib})
> +      message(FATAL_ERROR "library ${lib} is not found")
> +    endif()
> +    if (NOT RES EQUAL 0)
> +      message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed")

Nit: the message is not very informative. I still don't know how to
rewrite it, since compilers just print the input for unexisting files.
Hence, I am not sure: do we really need this error message, since the
given command is valid?

> +    endif()
> +
>       # Fortunately, we are not interested in macOS here, so we can
>       # use realpath.
> +    # Beware, builtin command returns exit code equal to 0,
> +    # we cannot check is it failed or not.
>       execute_process(
>         COMMAND realpath ${LIB_LINK}
>         OUTPUT_VARIABLE ${output}
>         OUTPUT_STRIP_TRAILING_WHITESPACE
> +      TIMEOUT 5

(*) And here?

>       )
>     endmacro()
>     LibRealPath(LIB_STDCPP libstdc++.so)
> -  # XXX: GCC requires both. Clang requires only libstdc++.
>     if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
>       LibRealPath(LIB_ASAN libasan.so)
> -    # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`.
> -    # ";" will expand to " " as we want.
> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}")
> +    # XXX: The items of the list in LD_PRELOAD can be separated
> +    # by spaces or colons, and there is no support for escaping
> +    # either separator, see ld.so(8).
> +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN}:${LIB_STDCPP})
>     else()
> -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}")
> +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP})
>     endif()
>   endif()

Minor: Maybe it is better to move this change to a separate patch.
Feel free to ignore.

<snipped>

> >> +add_test_suite_target(LuaJIT-tests
> >> +  LABEL ${TEST_SUITE_NAME}
> >> +  DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs
> > Why don't use `${LUAJIT_TEST_BINARY}` here?
> 
> 
> AFAIR we decided with Igor in a verbal conversation
> 
> to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}.
> 
> LUAJIT_TEST_BINARY is a single target that can be used as a command,
> 
> see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a 
> list of targets
> 
> that should be build before running tests.

Yes, but we are now setting LUAJIT_TEST_DEPS unconditionally.
Also, as I mentioned above for tarantool-c-tests, there is no need to
build the luajit-main target -- libluajit is enough. Hence, this change
may be dropped.
CC imun@ here.

> 
> >
> >> +)
> >> +
> >> +# The test suite has its own test runner
> >> +# (test/LuaJIT-tests/test.lua), it is not possible to run these
> >> +# tests one-by-one by CTest.
> >> +add_test(NAME "test/LuaJIT-tests"
> > Why don't use just LuaJIT-tests as a test name here?
> For consistency with other test titles.

Side note: It looks inconsistent anyway (partially):

|       Start   1: LuaJIT-tests-deps
| 1/211 Test   #1: LuaJIT-tests-deps ..............................................................   Passed    0.29 sec
|       Start   2: test/LuaJIT-tests
| 2/211 Test   #2: test/LuaJIT-tests ..............................................................   Passed    3.56 sec
|       Start   3: PUC-Rio-Lua-5.1-tests-deps
| 3/211 Test   #3: PUC-Rio-Lua-5.1-tests-deps .....................................................   Passed    0.35 sec
|       Start   4: test/PUC-Rio-Lua-5.1

As you can see, *-deps targets have no test in their name.
But I can't come up with something meaningful, so feel free to ignore
it.

> > The same question to the tests below.
> >
> >> +  COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
> >> +                                 +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA}
> >>     WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
> >>   )
> >> +set_tests_properties("test/LuaJIT-tests" PROPERTIES
> >> +  LABELS ${TEST_SUITE_NAME}
> >> +  ENVIRONMENT "${LUAJIT_TESTS_ENV}"
> >> +  DEPENDS LuaJIT-tests-deps
> >> +)
> >> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
> >> index 2f90da86..f0e639e1 100644
> >> --- a/test/LuaJIT-tests/src/CMakeLists.txt
> >> +++ b/test/LuaJIT-tests/src/CMakeLists.txt

<snipped>

> >> +  set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
> > I suppose, that general part "test/" may be ommited from the title.
> I would left as is. The main idea is to map test title to a file/dir on 
> the filesystem.
> >

It's kind of obvious to me that tests are placed in the directory
"test". I'm not insisting, but the names of the tests are already long
enough, it will be nicer to decrease their length with unnecessary
information. The "test" directory is still the root directory for all
tests.

OTOH, it is more convenient to copy-paste the filenames, but for sure, we
must use the whole filename to be consistent with the prove's output we
are replacing.

<snipped>

> >> +# To workaround this, we glob source files and generated tests
> >> +# with paths to executable binaries.
> >> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c")
> > Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c".
> >
> > You may use the cycle above, which adds build targets.
> 
> This is a good idea, but anyway we need two loops.
> 
> First should build a list TESTS_COMPILED that is used in 
> add_test_suite_target.
> 
> The second one should generate  CMake tests.
> 
> If you are still want to have a single loop for everything then
> 
> we need to build -deps target and CMake test for each tarantool C test.
Shouldn't it be enough to set
| DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${exe}
for each test? But, yes, in this way, the order of compilation and
execution isn't sequential...
Maybe we should use the proxy target here. In the loop, we append a
dependency for this target and set it as a dependency for each test
target. May it work?

> 
> Left without changes.

May you please add a comment about it to avoid confusion in the future?

> 

<snipped>

> >> +set(TEST_SUITE_NAME "tarantool-tests")
> >> +
> >> +# XXX: The call produces both test and target
> >> +# <tarantool-tests-deps> as a side effect.
> >> +add_test_suite_target(tarantool-tests
> >> +  LABEL ${TEST_SUITE_NAME}
> >> +  DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs
> >>   )
> >>   
> >> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
> >> +# with dependencies are set in scope of BuildTestLib macro.
> >> +set(TEST_ENV "LUA_PATH=${LUA_PATH};\;\;;LUA_CPATH=${LUA_CPATH}\;\;;${LUA_TEST_ENV_MORE}")
> >> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
> > Why do we need GLOB_RECURSE here, instead of GLOB?
> 
> 
> it is better to search test files recursively, no one can guarantee that
> files always will be in a top testsuite dir.

Ok, thanks for the clarification.

<snipped>

-- 
Best regards,
Sergey Kaplun