<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Суббота, 11 ноября 2023, 20:05 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16997223231430802208_BODY">Hi, Igor<br><br>On 10/31/23 22:17, Igor Munkin wrote:<br>> Sergey,<br>><br>> Thanks for the patch! I have some general comments prior to make more<br>> precise review for the particular changeset.<br>><br>> 1. Unfortunately, the old scenario is broken:<br>> The recipe I used to run for building and running LuaJIT test is the<br>> following one:<br>> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON && make -j test<br>> As a result of the changes, <test> target doesn't build the<br>> dependencies (neither luajit per se, nor particular test helpers<br>> required to be compiled). So the recipe is transformed to this one:<br>> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON<br>> | $ make -j # build LuaJIT<br>> | $ make -j tarantool-tests # run the particular suite to build the requirements<br>> | $ make -j tarantool-c-tests # ditto<br>> | $ make -j PUC-Rio-Lua-5.1-tests # some binary artefacts for Lua suite<br>> | $ make -j test<br>> Looks not such convenient comparing to the old mantra.<br><br>Updated patch a bit. Now steps are the same:<br><br> $ cmake -S . -B build<br> $ cmake --build build<br> $ cmake --build --target LuaJIT-test<br><br>ctest and test target works too.<br><br><br>><br>> 2. The new CTest targets should follow the existing naming practice<br>> (i.e. use the directory name, or its distinctive prefix), since the<br>> following command runs three suites.<br>> | $ ctest -L lua # runs lua, luajit and lua-harness<br>Updated labels, now labels are equals to prefix of appropriate testsuite.<br>><br>> 3. It looks like the logs will not be showed in CI to debug a<br>> hard-reproducible issue if one occurs. <--output-on-failure> can be a<br>> saviour here for us.<br><br>This option is already used by test targets, see TEST_FLAGS in<br>test/CMakeLists.txt.<br><br>If you run tests by ctest then you should set env variable<br>CTEST_OUTPUT_ON_FAILURE=1.<br><br>Another option is setting default ctest options via<br>CMAKE_CTEST_ARGUMENTS, but it is available since CMake 3.17.<br><br>><br>> 4. I'm afraid the current changes break integration with Tarantool<br>> tests. The reason is duplication of <test> target both in LuaJIT and<br>> Tarantool build systems. AFAIU, moving <enable_testing> under the<br>> condition similar to the one handled with LUAJIT_USE_TEST option<br>> doesn't solve the issue, since no test will be run as a result.<br>> Please check this part: it might be the main technical blocker for<br>> this changeset.<br><br>No, it doesn't break integration. Letter with patch has a link to PR to<br>tarantool repository,<br><br>all CI tests are passed.<br><br>> 5. I remember that we're talking regarding running particular tests for<br>> all suites, but I see this is done only for tarantool* and lua-Harness.<br>> Is it experimental and you'll fix it later in scope of this PR? There<br>> is definitely the way to run a particular test in LuaJIT suite<br>> (AFAIR, you can just use its number). Regarding PUC-Rio-Lua-5.1 we<br>> can think about replacing all.lua as a runner as it's done in uJIT.<br><br>The patch replaces prove with ctest. Updating test runner for PUC Rio<br>Lua and LuaJIT testsuites<br><br>is out of scope for this patch. This could be done later and by someone<br>else.<br><br>><br>> I believe that's all for the starters.</div></div></div></div></blockquote><div> </div></BODY></HTML>