<HTML><BODY><div>Hi, Igor!</div><div>Thanks for the fixes!</div><div>I am still not satisfied with `snap-restore` case,</div><div>but whatever.</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16774896791973038856_BODY">Sergey,<br><br>Thanks for the patch!<br><br>On 15.02.23, Sergey Kaplun wrote:<br>> Hi, Igor!<br>> Thanks for the patch!<br>> `skipcond` improvement is delightful, I realy like it!<br>><br>> Please, consider my comments below.<br>><br>> On 13.02.23, Igor Munkin wrote:<br>> > This patch provides two enhancements for <utils.skipcond>:<br>> > 1. As a result of the patch, <utils.skipcond> becomes multi-conditional,<br>> > so there is no need to concatenate one huge and complex condition<br>> > with one huge unreadable reason. Now skipcond receives the table with<br>> > conditions and skips the test if one of the given conditions is met.<br>> > 2. <utils.skipcond> yields the test object, that allows to chain<br>> > skipcond call right next to the test constructor.<br>> ><br>> > Finally as a result of the aforementioned changes <utils.skipcond> is<br>> > moved to tap.lua.<br>> ><br>> > Relates to tarantool/tarantool#8252<br>> ><br>> > Co-authored-by: Sergey Kaplun <<a href="/compose?To=skaplun@tarantool.org">skaplun@tarantool.org</a>><br>> > Signed-off-by: Igor Munkin <<a href="/compose?To=imun@tarantool.org">imun@tarantool.org</a>><br>> > ---<br><br><snipped><br><br>> > 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<br>> > index e0b6d86d..019fed29 100644<br>> > --- a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua<br>> > +++ b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua<br>><br>> <snipped><br>><br>> ><br>> > -jit.off()<br>> > -jit.flush()<br>> > +-- XXX: Run JIT tuning functions in a safe frame to avoid errors<br>> > +-- thrown when LuaJIT is compiled with JIT engine disabled.<br>> > +pcall(jit.off)<br>> > +pcall(jit.flush)<br>><br>> Should this be the part of the next patch?<br><br>Yes, thanks for noticing. Moved as well as all other occurrences.<br><br>><br>> ><br>> > local bufread = require "utils.bufread"<br>> > local symtab = require "utils.symtab"<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua<br>> > index b5353e85..c67da00e 100644<br>> > --- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua<br>> > +++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua<br>> > @@ -1,9 +1,15 @@<br>> > local tap = require('tap')<br>> > -<br>> > -- Test to demonstrate the incorrect JIT behaviour when an error<br>> > -- is raised on restoration from the snapshot.<br>> > -- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/603" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/603</a>.<br>> > -local test = tap.test('lj-603-err-snap-restore.test.lua')<br>> > +local test = tap.test('lj-603-err-snap-restore'):skipcond({<br>> > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',<br>><br>> Side note: discussed offline that we can disable the whole test file,<br>> even that the first test check the behaviour for the VM too.<br><br>I guess we'll go back to your original approach here (see Max<br>suggestion in the next thread).<br><br>><br>> > + -- XXX: The different amount of stack slots is in-use for<br>> > + -- Tarantool at start, so just skip test for it.<br>> > + -- luacheck: no global<br>> > + ['Disable test for Tarantool'] = _TARANTOOL,<br>> > +})<br>> > +<br>> > test:plan(2)<br>> ><br>> > -- XXX: This is fragile. We need a specific amount of Lua stack<br>> > @@ -15,7 +21,7 @@ test:plan(2)<br>> > -- error handling"), etc.).<br>> > -- This amount is suited well for GC64 and non-GC64 mode.<br>> > -- luacheck: no unused<br>> > -local _, _, _, _, _, _<br>> > +local _, _, _, _, _, _, _, _, _, _, _, _, _, _, _<br>><br>> Side note: Is necessary due to different allocations and memory mapping,<br>> so we can hit `ERRERR` error during raising the `STKOV` error and<br>> handler will be called.<br><br>Well, I can't agree with Max here: I guess this should be stopped within<br>this patchset. See the next thread for the changes.<br><br>><br>> ><br>> > local handler_is_called = false<br>> > local recursive_f<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua<br>> > index 42a9adf9..a41a1c7a 100644<br>> > --- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua<br>> > +++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua<br>> > @@ -1,12 +1,15 @@<br>><br>> <snipped><br>><br>> > +local maxnins = require('utils').const.maxnins<br>><br>> Minor: since this is the constant for the test should we name it in the<br>> upper-case?<br><br>Well, I don't have any arguments for or against this approach, so I just<br>do it, if you want. The diff is below.<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua<br>index c8879f57..654e5545 100644<br>--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua<br>+++ b/test/tarantool-tests/misclib-getmetrics-capi.test.lua<br>@@ -10,7 +10,7 @@ local path = arg[0]:gsub('%.test%.lua', '')<br> local suffix = package.cpath:match('?.(%a+);')<br> package.cpath = ('%s/?.%s;'):format(path, suffix)..package.cpath<br> <br>-local maxnins = require('utils').const.maxnins<br>+local MAXNINS = require('utils').const.maxnins<br> local jit_opt_default = {<br> 3, -- level<br> "hotloop=56",<br>@@ -95,7 +95,7 @@ end))<br> <br> -- Compiled loop with a side exit which does not get compiled.<br> test:ok(testgetmetrics.snap_restores(function()<br>- jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(maxnins))<br>+ jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(MAXNINS))<br> <br> local function foo(i)<br> -- math.fmod is not yet compiled!<br><br>================================================================================<br><br>><br>> > local jit_opt_default = {<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua<br>> > index 0c170d0c..e5ee7902 100644<br>> > --- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua<br>> > +++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua<br>><br>> <snipped><br>><br>> ><br>> > +local maxnins = require('utils').const.maxnins<br>><br>> Ditto.<br><br>Ditto, see the diff below:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua<br>index b1f86cea..881e717b 100644<br>--- a/test/tarantool-tests/misclib-getmetrics-lapi.test.lua<br>+++ b/test/tarantool-tests/misclib-getmetrics-lapi.test.lua<br>@@ -10,7 +10,7 @@ local test = tap.test("lib-misc-getmetrics"):skipcond({<br> <br> test:plan(10)<br> <br>-local maxnins = require('utils').const.maxnins<br>+local MAXNINS = require('utils').const.maxnins<br> local jit_opt_default = {<br> 3, -- level<br> "hotloop=56",<br>@@ -280,7 +280,7 @@ test:test("snap-restores-loop-side-exit-non-compiled", function(subtest)<br> -- Compiled loop with a side exit which does not get compiled.<br> subtest:plan(1)<br> <br>- jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(maxnins))<br>+ jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(MAXNINS))<br> <br> local function foo(i)<br> -- math.fmod is not yet compiled!<br><br>================================================================================<br><br>><br>> > local jit_opt_default = {<br>> > 3, -- level<br>> > "hotloop=56",<br>><br>> <snipped><br>><br>> > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua<br>> > index bae0c27c..accb1ee1 100644<br>> > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua<br>> > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua<br>><br>> <snipped><br>><br>> > @@ -28,8 +28,8 @@ local memprof = require "memprof.parse"<br>> > local process = require "memprof.process"<br>> > local symtab = require "utils.symtab"<br>> ><br>> > -local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin")<br>> > -local BAD_PATH = utils.profilename("memprofdata/tmp.bin")<br>> > +local TMP_BINFILE = require("utils").profilename("memprofdata.tmp.bin")<br>><br>> Don't get the point.<br>> Why do not require this module once, so there is no need to change these<br>> lines?<br><br>Hm. Ya prosto objebalsa tut. You and Max are right, I've fixed this the<br>following way (the diff is below):<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua<br>index e2701c13..1d3f734e 100644<br>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua<br>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua<br>@@ -27,9 +27,10 @@ local bufread = require "utils.bufread"<br> local memprof = require "memprof.parse"<br> local process = require "memprof.process"<br> local symtab = require "utils.symtab"<br>+local profilename = require("utils").profilename<br><br>-local TMP_BINFILE = require("utils").profilename("memprofdata.tmp.bin")<br>-local BAD_PATH = require("utils").profilename("memprofdata/tmp.bin")<br>+local TMP_BINFILE = profilename("memprofdata.tmp.bin")<br>+local BAD_PATH = profilename("memprofdata/tmp.bin")<br> local SRC_PATH = "@"..arg[0]<br><br> local function default_payload()<br><br>================================================================================<br><br>><br>> > +local BAD_PATH = require("utils").profilename("memprofdata/tmp.bin")<br>> > local SRC_PATH = "@"..arg[0]<br>> ><br>> > local function default_payload()<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua<br>> > index 4bf10e8d..8dc36592 100644<br>> > --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua<br>> > +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua<br>><br>> <snipped><br>><br>> > @@ -18,8 +13,8 @@ local bufread = require("utils.bufread")<br>> > local symtab = require("utils.symtab")<br>> > local sysprof = require("sysprof.parse")<br>> ><br>> > -local TMP_BINFILE = utils.profilename("sysprofdata.tmp.bin")<br>> > -local BAD_PATH = utils.profilename("sysprofdata/tmp.bin")<br>> > +local TMP_BINFILE = require("utils").profilename("sysprofdata.tmp.bin")<br>> > +local BAD_PATH = require("utils").profilename("sysprofdata/tmp.bin")<br>><br>> Ditto.<br><br>Ditto, see the diff below:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua<br>index dcde81b9..3e7c0005 100644<br>--- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua<br>+++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua<br>@@ -14,9 +14,10 @@ pcall(jit.flush)<br> local bufread = require("utils.bufread")<br> local symtab = require("utils.symtab")<br> local sysprof = require("sysprof.parse")<br>+local profilename = require("utils").profilename<br><br>-local TMP_BINFILE = require("utils").profilename("sysprofdata.tmp.bin")<br>-local BAD_PATH = require("utils").profilename("sysprofdata/tmp.bin")<br>+local TMP_BINFILE = profilename("sysprofdata.tmp.bin")<br>+local BAD_PATH = profilename("sysprofdata/tmp.bin")<br><br> local function payload()<br> local function fib(n)<br><br>================================================================================<br><br>><br>> ><br>> > local function payload()<br>> > local function fib(n)<br>> > diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua<br>> > index a1f54d20..ac04c01d 100644<br>> > --- a/test/tarantool-tests/tap.lua<br>> > +++ b/test/tarantool-tests/tap.lua<br>> > @@ -304,6 +304,17 @@ local function check(test)<br>> > return test.planned == test.total and test.failed == 0<br>> > end<br>> ><br>> > +local function skipcond(test, conditions)<br>> > + for message, condition in pairs(conditions) do<br>> > + if condition then<br>> > + test:plan(1)<br>> > + test:skip(message)<br>> > + os.exit(test:check() and 0 or 1)<br>><br>> Does this mean that the subtest should be the last test to run in the<br>> file? It's kinda not obvious. Also, I suggest the following approach:<br>> Keep that condition search result, and skip every test (and subtest) if<br>> the condition met. We can still skip full test, like you done, if it has<br>> no parent test to run (has no test.parent).<br><br>It means that ya objebalsa one more time... Well, I have to declare that<br>this approach doesn't work for subtests. Hence, I suggest the following:<br>1. Proceed with this patchset for the root (i.e. parent) tests, since<br> this is the most popular way to write tests in Tarantool suite.<br>2. Think a little how to solve the problem for subtests. For now it's<br> too complex: plain Lua assertions, JIT engine tuning, etc --<br> everything need to be considered and carefully handled.<br><br>Anyway, to prevent skipcond misuse I added the next assertion:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua<br>index ac04c01d..f7be9c81 100644<br>--- a/test/tarantool-tests/tap.lua<br>+++ b/test/tarantool-tests/tap.lua<br>@@ -305,6 +305,7 @@ local function check(test)<br> end<br> <br> local function skipcond(test, conditions)<br>+ assert(test.parent ~= nil, 'FIXME: test:skipcond works only for parent tests')<br> for message, condition in pairs(conditions) do<br> if condition then<br> test:plan(1)<br><br>================================================================================<br><br>><br>> > + end<br>> > + end<br>> > + return test<br>> > +end<br>> > +<br>> > test_mt = {<br>> > __index = {<br>> > test = new,<br><br><snipped><br><br>> > --<br>> > 2.30.2<br>> ><br>><br>> --<br>> Best regards,<br>> Sergey Kaplun<br><br>--<br>Best regards,<br>IM</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>