From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 5DD27A073E8; Mon, 8 Apr 2024 12:45:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5DD27A073E8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1712569513; bh=9VAuFkbHoBqOYJUVXpBSlDWJQhtseq8wzRezbeXPgcM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=rRNJBZXpVvabp767SqHifSbBbJ0iE3PiURWj9p7QWqKf/VcaolNEyRjGwhBDM8E37 O2GXxFctqxxhTqmAF4SkZess5bHFXNENOz1vCsmLq1bqJkEIrUXTU9+NOGx0usJ2QM K0oIDXv2jibkVtgR6T1wA744oeqhJyUuxtY7m8sA= Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [95.163.41.81]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 76693A073E7 for ; Mon, 8 Apr 2024 12:45:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 76693A073E7 Received: by smtp40.i.mail.ru with esmtpa (envelope-from ) id 1rtlYt-0000000D3yb-1q9j; Mon, 08 Apr 2024 12:45:11 +0300 Date: Mon, 8 Apr 2024 12:45:10 +0300 To: Sergey Bronnikov Message-ID: References: <121161805fff1e9b5855f1092bc16c24af3da3de.1712182830.git.m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9517FD34F787F37912125E25EEC01EB21C392840CC940D142182A05F538085040C5D23957FA685FDFAC8EDD30083ED68E7CE85EF88A0F8BC1B7605466E62ADAC7495FBEDC84B91D45 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73F300A97FDD4E158EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E0942BECCAD85D078638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D89F8CA3BC4BE251B574C83A94BD1E6F8232275A70C583BC25CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81D471462564A2E19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C4E7D9683544204AFC0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3F6D1C8D476B9D508BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF17B107DEF921CE791DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3C9EEE74C166EF7BC35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5788078326530E02B5002B1117B3ED696957AD48B5BAB53D0108A05421C070DB8823CB91A9FED034534781492E4B8EEAD47A3109F1ACFD409BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF671A2C935A9169ECE67A452ED2F46DFDB63BF38234C715FABACA1F1E1F4566FE6E47D229C2E76ED41CFDFDFCAF5E4A4B8AB7EB80A45649F097CD324D4B5441D3CDDC80BB77B75285C226CC413062362A913E6812662D5F2A54F6898A6FDCBDC72A617DFBE5FEC2C6383653B6C8D9AE0FD16FCAA6493B703A X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojabKNeIbMV3vj2NPeiVd7ww== X-Mailru-Sender: 7940E2A4EB16C9976F80DCDCD59EC22768C812DF0B2C521FAC8EDD30083ED68E7CE85EF88A0F8BC1E2527C969975515CFF9FCECFB8D89CB6C77752E0C033A69E235A20A81F3B0E39AB3C5F247CB2F7F93A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v6 2/2] test: add tests for debugging extensions X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: Maxim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > > > This patch adds tests for LuaJIT debugging > > extensions for lldb and gdb. > > --- > > +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?