From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 7C47B2ADDF8; Mon, 27 Feb 2023 12:21:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7C47B2ADDF8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1677489680; bh=kQ4uZF1pV7SIuQ4WdkIcaKDMVDempGsDCDaQLzmvuoc=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=HZZzHIJAZjGDgiTb7qCeCeTfNT5RbTf6xNsZMMc6ZdjoRkuwNIh1F45OT6AHjzV5k ps/EDzcIJcC4Pg3IDcsGYgKHAxS2ySGwCfW78mNZja/wqdcoeUNa7TTWieXFtK+7Wc zBb+CTu7dhOJDTASmp2RWqKqjtvqV2cUtadOfck4= Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [95.163.41.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 90A1B21C253 for ; Mon, 27 Feb 2023 12:21:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 90A1B21C253 Received: by smtp52.i.mail.ru with esmtpa (envelope-from ) id 1pWZh8-002OnE-M4; Mon, 27 Feb 2023 12:21:19 +0300 Date: Mon, 27 Feb 2023 09:18:39 +0000 To: Sergey Kaplun Message-ID: References: <813a3dbf14ec517cbfe7313c77aba3adbd5113ff.1676304797.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9806C989EC2150E3306C371BD2459F3C96D6C2FD6C17EDBFE182A05F538085040CA9BC69C4C6BBB4499C09E6D8D59D121F03930778FD59A63617A87B2DD5C25AB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77545ECFDF1E157EBEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063797DAEDA043FC3DBE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F4E2BAF791FFCB30B7B41C04777649E4117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC206EB891B201A04DA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520437C869540D2AB0F2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE437C869540D2AB0F03F1AB874ED890284AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3768D4C1570C5AD76BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7D151390FFDBF6399731C566533BA786AA5CC5B56E945C8DA X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C1D376EF32BB0896A2258CFE3B24392AAFD9B7DC6092F84C6EDC49C42C767E762370E7372DEAF3411D7E09C32AA3244C885DDF1718CF47DBD4440A4C85C313D77101BF96129E4011927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojY+HhiuGRi+pBlBOVHBrmPw== X-Mailru-Sender: 2FEBA92C8E508479FE7B9A1DF348D53113A34B6339B44C80C2C3104D2156F11D010E252E6674B2002326FE6F2A341ACE0FB9F97486540B4CD9E8847AB8CFED4D9ABF8A61C016C2CFB0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/7] test: make skipcond helper more convenient X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 : > > 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 > > Signed-off-by: Igor Munkin > > --- > > 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 > > > > > > > -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" > > 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 > > 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 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 = { > > 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 > > > > > > > +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", > > > > > 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 > > > > > @@ -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() > > 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 > > > > > @@ -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, > > -- > > 2.30.2 > > > > -- > Best regards, > Sergey Kaplun -- Best regards, IM