Hi, Igor! Thanks for the patch! Please consider my comments below.   >  >>This patch provides two enhancements for : >>1. As a result of the patch, 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. yields the test object, that allows to chain >>   skipcond call right next to the test constructor. >> >>Finally as a result of the aforementioned changes 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 > >>--- >> .../gh-4199-gc64-fuse.test.lua | 11 +++++---- >> .../gh-4427-ffi-sandwich.test.lua | 11 ++++----- >> .../gh-5813-resolving-of-c-symbols.test.lua | 23 ++++++++----------- >> ...4-add-proto-trace-sysprof-default.test.lua | 14 ++++------- >> .../lj-430-maxirconst.test.lua | 10 ++++---- >> .../lj-603-err-snap-restore.test.lua | 18 ++++++++------- >> ...lj-672-cdata-allocation-recording.test.lua | 12 +++++----- >> .../lj-906-fix-err-mem.test.lua | 12 +++++----- >> .../lj-flush-on-trace.test.lua | 11 ++++----- >> .../misclib-getmetrics-capi.test.lua | 17 ++++++-------- >> .../misclib-getmetrics-lapi.test.lua | 13 ++++------- >> .../misclib-memprof-lapi.test.lua | 22 +++++++++--------- >> .../misclib-sysprof-capi.test.lua | 18 ++++++--------- >> .../misclib-sysprof-lapi.test.lua | 17 +++++--------- >> test/tarantool-tests/tap.lua | 12 ++++++++++ >> test/tarantool-tests/utils.lua | 8 ------- >> 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 >>@@ -1,11 +1,12 @@ >>--- The test is GC64 only. >>-local ffi = require('ffi') >>-require('utils').skipcond(not ffi.abi('gc64'), 'test is GC64 only') >>- >> local tap = require('tap') >>-local test = tap.test('gh-4199-gc64-fuse') >>+local test = tap.test('gh-4199-gc64-fuse'):skipcond({ >>+ ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'), >>+}) >>+ >> test:plan(1) >>  >>+local ffi = require('ffi') >I don’t think that you needed to move that expression here, since >now you require the FFI module twice. >>+ >> collectgarbage() >> -- Chomp memory in currently allocated GC space. >> collectgarbage('stop') >>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 >>@@ -1,14 +1,11 @@ >>-local utils = require('utils') >>- >>--- Disabled on *BSD due to #4819. >>-utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819') >>- >> local tap = require('tap') >>+local test = tap.test('gh-4427-ffi-sandwich'):skipcond({ >>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>+}) >>  >>-local test = tap.test('gh-4427-ffi-sandwich') >> test:plan(2) >>  >>-local script = utils.makecmd(arg, { >>+local script = require('utils').makecmd(arg, { >>   -- TODO: Leave another toxic comment regarding SIP on macOS. >>   env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') }, >>   redirect = '2>&1', >>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 >>@@ -1,18 +1,15 @@ >>--- Memprof is implemented for x86 and x64 architectures only. >>-local utils = require("utils") >>- >>-utils.skipcond( >>- jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux", >>- jit.arch.." architecture or "..jit.os.. >>- " OS is NIY for memprof c symbols resolving" >>-) >>- >> local tap = require("tap") >>-local test = tap.test("gh-5813-resolving-of-c-symbols") >>+local test = tap.test("gh-5813-resolving-of-c-symbols"):skipcond({ >>+ ["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and jit.arch ~= "x64", >>+ ["Memprof is implemented for Linux only"] = jit.os ~= "Linux", >>+}) >>+ >> test:plan(5) >>  >>-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) >I think that change should be in commit «build: fix build with JIT disabled» >(a835fb0f778db6e9f0109a66ea1d2ac78fe682e4) >>  >> local bufread = require "utils.bufread" >> local symtab = require "utils.symtab" >>@@ -20,7 +17,7 @@ local testboth = require "resboth" >> local testhash = require "reshash" >> local testgnuhash = require "resgnuhash" >>  >>-local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin") >>+local TMP_BINFILE = require("utils").profilename("memprofdata.tmp.bin") >>  >> local function tree_contains(node, name) >>   if node == nil then >>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 >>@@ -1,13 +1,9 @@ >>--- Sysprof is implemented for x86 and x64 architectures only. >>-require('utils').skipcond( >>- jit.arch ~= 'x86' and jit.arch ~= 'x64' or jit.os ~= 'Linux' >>- or require('ffi').abi('gc64'), >>- jit.arch..' architecture or '..jit.os.. >>- ' OS is NIY for sysprof' >>-) >>- >> local tap = require('tap') >>-local test = tap.test('gh-7264-add-proto-trace-sysprof-default.test.lua') >>+local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({ >>+ ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and jit.arch ~= 'x64', >>+ ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux', >>+}) >>+ >> test:plan(2) >>  >> local chunk = [[ >>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 >>@@ -1,12 +1,12 @@ >>--- Disabled on *BSD due to #4819. >>-require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819') >>- >> local tap = require('tap') >>-local traceinfo = require('jit.util').traceinfo >>+local test = tap.test('lj-430-maxirconst'):skipcond({ >>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>+}) >>  >>-local test = tap.test('lj-430-maxirconst') >> test:plan(2) >>  >>+local traceinfo = require('jit.util').traceinfo >>+ >> -- This function has only 3 IR constant. >> local function irconst3() >> end >>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', >>+ -- 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: I’ve already mentioned it several times, but I’ll say this again: >that test is insanely fragile, and something needs to stop this nonsense. >>  >> local handler_is_called = false >> local recursive_f >>@@ -38,11 +44,7 @@ end >> recursive_f() >>  >> test:ok(true) >>--- Disabled on *BSD due to #4819. >>--- XXX: The different amount of stack slots is in-use for >>--- Tarantool at start, so just skip test for it. >>--- luacheck: no global >>-test:ok(jit.os == 'BSD' or _TARANTOOL or not handler_is_called) >>+test:ok(not handler_is_called) >>  >> -- XXX: Don't use `os.exit()` here by intention. When error on >> -- snap restoration is raised, `err_unwind()` doesn't stop on >>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 >>@@ -1,13 +1,13 @@ >>--- Disabled on *BSD due to #4819. >>-require('utils').skipcond(jit.os == 'BSD', 'Disabled due to #4819') >>+local tap = require('tap') >>+local test = tap.test('lj-672-cdata-allocation-recording'):skipcond({ >>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>+}) >>+ >>+test:plan(1) >>  >> local ffi = require('ffi') >> local traceinfo = require('jit.util').traceinfo >>  >>-local tap = require('tap') >>-local test = tap.test('lj-672-cdata-allocation-recording') >>-test:plan(1) >>- >> -- Structure with array. >> ffi.cdef('struct my_struct {int a; char d[8];}') >>  >>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 >>@@ -1,13 +1,13 @@ >> local tap = require('tap') >>-local ffi = require('ffi') >>-local table_new = require('table.new') >>- >>--- Avoid test to be killed. >>-require('utils').skipcond(ffi.abi('gc64'), 'test is not GC64 only') >>+local test = tap.test('lj-906-fix-err-mem'):skipcond({ >>+ ['Test requires GC64 mode disabled'] = require('ffi').abi('gc64'), >>+}) >>  >>-local test = tap.test('lj-906-fix-err-mem') >> test:plan(1) >>  >>+local ffi = require('ffi') >>+local table_new = require('table.new') >>+ >> local KB = 1024 >> local MB = 1024 * KB >> local sizes = {8 * MB, 8 * KB, 8} >>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 >>@@ -1,14 +1,11 @@ >>-local utils = require('utils') >>- >>--- Disabled on *BSD due to #4819. >>-utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819') >>- >> local tap = require('tap') >>+local test = tap.test('lj-flush-on-trace'):skipcond({ >>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>+}) >>  >>-local test = tap.test('lj-flush-on-trace') >> test:plan(2) >>  >>-local script = utils.makecmd(arg, { >>+local script = require('utils').makecmd(arg, { >>   -- TODO: Leave another toxic comment regarding SIP on macOS. >>   env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') }, >>   redirect = '2>&1', >>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 @@ >>-local utils = require('utils') >>+local tap = require('tap') >>+local test = tap.test("clib-misc-getmetrics"):skipcond({ >>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>+}) >>  >>--- Disabled on *BSD due to #4819. >>-utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819') >>+test:plan(11) >>  >> 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 jit_opt_default = { >>     3, -- level >>     "hotloop=56", >>@@ -14,11 +17,6 @@ local jit_opt_default = { >>     "minstitch=0", >> } >>  >>-local tap = require('tap') >>- >>-local test = tap.test("clib-misc-getmetrics") >>-test:plan(11) >>- >> local testgetmetrics = require("testgetmetrics") >>  >> test:ok(testgetmetrics.base()) >>@@ -96,8 +94,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(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 >>@@ -2,16 +2,14 @@ >> -- Major portions taken verbatim or adapted from the LuaVela testing suite. >> -- Copyright (C) 2015-2019 IPONWEB Ltd. >>  >>-local utils = require('utils') >>- >>--- Disabled on *BSD due to #4819. >>-utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819') >>- >> local tap = require('tap') >>+local test = tap.test("lib-misc-getmetrics"):skipcond({ >>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', >>+}) >>  >>-local test = tap.test("lib-misc-getmetrics") >> test:plan(10) >>  >>+local maxnins = require('utils').const.maxnins >> local jit_opt_default = { >>     3, -- level >>     "hotloop=56", >>@@ -281,8 +279,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(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-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 >>@@ -1,14 +1,14 @@ >>--- Memprof is implemented for x86 and x64 architectures only. >>-local utils = require("utils") >>- >>-utils.skipcond( >>- jit.arch ~= "x86" and jit.arch ~= "x64", >>- jit.arch.." architecture is NIY for memprof" >>-) >>- >>+-- XXX: This comment is a reminder to reimplement memprof tests >>+-- assertions to make them more indepentent to the changes made. >>+-- Now I just leave this 5 lines comment to preserve line numbers. >>+-- TODO: This line will be removed in the next patch. >>+-- TODO: This line will be removed in the next patch. >> local tap = require("tap") >>+local test = tap.test("misc-memprof-lapi"):skipcond({ >>+ ["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and >>+ jit.arch ~= "x64", >>+}) >>  >>-local test = tap.test("misc-memprof-lapi") >> test:plan(5) >>  >> local jit_opt_default = { >>@@ -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") >>+local BAD_PATH = require("utils").profilename("memprofdata/tmp.bin") >IMO, it’s better to require the «utils» module only once. >> 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 >>@@ -1,21 +1,17 @@ >>--- Sysprof is implemented for x86 and x64 architectures only. >>-local utils = require("utils") >>-utils.skipcond( >>- jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux", >>- jit.arch.." architecture or "..jit.os.. >>- " OS is NIY for sysprof" >>-) >>+local tap = require("tap") >>+local test = tap.test("clib-misc-sysprof"):skipcond({ >>+ ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and jit.arch ~= "x64", >>+ ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux", >>+}) >>+ >>+test:plan(2) >>  >> local testsysprof = require("testsysprof") >>  >>-local tap = require("tap") >> local jit = require('jit') >>  >> jit.off() >>  >>-local test = tap.test("clib-misc-sysprof") >>-test:plan(2) >>- >> test:ok(testsysprof.base()) >> test:ok(testsysprof.validation()) >>  >>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 >>@@ -1,14 +1,9 @@ >>--- Sysprof is implemented for x86 and x64 architectures only. >>-local utils = require("utils") >>-utils.skipcond( >>- jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux", >>- jit.arch.." architecture or "..jit.os.. >>- " OS is NIY for sysprof" >>-) >>- >> local tap = require("tap") >>+local test = tap.test("misc-sysprof-lapi"):skipcond({ >>+ ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and jit.arch ~= "x64", >>+ ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux", >>+}) >>  >>-local test = tap.test("misc-sysprof-lapi") >> test:plan(19) >>  >> jit.off() >>@@ -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") >Same as for memprof. >>  >> 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) >>+ 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 >>@@ -67,14 +67,6 @@ function M.makecmd(arg, opts) >>   }) >> end >>  >>-function M.skipcond(condition, message) >>- if not condition then return end >>- local test = tap.test(arg[0]:match('/?(.+)%.test%.lua')) >>- test:plan(1) >>- test:skip(message) >>- os.exit(test:check() and 0 or 1) >>-end >>- >> function M.hasbc(f, bytecode) >>   assert(type(f) == 'function', 'argument #1 should be a function') >>   assert(type(bytecode) == 'string', 'argument #2 should be a string') >>-- >>2.30.2 >-- >Best regards, >Maxim Kokryashkin >