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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 14002 bytes --]


Hi, Igor!
Thanks for the fixes!
I am still not satisfied with `snap-restore` case,
but whatever.
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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
> 

[-- Attachment #2: Type: text/html, Size: 16616 bytes --]