Re: [Tarantool-patches] [PATCH v2 luajit 1/6] test: introduce tests for debugging extensions

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

Hi, Sergey!
Thanks for the review!

See my answers below.

On 20.05.26, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for the patch! Please see my comments.
> 
> Sergey
> 
> On 5/19/26 15:39, Sergey Kaplun wrote:
> > From: Maxim Kokryashkin<m.kokryashkin@tarantool.org>
> 
> It looks like the patch was made by Maxim K. and you took it as is, but 
> it is not so.
> 
> The latest version of this patch is here [1] and there several changes 
> introduced by you.
> 
> I suppose these changes should be described and you should to somehow
> 
> say that you changed the patch a little (for example using Git trailer 
> "Co-authored-by:").

Added Co-authored-by.
| test: introduce tests for debugging extensions
|
| This patch adds tests for LuaJIT debugging extensions for lldb and gdb.
| The tests are written in Python's unittest framework [1].
|
| Most of the tests are failed for the lldb due to outdated extension
| sources and overcomplicated hard-coded C structures fields
| introspection. Hence, tests for LLDB are disabled since they are failing
| anyway.
|
| The tarantool-debugger-tests target is introduced. This target is
| included in the LuaJIT-check-all target but not in the LuaJIT-test
| target to avoid it running for all LuaJIT builds by default in CI.
|
| [1]: https://docs.python.org/3/library/unittest.html
|
| Co-authored-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> Otherwise, we are following bad practices, let's not be like those 
> people we usually joke about.
> 
> The full diff is below:

<snipped>

> [1]: 
> https://lists.tarantool.org/tarantool-patches/cover.1712182830.git.m.kokryashkin@tarantool.org/
> 
> > This patch adds tests for LuaJIT debugging extensions for lldb and gdb.
> > The tests are written in Python's unittest framework [1].
> >
> > Most of the tests are failed for the lldb due to outdated extension
> > sources and overcomplicated hard-coded C structures fields
> > introspection. Hence, tests for LLDB are disabled since they are failing
> > anyway.
> >
> > The tarantool-debugger-tests target is introduced. This target is
> > included in the LuaJIT-check-all target but not in the LuaJIT-test
> > target to avoid it running for all LuaJIT builds by default in CI.
> 
> I don't get why we cannot run debugger tests together with other tests.
> 
> dbg extension must work with all LuaJIT configurations that we test in CI,
> 
> so I suppose we should run these (dbg) tests with all other regression 
> tests.

It complicates testing requirements (you need to install the
corresponding packages) or tests will be skipped anyway. Also, it adds
issues for GDB installation on macOS. This tests can't be run without
debugging symbols (i.e. in Release builds and may be broken in
RelWithDebInfo builds). Hence it effectively restricts tests to the
mentioned in the last commit. All exotic builds are not related to the
testing nature.

Hence, I believe that tests for __language runtime__ should be separated
from the tests of the debug extension for this runtime, since they are
different entities. All runtime differences are unrelated to the testing
of this extension (as all interesting cases for us are covered by the
set of runs in the last patch).

> For example, the proposed GHA workflows doesn't cover DUALNUM build,

It is tested by arm64 build.

> 
> сan I be sure that the extension will work with DUALNUM build? Seems no.
> 
> >
> > [1]:https://docs.python.org/3/library/unittest.html
> > ---
> >   test/CMakeLists.txt                           |   7 +
> >   test/tarantool-debugger-tests/CMakeLists.txt  |  93 ++++++
> >   .../debug-extension-tests.py                  | 286 ++++++++++++++++++
> >   3 files changed, 386 insertions(+)
> >   create mode 100644 test/tarantool-debugger-tests/CMakeLists.txt
> >   create mode 100644 test/tarantool-debugger-tests/debug-extension-tests.py
> >
> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index f48afa25..26b15892 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -175,6 +175,7 @@ add_subdirectory(LuaJIT-tests)
> >   add_subdirectory(PUC-Rio-Lua-5.1-tests)
> >   add_subdirectory(lua-Harness-tests)
> >   add_subdirectory(tarantool-c-tests)
> > +add_subdirectory(tarantool-debugger-tests)
> >   add_subdirectory(tarantool-tests)
> >   
> >   # Each testsuite has its own CMake target, but combining these
> > @@ -186,6 +187,9 @@ add_subdirectory(tarantool-tests)
> >   # command that runs all generated CMake tests.
> >   add_custom_target(${PROJECT_NAME}-test
> >     COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}
> > +    # Omit this target in LuaJIT-test since we don't want to set
> > +    # up and run debuggers for every build.
> > +    --label-exclude tarantool-debugger-tests
> see the comment above

See my answer above.

> >     DEPENDS tarantool-c-tests-deps
> >             tarantool-tests-deps
> >             lua-Harness-tests-deps
> > @@ -195,5 +199,8 @@ add_custom_target(${PROJECT_NAME}-test
> >   
> >   add_custom_target(${PROJECT_NAME}-check-all
> >     DEPENDS ${PROJECT_NAME}-test
> > +          # Omit this target in LuaJIT-test since we don't want to
> > +          # set up and run debuggers for every build.
> > +          tarantool-debugger-tests
> >             ${PROJECT_NAME}-lint
> >   )
> > diff --git a/test/tarantool-debugger-tests/CMakeLists.txt b/test/tarantool-debugger-tests/CMakeLists.txt
> > new file mode 100644
> > index 00000000..7fd0debc
> > --- /dev/null
> > +++ b/test/tarantool-debugger-tests/CMakeLists.txt
> > @@ -0,0 +1,93 @@
> > +set(TEST_SUITE_NAME "tarantool-debugger-tests")
> > +
> > +# XXX: The call produces both test and target
> > +# <tarantool-debugger-tests-deps> as a side effect.
> > +add_test_suite_target(tarantool-debugger-tests
> > +  LABELS ${TEST_SUITE_NAME}
> > +  DEPENDS ${LUAJIT_TEST_BINARY}
> > +)
> > +
> > +# Debug info is required for testing of extensions.
> > +if(NOT (CMAKE_BUILD_TYPE MATCHES Debug))
> > +  message(WARNING
> > +    "Not a DEBUG build, tarantool-debugger-tests is dummy"
> > +  )
> 
> it is not dummy, it doesn't exist at all:
> 
> cmake -S . -B build -DCMAKE_BUILD_TYPE=Release
> 
> cd build
> 
> make tarantool-debugger-tests
> make[3]: *** No rule to make target 'src/luajit', needed by 
> 'test/tarantool-debugger-tests/CMakeFiles/tarantool-debugger-tests-deps'. 
> Stop.

Replaced elsewhere with "omitted".
===================================================================
diff --git a/test/tarantool-debugger-tests/CMakeLists.txt b/test/tarantool-debugger-tests/CMakeLists.txt
index 7fd0debc..a6684e72 100644
--- a/test/tarantool-debugger-tests/CMakeLists.txt
+++ b/test/tarantool-debugger-tests/CMakeLists.txt
@@ -10,7 +10,7 @@ add_test_suite_target(tarantool-debugger-tests
 # Debug info is required for testing of extensions.
 if(NOT (CMAKE_BUILD_TYPE MATCHES Debug))
   message(WARNING
-    "Not a DEBUG build, tarantool-debugger-tests is dummy"
+    "Not a DEBUG build, tarantool-debugger-tests is omitted"
   )
   return()
 endif()
@@ -22,7 +22,7 @@ endif()
 if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND DEFINED ENV{CI})
   message(WARNING
     "Interactive debugging is unavailable for macOS CI builds,"
-    " tarantool-debugger-tests is dummy"
+    " tarantool-debugger-tests is omitted"
   )
   return()
 endif()
@@ -31,13 +31,17 @@ if(CMAKE_VERSION VERSION_LESS "3.12")
   # TODO:Can remove this after upgrading to CMake >= 3.12.
   find_package(PythonInterp)
   if(NOT PYTHONINTERP_FOUND)
-    message(WARNING "`python` is not found, tarantool-debugger-tests is dummy")
+    message(WARNING
+      "`python` is not found, tarantool-debugger-tests is omitted"
+    )
     return()
   endif()
 else()
   find_package(Python COMPONENTS Interpreter)
   if(NOT PYTHON_FOUND)
-    message(WARNING "`python` is not found, tarantool-debugger-tests is dummy")
+    message(WARNING
+      "`python` is not found, tarantool-debugger-tests is omitted"
+    )
     return()
   endif()
   set(PYTHON_EXECUTABLE "${Python_EXECUTABLE}")
===================================================================

> 
> > +  return()
> > +endif()
> > +
> > +# MacOS asks for permission to debug a process even when the
> > +# machine is set into development mode. To solve the issue,
> > +# it is required to add relevant users to the `_developer` user
> > +# group in macOS. Disabled for now.
> > +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND DEFINED ENV{CI})
> > +  message(WARNING
> > +    "Interactive debugging is unavailable for macOS CI builds,"
> > +    " tarantool-debugger-tests is dummy"
> > +  )
> the same as above, it is not dummy
> > +  return()
> > +endif()
> > +
> > +if(CMAKE_VERSION VERSION_LESS "3.12")
> > +  #TODO:Can remove this after upgrading to CMake >= 3.12.
> s/TODO:/TODO/

Fixed:

===================================================================
diff --git a/test/tarantool-debugger-tests/CMakeLists.txt b/test/tarantool-debugger-tests/CMakeLists.txt
index f97e4b3c..39414db3 100644
--- a/test/tarantool-debugger-tests/CMakeLists.txt
+++ b/test/tarantool-debugger-tests/CMakeLists.txt
@@ -28,7 +28,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin" AND DEFINED ENV{CI})
 endif()
 
 if(CMAKE_VERSION VERSION_LESS "3.12")
