Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <estetus@gmail.com>
Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit][v2] cmake: replace prove with CTest
Date: Tue, 31 Oct 2023 19:17:50 +0000	[thread overview]
Message-ID: <ZUFS3sY59m0EqB7a@tarantool.org> (raw)
In-Reply-To: <9b4befc22674bcc924ad74313d968d1748b85bd9.1698053812.git.sergeyb@tarantool.org>

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.

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

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.

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.

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.

I believe that's all for the starters.

-- 
Best regards,
IM

  parent reply	other threads:[~2023-10-31 19:18 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-23  9:48 Sergey Bronnikov via Tarantool-patches
2023-10-24 13:41 ` Maxim Kokryashkin via Tarantool-patches
2023-10-31 10:42   ` Sergey Bronnikov via Tarantool-patches
2023-10-31 19:17 ` Igor Munkin via Tarantool-patches [this message]
2023-11-11 17:05   ` Sergey Bronnikov via Tarantool-patches
2023-11-16 16:15     ` Maxim Kokryashkin via Tarantool-patches
2024-03-10 20:03 ` Igor Munkin via Tarantool-patches
2024-03-12  7:09   ` Sergey Bronnikov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZUFS3sY59m0EqB7a@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=imun@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --subject='Re: [Tarantool-patches] [PATCH luajit][v2] cmake: replace prove with CTest' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox