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