From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Igor Munkin <imun@tarantool.org>, Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable Date: Mon, 15 Aug 2022 15:08:22 +0300 [thread overview] Message-ID: <d98192a9-1e10-1022-e8ec-9acdc1ff4cea@tarantool.org> (raw) In-Reply-To: <d21c760d38561c2b790d0de6d0bb765763f203f3.1660216002.git.imun@tarantool.org> Igor, thanks for the patch. See my comments inline. On 11.08.2022 14:17, Igor Munkin wrote: > Before the patch both memprof and sysprof artefacts are generated within > the binary artefacts tree (i.e. in the same directory LuaJIT binary is > generated). However, more convenient way is producing these temporary > profiles in a separate directory (e.g. located on the partition with > more strict space limits). As a result of the patch all memprof and > sysprof test chunks consider LUAJIT_TEST_VARDIR environment variable to > set the directory where test profiles are generated. For the case when > LUAJIT_TEST_VARDIR is not set, everything works as before. Commit message is inconsistent a bit with a patch itself. As far as I understand many hunks are not related to introducing LUAJIT_TEST_VARDIR. Probably it is better to change commit one-line message from "test: introduce LUAJIT_TEST_VARDIR variable" to something like "test: refactoring and introduce LUAJIT_TEST_VARDIR variable". It allows to keep patch as is and change expectations for those who will look at your patch. > > Part of tarantool/tarantool#7472 > > Signed-off-by: Igor Munkin <imun@tarantool.org> > --- > .../gh-5813-resolving-of-c-symbols.test.lua | 6 +++-- > .../misclib-memprof-lapi.test.lua | 22 ++++++++++--------- > .../misclib-sysprof-lapi.test.lua | 8 ++++--- > test/tarantool-tests/utils.lua | 12 ++++++++++ > 4 files changed, 33 insertions(+), 15 deletions(-) > > 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 d589dddf..e0b6d86d 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,5 +1,7 @@ > -- Memprof is implemented for x86 and x64 architectures only. > -require("utils").skipcond( > +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" > @@ -18,7 +20,7 @@ local testboth = require "resboth" > local testhash = require "reshash" > local testgnuhash = require "resgnuhash" > > -local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin") > +local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin") > > local function tree_contains(node, name) > if node == nil then > diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua > index a11f0be1..bae0c27c 100644 > --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua > @@ -1,5 +1,7 @@ > -- Memprof is implemented for x86 and x64 architectures only. > -require("utils").skipcond( > +local utils = require("utils") > + > +utils.skipcond( > jit.arch ~= "x86" and jit.arch ~= "x64", > jit.arch.." architecture is NIY for memprof" > ) > @@ -26,8 +28,8 @@ local memprof = require "memprof.parse" > local process = require "memprof.process" > local symtab = require "utils.symtab" > > -local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin") > -local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/memprofdata.tmp.bin") > +local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin") > +local BAD_PATH = utils.profilename("memprofdata/tmp.bin") > local SRC_PATH = "@"..arg[0] > > local function default_payload() > @@ -189,9 +191,9 @@ test:test("output", function(subtest) > -- one is the number of allocations. 1 event - alocation of > -- table by itself + 1 allocation of array part as far it is > -- bigger than LJ_MAX_COLOSIZE (16). > - subtest:ok(check_alloc_report(alloc, { line = 35, linedefined = 33 }, 2)) > + subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2)) > -- 20 strings allocations. > - subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 33 }, 20)) > + subtest:ok(check_alloc_report(alloc, { line = 42, linedefined = 35 }, 20)) What are these magic numbers mean and why we should change them for introducing LUAJIT_TEST_VARDIR? > > -- Collect all previous allocated objects. > subtest:ok(free.INTERNAL.num == 22) > @@ -199,8 +201,8 @@ test:test("output", function(subtest) > -- Tests for leak-only option. > -- See also https://github.com/tarantool/tarantool/issues/5812. > local heap_delta = process.form_heap_delta(events, symbols) > - local tab_alloc_stats = heap_delta[form_source_line(35)] > - local str_alloc_stats = heap_delta[form_source_line(40)] > + local tab_alloc_stats = heap_delta[form_source_line(37)] > + local str_alloc_stats = heap_delta[form_source_line(42)] > subtest:ok(tab_alloc_stats.nalloc == tab_alloc_stats.nfree) > subtest:ok(tab_alloc_stats.dbytes == 0) > subtest:ok(str_alloc_stats.nalloc == str_alloc_stats.nfree) > @@ -291,10 +293,10 @@ test:test("jit-output", function(subtest) > -- 10 allocations in interpreter mode, 1 allocation for a trace > -- recording and assembling and next 9 allocations will happen > -- while running the trace. > - subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 33 }, 11)) > - subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 38 }, 9)) > + subtest:ok(check_alloc_report(alloc, { line = 42, linedefined = 35 }, 11)) > + subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 40 }, 9)) > -- See same checks with jit.off(). > - subtest:ok(check_alloc_report(alloc, { line = 35, linedefined = 33 }, 2)) > + subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2)) > > -- Restore default JIT settings. > jit.opt.start(unpack(jit_opt_default)) > diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua > index 9e0a8a77..dbff41b0 100644 > --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua > +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua > @@ -1,6 +1,8 @@ > -- Sysprof is implemented for x86 and x64 architectures only. > local ffi = require("ffi") > -require("utils").skipcond( > +local utils = require("utils") > + > +utils.skipcond( > jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux" > or ffi.abi("gc64"), > jit.arch.." architecture or "..jit.os.. > @@ -19,8 +21,8 @@ local bufread = require("utils.bufread") > local symtab = require("utils.symtab") > local sysprof = require("sysprof.parse") > > -local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.sysprofdata.tmp.bin") > -local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/sysprofdata.tmp.bin") > +local TMP_BINFILE = utils.profilename("sysprofdata.tmp.bin") > +local BAD_PATH = utils.profilename("sysprofdata/tmp.bin") > > local function payload() > local function fib(n) > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua > index a1906498..87f7ff15 100644 > --- a/test/tarantool-tests/utils.lua > +++ b/test/tarantool-tests/utils.lua > @@ -123,4 +123,16 @@ function M.hasbc(f, bytecode) > return hasbc > end > > +function M.profilename(name) > + local vardir = os.getenv('LUAJIT_TEST_VARDIR') > + -- Replace pattern will change directory name of the generated > + -- profile to LUAJIT_TEST_VARDIR if it is set in the process > + -- environment. Otherwise, the original dirname is left intact. > + -- As a basename for this profile the test name is concatenated > + -- with the name given as an argument. > + local replacepattern = ('%s/%s-%s'):format(vardir or '%1', '%2', name) > + -- XXX: return only the resulting string. > + return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern)) > +end > + > return M
next prev parent reply other threads:[~2022-08-15 12:08 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-11 11:17 [Tarantool-patches] [PATCH luajit 0/8] LuaJIT tests and CI enhancements Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable Igor Munkin via Tarantool-patches 2022-08-15 12:08 ` Sergey Bronnikov via Tarantool-patches [this message] 2022-08-18 8:27 ` Sergey Kaplun via Tarantool-patches 2022-08-31 14:53 ` Igor Munkin via Tarantool-patches 2022-09-02 12:06 ` Sergey Bronnikov via Tarantool-patches 2022-10-05 19:51 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 2/8] test: introduce MakeLuaPath.cmake helper Igor Munkin via Tarantool-patches 2022-08-15 12:08 ` Sergey Bronnikov via Tarantool-patches 2022-08-31 15:07 ` Igor Munkin via Tarantool-patches 2022-09-02 12:09 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 9:37 ` Sergey Kaplun via Tarantool-patches 2022-08-31 15:19 ` Igor Munkin via Tarantool-patches 2022-09-01 10:16 ` Sergey Kaplun via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 3/8] test: fix tarantool suite for out of source build Igor Munkin via Tarantool-patches 2022-08-15 12:10 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 9:49 ` Sergey Kaplun via Tarantool-patches 2022-08-31 17:20 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 4/8] ci: use out of source build in GitHub Actions Igor Munkin via Tarantool-patches 2022-08-15 12:13 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 9:58 ` Sergey Kaplun via Tarantool-patches 2022-08-31 15:34 ` Igor Munkin via Tarantool-patches 2022-08-31 15:33 ` Igor Munkin via Tarantool-patches 2022-09-02 12:09 ` Sergey Bronnikov via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 5/8] ci: remove excess parallel level setup Igor Munkin via Tarantool-patches 2022-08-15 12:14 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:09 ` Sergey Kaplun via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 6/8] ci: remove arch prefix for macOS M1 workflow Igor Munkin via Tarantool-patches 2022-08-15 12:17 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:14 ` Sergey Kaplun via Tarantool-patches 2022-08-31 15:55 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 7/8] ci: merge x86_64 and ARM64 workflows Igor Munkin via Tarantool-patches 2022-08-15 12:22 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:21 ` Sergey Kaplun via Tarantool-patches 2022-08-31 16:02 ` Igor Munkin via Tarantool-patches 2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 8/8] ci: merge Linux and macOS workflows Igor Munkin via Tarantool-patches 2022-08-15 12:27 ` Sergey Bronnikov via Tarantool-patches 2022-08-18 10:32 ` Sergey Kaplun via Tarantool-patches 2022-11-11 8:56 ` [Tarantool-patches] [PATCH luajit 0/8] LuaJIT tests and CI enhancements Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d98192a9-1e10-1022-e8ec-9acdc1ff4cea@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=imun@tarantool.org \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox