[Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable
Sergey Kaplun
skaplun at tarantool.org
Thu Aug 18 11:27:15 MSK 2022
Hi, Igor!
Thanks for the patch!
LGTM, after fixes for commit message mentioned by Sergey.
On 15.08.22, Sergey Bronnikov wrote:
> 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
Agree with Sergey here. This patch is more about prerequisites for this
variable.
>
> as is and change expectations for those who will look at your patch.
>
>
> >
> > Part of tarantool/tarantool#7472
> >
> > Signed-off-by: Igor Munkin <imun at 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?
Yes, this is nasty problems with the lines where allocation happens. As
far as we add new lines before tested function these numbers change.
> >
> > -- Collect all previous allocated objects.
> > subtest:ok(free.INTERNAL.num == 22)
<snipped>
> > return M
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list