Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for your review!

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
> 
> as is and change expectations for those who will look at your patch.

I believe your commit subject is too long, so I propose another
solution. Since the only thing affected by LUAJIT_TEST_VARDIR is
<utils.profilename>, so I can write something like "test: introduce
utils.profilename helper" and describe its rationale and functions
within the commit message. Are you OK with it?

> 
> 
> >
> > 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(-)
> >

<snipped>

> > @@ -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?

These are artefacts from not such elegant memprof tests...

> >   
> >     -- Collect all previous allocated objects.
> >     subtest:ok(free.INTERNAL.num == 22)

<snipped>

-- 
Best regards,
IM