Re: [Tarantool-patches] [PATCH v5 luajit 2/3] cmake: run tests with Valgrind

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

Hi, Sergey!
Thanks for the comments!
Fixed them and force-pushed the branch.

On 17.12.24, Sergey Bronnikov wrote:
> Hello,
> 
> Thanks for fixes!
> 
> LGTM after fixing minor comments below.
> 
> On 16.12.2024 19:40, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thansk for the review!
> > See my answers below, all patches are force-pushed to the branch.
> > The branch is rebased on master.
> >
> > On 13.12.24, Sergey Bronnikov wrote:
> >
> <snipped>
> > | cmake: run tests with Valgrind
> > |
> > | This patch enables running tests with Valgrind. There is a
> > | `VALGRIND_OPTS` variable [1] that we can set -- it makes the usage of
> > | Valgrind more flexible -- we can define any necessary flags in the
> > | command line (not at the building stage). By default, the suppression
> > | files are set to <src/lj.supp> (original suppression file in LuaJIT) and
> > | an additional one <src/lj_extra.supp> (maintained by us).
> > |
> > | Also, this patch disables the following tests when running with Valgrind
> > | due to failures.
> please replace dot with colon, because below the enumeration of disabled 
> tests

Replaced.

> > |
> > | The <tarantool-tests/lj-512-profiler-hook-finalizers.test.lua> test is
> > | disabled due to its time sensitivity (it is not run the expected amount
> > | of time with Valgrind).
> > |
> > | These tests from the tarantool-tests suite are disabled due to
> > | tarantool/tarantool#10803:
> > |   - lj-726-profile-flush-close.test.lua
> > |   - profilers/gh-5688-tool-cli-flag.test.lua
> > |   - profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
> > |   - profilers/misclib-sysprof-lapi.test.lua
> > |
> > | Timed out due to running under Valgrind:
> > |   - tarantool-c-tests/gh-8594-sysprof-ffunc-crash.c_test
> > |   - tarantool-tests/gh-7745-oom-on-trace.test.lua
> > |   - tarantool-tests/lj-1034-tabov-error-frame.test.lua
> > |
> > | [1]:https://valgrind.org/docs/manual/manual-core.html#manual-core.defopts
> > |
> > | Part of tarantool/tarantool#3705
> >

<snipped>

> >     )
> > +  # Set the exit code to non-zero to automatically detect
> > +  # failing tests.
> because "when set to the default value (zero),
> the return value from Valgrind will always be the return value of the
> process being simulated.", I believe it is an important thing, and it's 
> worth to add to comment.

Enriched the comment:
===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index a5075f05..87e4b907 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -88,8 +88,10 @@ if(LUAJIT_USE_VALGRIND)
     --suppressions=${LUAJIT_SOURCE_DIR}/lj.supp
     --suppressions=${LUAJIT_SOURCE_DIR}/lj_extra.supp
   )
-  # Set the exit code to non-zero to automatically detect
-  # failing tests.
+  # When set to the default value (zero), the return value from
+  # Valgrind will always be the return value of the process being
+  # simulated. Set the exit code to non-zero to automatically
+  # detect failing tests.
   set(LUAJIT_TEST_VALGRIND_COMMAND
       ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
   set(LUAJIT_TEST_COMMAND
===================================================================

> >     set(LUAJIT_TEST_COMMAND
> >       "${VALGRIND} --error-exitcode=1 "
> >       "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
> > ===================================================================
> >
> >>> +    "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")

<snipped>

> >>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> >>> index 30d174bb..5f6c45da 100644
> >>> --- a/test/tarantool-c-tests/CMakeLists.txt
> >>> +++ b/test/tarantool-c-tests/CMakeLists.txt
> >>> @@ -56,10 +56,19 @@ foreach(test_source ${tests})
> >>>    
> >>>      # Generate CMake tests.
> >>>      set(test_title "test/${TEST_SUITE_NAME}/${exe}${C_TEST_SUFFIX}")
> >>> +  set(test_command ${CMAKE_CURRENT_BINARY_DIR}/${exe}${C_TEST_SUFFIX})
> >>> +
> >>> +  if(LUAJIT_USE_VALGRIND)
> >>> +    set(test_command ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP}
> >> The same already defined in test/CMakeLists.txt, could you reuse already
> >> defined value?
> > Yes, good catch!
> > ===================================================================
> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index 9685c494..fcd697c9 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -90,9 +90,10 @@ if(LUAJIT_USE_VALGRIND)
> >     )
> >     # Set the exit code to non-zero to automatically detect
> >     # failing tests.
> > +  set(LUAJIT_TEST_VALGRIND_COMMAND
> > +    ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
> bad indentation
> >     set(LUAJIT_TEST_COMMAND
> > -    "${VALGRIND} --error-exitcode=1 "
> > -    "${LUAJIT_TEST_VALGRIND_SUPP} ${LUAJIT_TEST_COMMAND}")
> > +    "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
> bad indentation

Added 2 addtional spaces:

===================================================================
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index fcd697c9..a5075f05 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -91,9 +91,9 @@ if(LUAJIT_USE_VALGRIND)
   # Set the exit code to non-zero to automatically detect
   # failing tests.
   set(LUAJIT_TEST_VALGRIND_COMMAND
-    ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
+      ${VALGRIND} --error-exitcode=1 ${LUAJIT_TEST_VALGRIND_SUPP})
   set(LUAJIT_TEST_COMMAND
-    "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
+      "${LUAJIT_TEST_VALGRIND_COMMAND} ${LUAJIT_TEST_COMMAND}")
 endif()
 
 separate_arguments(LUAJIT_TEST_COMMAND)
===================================================================

> >   endif()
> >   
> >   separate_arguments(LUAJIT_TEST_COMMAND)

<snipped>

> >

-- 
Best regards,
Sergey Kaplun