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

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

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


Hi, Igor!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Понедельник, 27 февраля 2023, 12:10 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>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.
>
>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 | 19 ++++++-------
> ...4-add-proto-trace-sysprof-default.test.lua | 15 ++++------
> .../lj-430-maxirconst.test.lua | 10 +++----
> .../lj-603-err-snap-restore.test.lua | 19 ++++++++-----
> ...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 | 28 ++++++++-----------
> .../misclib-sysprof-capi.test.lua | 19 ++++++-------
> .../misclib-sysprof-lapi.test.lua | 19 ++++++-------
> test/tarantool-tests/tap.lua | 11 ++++++++
> test/tarantool-tests/utils.lua | 8 ------
> 16 files changed, 107 insertions(+), 128 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')
>+
> 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 06985dcd..ed3f50d1 100644
>--- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
>+++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
>@@ -1,16 +1,13 @@
>-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)
> 
> -- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
> -- with the given environment, launch options and CLI arguments.
>-local script = utils.makecmd(arg, {
>+local script = require('utils').makecmd(arg, {
>   -- XXX: Apple tries their best to "protect their users from
>   -- malware". As a result SIP (see the link[1] below) has been
>   -- designed and released. Now, Apple developers are so
>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..3c6833fc 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,14 +1,10 @@
>--- 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()
>@@ -19,8 +15,9 @@ local symtab = require "utils.symtab"
> local testboth = require "resboth"
> local testhash = require "reshash"
> local testgnuhash = require "resgnuhash"
>+local profilename = require("utils").profilename
> 
>-local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin")
>+local TMP_BINFILE = 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..472bc2d1 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,10 @@
>--- 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..be54a5f3 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,9 @@
> 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')
>+
> test:plan(2)
> 
> -- XXX: This is fragile. We need a specific amount of Lua stack
>@@ -38,11 +38,16 @@ 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: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: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 3351cc5a..099e9650 100644
>--- a/test/tarantool-tests/lj-flush-on-trace.test.lua
>+++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
>@@ -1,16 +1,13 @@
>-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)
> 
> -- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
> -- with the given environment, launch options and CLI arguments.
>-local script = utils.makecmd(arg, {
>+local script = require('utils').makecmd(arg, {
>   -- XXX: Apple tries their best to "protect their users from
>   -- malware". As a result SIP (see the link[1] below) has been
>   -- designed and released. Now, Apple developers are so
>diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-tests/misclib-getmetrics-capi.test.lua
>index 42a9adf9..c5a91955 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..e71bc239 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..18c8aaab 100644
>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>@@ -1,14 +1,13 @@
>--- 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 3 lines comment to preserve line numbers.
> local tap = require("tap")
>+local test = tap.test("misc-memprof-lapi"):skipcond({
>+ ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>+ ["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 = {
>@@ -27,9 +26,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 = utils.profilename("memprofdata.tmp.bin")
>-local BAD_PATH = 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()
>@@ -267,12 +267,6 @@ end)
> jit.on()
> 
> test:test("jit-output", function(subtest)
>- -- Disabled on *BSD due to #4819.
>- if jit.os == 'BSD' then
>- subtest:plan(1)
>- subtest:skip('Disabled due to #4819')
>- return
>- end
> 
>   subtest:plan(4)
> 
>diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
>index dad0fe4a..a9b712a5 100644
>--- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
>+++ b/test/tarantool-tests/misclib-sysprof-capi.test.lua
>@@ -1,21 +1,18 @@
>--- 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..fff89dfd 100644
>--- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
>+++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
>@@ -1,14 +1,10 @@
>--- 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()
>@@ -17,9 +13,10 @@ 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 = utils.profilename("sysprofdata.tmp.bin")
>-local BAD_PATH = 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)
>diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
>index 343f97e3..47a8fe87 100644
>--- a/test/tarantool-tests/tap.lua
>+++ b/test/tarantool-tests/tap.lua
>@@ -327,6 +327,16 @@ local function check(test)
>   return test.planned == test.total and test.failed == 0
> end
> 
>+local function skipcond(test, conditions)
>+ for reason, condition in pairs(conditions) do
>+ if condition then
>+ local skipfunc = test.planned and skiprest or skipall
>+ skipfunc(test, reason)
>+ end
>+ end
>+ return test
>+end
>+
> test_mt = {
>   __index = {
>     test = new,
>@@ -338,6 +348,7 @@ test_mt = {
>     skip = skip,
>     skipall = skipall,
>     skiprest = skiprest,
>+ skipcond = skipcond,
>     is = is,
>     isnt = isnt,
>     isnil = isnil,
>diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
>index  8355149 b..83716bb3 100644
>--- a/test/tarantool-tests/utils.lua
>+++ b/test/tarantool-tests/utils.lua
>@@ -81,14 +81,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
 

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