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