[Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable
Igor Munkin
imun at tarantool.org
Wed Aug 31 17:53:24 MSK 2022
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 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(-)
> >
<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
More information about the Tarantool-patches
mailing list