Re: [Tarantool-patches] [PATCH luajit 4/7] test: make skipcond helper more convenient

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!
Thanks for the patch!
`skipcond` improvement is delightful, I realy like it!

Please, consider my comments below.

On 13.02.23, Igor Munkin wrote:
> This patch provides two enhancements for <utils.skipcond>:
> 1. As a result of the patch, <utils.skipcond> becomes multi-conditional,
>    so there is no need to concatenate one huge and complex condition
>    with one huge unreadable reason. Now skipcond receives the table with
>    conditions and skips the test if one of the given conditions is met.
> 2. <utils.skipcond> yields the test object, that allows to chain
>    skipcond call right next to the test constructor.
> 
> Finally as a result of the aforementioned changes <utils.skipcond> is
> moved to tap.lua.
> 
> Relates to tarantool/tarantool#8252
> 
> Co-authored-by: Sergey Kaplun <skaplun@tarantool.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---

<snipped>

>  16 files changed, 104 insertions(+), 125 deletions(-)
> 
> diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> index 4d69250f..65f9faac 100644
> --- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
> +++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua

<snipped>

> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index f4795db0..2d6c3b06 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua

<snipped>

> diff --git a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> index e0b6d86d..019fed29 100644
> --- a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> +++ b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua

<snipped>

>  
> -jit.off()
> -jit.flush()
> +-- XXX: Run JIT tuning functions in a safe frame to avoid errors
> +-- thrown when LuaJIT is compiled with JIT engine disabled.
> +pcall(jit.off)
> +pcall(jit.flush)

Should this be the part of the next patch?

>  
>  local bufread = require "utils.bufread"
>  local symtab = require "utils.symtab"
> @@ -20,7 +17,7 @@ local testboth = require "resboth"

<snipped>

> diff --git a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> index 15bd0a8b..f139b6b5 100644
> --- a/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua
> +++ b/test/tarantool-tests/gh-7264-add-proto-trace-sysprof-default.test.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-430-maxirconst.test.lua b/test/tarantool-tests/lj-430-maxirconst.test.lua
> index 8bc523c2..633ab676 100644
> --- a/test/tarantool-tests/lj-430-maxirconst.test.lua
> +++ b/test/tarantool-tests/lj-430-maxirconst.test.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> index b5353e85..c67da00e 100644
> --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
> @@ -1,9 +1,15 @@
>  local tap = require('tap')
> -
>  -- Test to demonstrate the incorrect JIT behaviour when an error
>  -- is raised on restoration from the snapshot.
>  -- See also https://github.com/LuaJIT/LuaJIT/issues/603.
> -local test = tap.test('lj-603-err-snap-restore.test.lua')
> +local test = tap.test('lj-603-err-snap-restore'):skipcond({
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',

Side note: discussed offline that we can disable the whole test file,
even that the first test check the behaviour for the VM too.

> +  -- XXX: The different amount of stack slots is in-use for
> +  -- Tarantool at start, so just skip test for it.
> +  -- luacheck: no global
> +  ['Disable test for Tarantool'] = _TARANTOOL,
> +})
> +
>  test:plan(2)
>  
>  -- XXX: This is fragile. We need a specific amount of Lua stack
> @@ -15,7 +21,7 @@ test:plan(2)
>  -- error handling"), etc.).
>  -- This amount is suited well for GC64 and non-GC64 mode.
>  -- luacheck: no unused
> -local _, _, _, _, _, _
> +local _, _, _, _, _, _, _, _, _, _, _, _, _, _, _

Side note: Is necessary due to different allocations and memory mapping,
so we can hit `ERRERR` error during raising the `STKOV` error and
handler will be called.

>  
>  local handler_is_called = false
>  local recursive_f
> @@ -38,11 +44,7 @@ end

<snipped>

