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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for the patch!

On 15.02.23, Sergey Kaplun wrote:
> 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>

> > 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?

Yes, thanks for noticing. Moved as well as all other occurrences.

> 
> >  
> >  local bufread = require "utils.bufread"
> >  local symtab = require "utils.symtab"

<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.

I guess we'll go back to your original approach here (see Max
suggestion in the next thread).

> 
> > +  -- 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.

Well, I can't agree with Max here: I guess this should be stopped within
this patchset. See the next thread for the changes.

> 
> >  
> >  local handler_is_called = false
> >  local recursive_f

<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?

Well, I don't have any arguments for or against this approach, so I just
do it, if you want. The diff is below.

================================================================================

diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
index c8879f57..654e5545 100644
--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
+++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
@@ -10,7 +10,7 @@ local path = arg[0]:gsub('%.test%.lua', '')
 local suffix = package.cpath:match('?.(%a+);')
 package.cpath = ('%s/?.%s;'):format(path, suffix)..package.cpath
 
-local maxnins = require('utils').const.maxnins
+local MAXNINS = require('utils').const.maxnins
 local jit_opt_default = {
     3, -- level
     "hotloop=56",
@@ -95,7 +95,7 @@ end))
 
 -- Compiled loop with a side exit which does not get compiled.
 test:ok(testgetmetrics.snap_restores(function()
-    jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(maxnins))
+    jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(MAXNINS))
 
     local function foo(i)
         -- math.fmod is not yet compiled!

================================================================================

> 
> >  local jit_opt_default = {

<snipped>

> > 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.

Ditto, see the diff below:

================================================================================

diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
index b1f86cea..881e717b 100644
--- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
+++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua
@@ -10,7 +10,7 @@ local test = tap.test("lib-misc-getmetrics"):skipcond({
 
 test:plan(10)
 
-local maxnins = require('utils').const.maxnins
+local MAXNINS = require('utils').const.maxnins
 local jit_opt_default = {
     3, -- level
     "hotloop=56",
@@ -280,7 +280,7 @@ test:test("snap-restores-loop-side-exit-non-compiled", function(subtest)
     -- Compiled loop with a side exit which does not get compiled.
     subtest:plan(1)
 
-    jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(maxnins))
+    jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(MAXNINS))
 
     local function foo(i)
         -- math.fmod is not yet compiled!

================================================================================

> 
> >  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?

Hm. Ya prosto objebalsa tut. You and Max are right, I've fixed this the
following way (the diff is below):

================================================================================

diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index e2701c13..1d3f734e 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -27,9 +27,10 @@ local bufread = require "utils.bufread"
 local memprof = require "memprof.parse"
 local process = require "memprof.process"
 local symtab = require "utils.symtab"
+local profilename = require("utils").profilename

-local TMP_BINFILE = require("utils").profilename("memprofdata.tmp.bin")
-local BAD_PATH = require("utils").profilename("memprofdata/tmp.bin")
+local TMP_BINFILE = profilename("memprofdata.tmp.bin")
+local BAD_PATH = profilename("memprofdata/tmp.bin")
 local SRC_PATH = "@"..arg[0]

 local function default_payload()

================================================================================

> 
> > +local BAD_PATH = require("utils").profilename("memprofdata/tmp.bin")
> >  local SRC_PATH = "@"..arg[0]
> >  
> >  local function default_payload()

<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.

Ditto, see the diff below:

================================================================================

diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
index dcde81b9..3e7c0005 100644
--- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
@@ -14,9 +14,10 @@ pcall(jit.flush)
 local bufread = require("utils.bufread")
 local symtab = require("utils.symtab")
 local sysprof = require("sysprof.parse")
+local profilename = require("utils").profilename

-local TMP_BINFILE = require("utils").profilename("sysprofdata.tmp.bin")
-local BAD_PATH = require("utils").profilename("sysprofdata/tmp.bin")
+local TMP_BINFILE = profilename("sysprofdata.tmp.bin")
+local BAD_PATH = profilename("sysprofdata/tmp.bin")

 local function payload()
   local function fib(n)

================================================================================

> 
> >  
> >  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).

It means that ya objebalsa one more time... Well, I have to declare that
this approach doesn't work for subtests. Hence, I suggest the following:
1. Proceed with this patchset for the root (i.e. parent) tests, since
   this is the most popular way to write tests in Tarantool suite.
2. Think a little how to solve the problem for subtests. For now it's
   too complex: plain Lua assertions, JIT engine tuning, etc --
   everything need to be considered and carefully handled.

Anyway, to prevent skipcond misuse I added the next assertion:

================================================================================

diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
index ac04c01d..f7be9c81 100644
--- a/test/tarantool-tests/tap.lua
+++ b/test/tarantool-tests/tap.lua
@@ -305,6 +305,7 @@ local function check(test)
 end
 
 local function skipcond(test, conditions)
+  assert(test.parent ~= nil, 'FIXME: test:skipcond works only for parent tests')
   for message, condition in pairs(conditions) do
     if condition then
       test:plan(1)

================================================================================

> 
> > +    end
> > +  end
> > +  return test
> > +end
> > +
> >  test_mt = {
> >    __index = {
> >      test       = new,

<snipped>

> > -- 
> > 2.30.2
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM