Re: [Tarantool-patches] [PATCH luajit v6 2/2] test: add tests for debugging extensions

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
See my comments below.
On Thu, Apr 04, 2024 at 01:27:19PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> Hi, Max
>
> thanks for the patch. See my comments below:
>
>
> On 4/4/24 01:21, Maxim Kokryashkin wrote:
> > From: Maksim Kokryashkin <max.kokryashkin@gmail.com>
> >
> > This patch adds tests for LuaJIT debugging
> > extensions for lldb and gdb.
> > ---
<snipped>
> > +else()
> > +  message(WARNING "`gdb' is not found, so LuaJIT-gdb-extension-tests is dummy")
> > +endif()
> > +
> > +find_program(LLDB lldb)
> > +if(LLDB)
> > +  set(test_title "test/${TEST_SUITE_NAME}/lldb")
>
> I would use a real path as a test title like we do in other testsuites
>
> (PUC Rio Lua and LuaJIT tests are exception in this rule).
>
> For these test we have two flavors, so I suggest to create symlinks:
>
> ~/sources/MRG/tarantool/third_party/luajit/test/LuaJIT-debug-extensions-tests$
> ls -la
> total 20
> drwxrwxr-x  2 sergeyb sergeyb 4096 Apr  4 13:15 .
> drwxrwxr-x 10 sergeyb sergeyb 4096 Apr  4 12:44 ..
> -rw-rw-r--  1 sergeyb sergeyb 2451 Apr  4 12:44 CMakeLists.txt
> -rw-rw-r--  1 sergeyb sergeyb 6787 Apr  4 12:44 debug-extension-tests.py
> lrwxrwxrwx  1 sergeyb sergeyb   24 Apr  4 13:15 gdb-debug-extension-tests.py
> -> debug-extension-tests.py
> lrwxrwxrwx  1 sergeyb sergeyb   24 Apr  4 13:15
> lldb-debug-extension-tests.py -> debug-extension-tests.py
>
> And generate CMake tests for these files:
>
> --- a/test/LuaJIT-debug-extensions-tests/CMakeLists.txt
> +++ b/test/LuaJIT-debug-extensions-tests/CMakeLists.txt
> @@ -41,14 +41,11 @@ set(DEBUGGER_TEST_ENV
> "DEBUGGER_EXTENSION_PATH=${PROJECT_SOURCE_DIR}/src/luajit_dbg.py"
>  )
>
> -set(TEST_SCRIPT_PATH
> - ${PROJECT_SOURCE_DIR}/test/LuaJIT-debug-extensions-tests/debug-extension-tests.py
> -)
> -
>  find_program(GDB gdb)
>  if(GDB)
> -  set(test_title "test/${TEST_SUITE_NAME}/gdb")
> +  set(test_title "test/${TEST_SUITE_NAME}/gdb-debug-extension-tests.py")
>    set(GDB_TEST_ENV ${DEBUGGER_TEST_ENV} "DEBUGGER_COMMAND=${GDB}")
> +  set(TEST_SCRIPT_PATH
> ${CMAKE_CURRENT_SOURCE_DIR}/gdb-debug-extension-tests.py)
>    add_test(NAME "${test_title}"
>      COMMAND ${PYTHON_EXECUTABLE} ${TEST_SCRIPT_PATH}
>      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> @@ -64,8 +61,9 @@ endif()
>
>  find_program(LLDB lldb)
>  if(LLDB)
> -  set(test_title "test/${TEST_SUITE_NAME}/lldb")
> +  set(test_title "test/${TEST_SUITE_NAME}/lldb-debug-extension-tests.py")
>    set(LLDB_TEST_ENV ${DEBUGGER_TEST_ENV} "DEBUGGER_COMMAND=${LLDB}")
> +  set(TEST_SCRIPT_PATH
> ${CMAKE_CURRENT_SOURCE_DIR}/lldb-debug-extension-tests.py)
>    add_test(NAME "test/${TEST_SUITE_NAME}/lldb"
>      COMMAND ${PYTHON_EXECUTABLE} ${TEST_SCRIPT_PATH}
>      WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>
> In CTest test titles looks as a real file names:
>
> $ ctest -L LuaJIT-dbg-extension-tests
> Test project
> /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64
>     Start 212: LuaJIT-dbg-extension-tests-deps
> 1/2 Test #212: LuaJIT-dbg-extension-tests-deps
> ................................   Passed    0.01 sec
>     Start 213: test/LuaJIT-dbg-extension-tests/gdb-debug-extension-tests.py
> 2/2 Test #213: test/LuaJIT-dbg-extension-tests/gdb-debug-extension-tests.py
> ... Passed    2.11 sec
>
> 100% tests passed, 0 tests failed out of 2
>
> Label Time Summary:
> LuaJIT-dbg-extension-tests    =   2.12 sec*proc (2 tests)
>
> Total Test time (real) =   2.14 sec
>
>
> What do you think?
>
Well, while I see the clear reason why you suggest to create those
symlinks, I don't favor this idea.

The issue is that symlinks are going to mislead people from
outside of the team. You are not likely to check whether the file you
decided to open is a symlink or a hardlink, but you are likely to
think that these are duplicates. At this point, the test file is kind of
defeats its own purpose of being an easy to maintain singular source of
truth for our debugging extensions testing.

If we want this consistency with test names in other suites, maybe we
can set the same path for both tests and add the corresponding prefix
with the debugger name in front of the path.

What do you think?


<snipped>