> diff --git a/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua b/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua
> index 1dc741d8..2165afe3 100644
> --- a/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua
> +++ b/test/tarantool-tests/lj-672-cdata-allocation-recording.test.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-906-fix-err-mem.test.lua b/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> index 450beeff..6c6df338 100644
> --- a/test/tarantool-tests/lj-906-fix-err-mem.test.lua
> +++ b/test/tarantool-tests/lj-906-fix-err-mem.test.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index cf92757c..60a0ccbd 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua

<snipped>

> diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> index 42a9adf9..a41a1c7a 100644
> --- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> +++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> @@ -1,12 +1,15 @@

<snipped>

> +local maxnins = require('utils').const.maxnins

Minor: since this is the constant for the test should we name it in the
upper-case?

>  local jit_opt_default = {

<snipped>

> -    jit.opt.start(0, "hotloop=1", "hotexit=2",
> -                  ("minstitch=%d"):format(utils.const.maxnins))
> +    jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(maxnins))
>  
>      local function foo(i)
>          -- math.fmod is not yet compiled!
> diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> index 0c170d0c..e5ee7902 100644
> --- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua

<snipped>

>  
> +local maxnins = require('utils').const.maxnins

Ditto.

>  local jit_opt_default = {
>      3, -- level
>      "hotloop=56",

<snipped>

> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index bae0c27c..accb1ee1 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua

<snipped>

> @@ -28,8 +28,8 @@ local memprof = require "memprof.parse"
>  local process = require "memprof.process"
>  local symtab = require "utils.symtab"
>  
> -local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin")
> -local BAD_PATH = utils.profilename("memprofdata/tmp.bin")
> +local TMP_BINFILE = require("utils").profilename("memprofdata.tmp.bin")

Don't get the point.
Why do not require this module once, so there is no need to change these
lines?

> +local BAD_PATH = require("utils").profilename("memprofdata/tmp.bin")
>  local SRC_PATH = "@"..arg[0]
>  
>  local function default_payload()
> diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
> index dad0fe4a..2b0e25ae 100644
> --- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-capi.test.lua

<snipped>

> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index 4bf10e8d..8dc36592 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua

<snipped>

> @@ -18,8 +13,8 @@ local bufread = require("utils.bufread")
>  local symtab = require("utils.symtab")
>  local sysprof = require("sysprof.parse")
>  
> -local TMP_BINFILE = utils.profilename("sysprofdata.tmp.bin")
> -local BAD_PATH = utils.profilename("sysprofdata/tmp.bin")
> +local TMP_BINFILE = require("utils").profilename("sysprofdata.tmp.bin")
> +local BAD_PATH = require("utils").profilename("sysprofdata/tmp.bin")

Ditto.

>  
>  local function payload()
>    local function fib(n)
> diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
> index a1f54d20..ac04c01d 100644
> --- a/test/tarantool-tests/tap.lua
> +++ b/test/tarantool-tests/tap.lua
> @@ -304,6 +304,17 @@ local function check(test)
>    return test.planned == test.total and test.failed == 0
>  end
>  
> +local function skipcond(test, conditions)
> +  for message, condition in pairs(conditions) do
> +    if condition then
> +      test:plan(1)
> +      test:skip(message)
> +      os.exit(test:check() and 0 or 1)

Does this mean that the subtest should be the last test to run in the
file? It's kinda not obvious. Also, I suggest the following approach:
Keep that condition search result, and skip every test (and subtest) if
the condition met. We can still skip full test, like you done, if it has
no parent test to run (has no test.parent).

> +    end
> +  end
> +  return test
> +end
> +
>  test_mt = {
>    __index = {
>      test       = new,
> @@ -313,6 +324,7 @@ test_mt = {
>      ok         = ok,
>      fail       = fail,
>      skip       = skip,
> +    skipcond   = skipcond,
>      is         = is,
>      isnt       = isnt,
>      isnil      = isnil,
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index 41a7c22a..4fb66600 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua

<snipped>

> -- 
> 2.30.2
> 

-- 
Best regards,
Sergey Kaplun