-  # TODO:Can remove this after upgrading to CMake >= 3.12.
+  # TODO: Can remove this after upgrading to CMake >= 3.12.
   find_package(PythonInterp)
   if(NOT PYTHONINTERP_FOUND)
     message(WARNING
===================================================================

> > +  find_package(PythonInterp)
> > +  if(NOT PYTHONINTERP_FOUND)
> > +    message(WARNING "`python` is not found, tarantool-debugger-tests is dummy")

Fixed backtick here and below.
===================================================================
diff --git a/test/tarantool-debugger-tests/CMakeLists.txt b/test/tarantool-debugger-tests/CMakeLists.txt
index a6684e72..f97e4b3c 100644
--- a/test/tarantool-debugger-tests/CMakeLists.txt
+++ b/test/tarantool-debugger-tests/CMakeLists.txt
@@ -32,7 +32,7 @@ if(CMAKE_VERSION VERSION_LESS "3.12")
   find_package(PythonInterp)
   if(NOT PYTHONINTERP_FOUND)
     message(WARNING
-      "`python` is not found, tarantool-debugger-tests is omitted"
+      "`python' is not found, tarantool-debugger-tests is omitted"
     )
     return()
   endif()
@@ -40,7 +40,7 @@ else()
   find_package(Python COMPONENTS Interpreter)
   if(NOT PYTHON_FOUND)
     message(WARNING
-      "`python` is not found, tarantool-debugger-tests is omitted"
+      "`python' is not found, tarantool-debugger-tests is omitted"
     )
     return()
   endif()
===================================================================

> it is not dummy
> > +    return()
> > +  endif()
> > +else()
> > +  find_package(Python COMPONENTS Interpreter)
> > +  if(NOT PYTHON_FOUND)
> > +    message(WARNING "`python` is not found, tarantool-debugger-tests is dummy")
> it is not dummy
> > +    return()
> > +  endif()
> > +  set(PYTHON_EXECUTABLE "${Python_EXECUTABLE}")
> > +endif()
> > +
> > +set(DEBUGGER_TEST_ENV
> > +  "LUAJIT_TEST_BINARY=${LUAJIT_TEST_BINARY}"
> > +  # Suppresses __pycache__ generation.
> > +  "PYTHONDONTWRITEBYTECODE=1"
> > +  "DEBUGGER_EXTENSION_PATH=${PROJECT_SOURCE_DIR}/src"
> > +)
> > +
> > +set(TEST_SCRIPT_PATH
> > +  ${CMAKE_CURRENT_SOURCE_DIR}/debug-extension-tests.py
> > +)
> > +
> > +find_program(GDB gdb)
> > +if(GDB)
> > +  set(test_title "test/${TEST_SUITE_NAME}/gdb")
> > +  set(GDB_TEST_ENV ${DEBUGGER_TEST_ENV} "DEBUGGER_COMMAND=${GDB}")
> > +  add_test(NAME "${test_title}"
> > +    COMMAND ${PYTHON_EXECUTABLE} ${TEST_SCRIPT_PATH}
> > +    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> > +  )
> > +  set_tests_properties("${test_title}" PROPERTIES
> > +    ENVIRONMENT "${GDB_TEST_ENV}"
> > +    LABELS ${TEST_SUITE_NAME}
> > +    DEPENDS tarantool-debugger-tests-deps
> > +  )
> > +else()
> > +  message(WARNING
> > +    "`gdb' is not found, so tarantool-debugger-tests/gdb is omitted"
> > +  )
> > +endif()
> > +
> > +find_program(LLDB lldb)
> > +if(LLDB)
> > +  set(test_title "test/${TEST_SUITE_NAME}/lldb")
> > +  set(LLDB_TEST_ENV ${DEBUGGER_TEST_ENV} "DEBUGGER_COMMAND=${LLDB}")
> > +  add_test(NAME "${test_title}"
> > +    COMMAND ${PYTHON_EXECUTABLE} ${TEST_SCRIPT_PATH}
> > +    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> > +  )
> > +  set_tests_properties("${test_title}" PROPERTIES
> > +    ENVIRONMENT "${LLDB_TEST_ENV}"
> > +    LABELS ${TEST_SUITE_NAME}
> > +    DEPENDS tarantool-debugger-tests-deps
> > +  )
> > +else()
> > +  message(WARNING
> > +    "`lldb' is not found, so tarantool-debugger-tests/lldb is omitted"
> > +  )
> > +endif()
> Duplicate code for GDB and LLDB. May be use a macro for this?

I prefer not to, since it may have custom logging for debugger, and this
will make the code less readable.

> > diff --git a/test/tarantool-debugger-tests/debug-extension-tests.py b/test/tarantool-debugger-tests/debug-extension-tests.py
> > new file mode 100644
> > index 00000000..6094c535
> > --- /dev/null
> > +++ b/test/tarantool-debugger-tests/debug-extension-tests.py

<snipped>

-- 
Best regards,
Sergey Kaplun