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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi,

On 31.08.2022 17:53, Igor Munkin wrote:
> 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?

I'm ok!

<snipped>