Re: [Tarantool-patches] [PATCH v2 luajit 3/5] test: adjust lua-Harness test suite for Tarantool

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

[-- Attachment #1: Type: text/plain, Size: 8888 bytes --]

Hi!

Thanks for patch and review!

With all comments below fixed the patch is LGTM.

My proposal on dofile_fullpath() naming:
- split the functions, it’ll ease reading. Keep dofile() in the place of use, 
  wrap the name with additional function
- pick a name that echo what function does: delivers a filename relative to the
  test itself e.g. relative_to_test()
This results in

    dofile(relative_to_test('lexico53/boolean.t’))

regards,
Sergos


> On 16 Mar 2021, at 13:51, Igor Munkin <imun@tarantool.org> wrote:
> 
> Sergey,
>  
> On 16.03.21, Sergey Kaplun wrote:
>> Igor,
>> 
>> Thanks for the review!
>> 
>> On 15.03.21, Igor Munkin wrote:
>>> Sergey,
>>> 
>>> This is ridiculous: you split two similar renames required by the one
>>> issue into *two* separate commits, but leaving the changes *totally
>>> unrelated to each other* within a single commit. Please, split this
>>> patch into two: one for the test directory tweak and another with
>>> mocking environment variable in CMake.
>> 
>> Sorry, but why then you said nothing about commits separation/joining
>> for this [1] draft series, as I asked for?
> 
> I mentioned it here[1] and you decided to left everything intact (that's
> OK for me). This commit is a new one and you created it from two
> separate commits in the previous version. So it's just a new feedback
> for the new changes.
> 
>> 
>> Offline we came to the agreement that we should use 3 commits for each
>> test suite. The first to onboard it, the second to adjust for LuaJIT,
>> the third to adjust it for Tarantool.
> 
> I remember only 3 bullets on which we have explicitly agreed:
> * The suite should be taken intact in the first commit despite the fact
>  tests can be broken.
> * LuaJIT related changes should be separated from Tarantool related
>  ones to ease their further maintenance. Furthermore, every change
>  should be mentioned in our GitHub queue.
> * All tests should be run in a unified way via both LuaJIT and Tarantool
>  testing machinery. It means test chunk or test case is either run or
>  not despite the testing environment.
> 
> Regarding everything else I was open to discuss it on review, since it's
> too hard (at least for me) to consider all issues faced while adopting
> these suites and create the most unified way to handle them. I believe,
> this is the purpose of review process: to see what is done and think
> whether it can be done in a better way (better doesn't mean *you* made
> it wrong, but *we* chose a bad solution).
> 
>> Later your asked me to send Mergens changes as is.
>> 
>> I specially send draft to discuss the commits content, order and so on.
>> You said only about separating patchset for different test suites and
>> merged LuaJIT test suite, so I thought that commit order is OK for you.
>> I'd merged changes with Tarantool-related one.
> 
> I hope you remember that I've asked a couple of times to polish and send
> LuaJIT-test-cleanup patchset. The last time I was asking was right
> before you sent WIP series. LuaJIT related patches per se were ready to
> be merged; the only issue was disabling strict for it. Additionally I've
> adjusted tarantool-tests runner to use LUAJIT_TEST_COMMAND to make all
> tests be run by unified way.
> 
> Then I asked you to split two remaining suites into two series so I can
> look on them separately. Hence, I was OK with the general approach, but
> was afraid to miss some details.
> 
>> 
>> If it is not comfortable for you getting WIP series and discussing them
>> let's decline this practise.
> 
> I'm totally fine with WIP series: e.g. LUAJIT_TEST_INIT has been done
> after I've glanced all the patches with tests you've sent.
> 
>> 
>> Back to business, I've split the patch into two, considering your
>> proposal, see them below (their order is the same).
> 
> Nice, thanks a lot!
> 
>> 
> 
> <snipped>
> 
>> 
>> Patch for test directory tweak. Adjusted considering [2].
>> 
>> ===================================================================
>> commit a84980334dce27243a25508e3bb8fd491689e552
>> Author: Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>>
>> Date:   Mon Mar 15 16:24:07 2021 +0300
>> 
>>    test: adjust lua-Harness tests that using dofile
>> 
>>    This patch makes out-of-source execution lua-Harness suite tests that
>>    using `dofile()` correct.
> 
> Minor: There are other tests using <dofile>, but they are fine. The root
> problem relates to the approach used in the patched tests: the .t files
> are kinda wrapper, containing the basic checks (or not, e.g. UTF-8 test
> chunk), and some specific checks are moved to the auxiliary chunks
> "dofiled" in .t script.
> 
>> 
>>    There are the following files that used `dofile()` function
>>    on file in test sources directory:
>>    * 101-boolean.t
>>    * 102-function.t
>>    * 103-nil.t
>>    * 104-number.t
>>    * 105-string.t
>>    * 106-table.t
>>    * 107-thread.t
>>    * 108-userdata.t
>>    * 203-lexico.t
>>    * 231-metatable.t
>>    * 301-basic.t
>>    * 305-utf8.t
>>    * 404-ext.t
>> 
>>    `dofile()` looks for files to execute in the current working directory,
>>    that might be not the same as the test source directory.
>> 
>>    This patch introduces the new function `dofile_fullpath()` that
> 
> What does "fullpath" mean? If this is absolute path, then naming is
> ambiguous; and I guess nothing stops one from using relative paths (but
> not symlink, I think) here. Furthermore, the naming doesn't represent
> the purpose: as I mentioned above this dofile is needed to implement
> specific checks, so you can adjust the naming to more verbose one:
> <test_more> or <make_specific_checks>. Thoughts?
> 
>>    evaluates full path to file considering arg[0] (i.e. test filename)
> 
> It's worth to check and mention the caveat with symlinks. If the issue
> exists, then the paths should be resolved in CMake the way similar to
> the one used for luacheck target.
> 
>>    value.
>> 
>>    Part of tarantool/tarantool#5844
>>    Part of tarantool/tarantool#4473
>> 
> 
> <snipped>
> 
>> ===================================================================
>> 
>> Patch for mocking environment variable in CMake.
>> 
>> ===================================================================
>> commit 483508b0a7863efabcde6d232ab9af2033e0011f
>> Author: Sergey Kaplun <skaplun@tarantool.org <mailto:skaplun@tarantool.org>>
>> Date:   Mon Mar 15 22:08:07 2021 +0300
>> 
>>    test: forcify set USERNAME env var for lua-Harness
> 
> Strictly saying, you do not force, but just set. Furthermore, I doubt
> about word formation you applied to "force".
> 
>> 
>>    309-os.t checks `os.getenv()` function by examining of
> 
> Typo: examine <smth>, not examine of <smth>[2].
> 
>>    USERNAME or LOGNAME environment variable.
>>    These variables might not be set in the environment, that leads to test
>>    failure.
>> 
>>    This patchs sets manually USERNAME environment variable for
> 
> Typo: s/manually/explicitly/.
> 
>>    lua-Harness-tests target.
>> 
>>    Part of tarantool/tarantool#5844
>>    Part of tarantool/tarantool#4473
>> 
>> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
>> index f8611ce..b844788 100644
>> --- a/test/lua-Harness-tests/CMakeLists.txt
>> +++ b/test/lua-Harness-tests/CMakeLists.txt
>> @@ -34,6 +34,11 @@ add_custom_command(TARGET lua-Harness-tests
>>   env
>>     LUA_PATH="${LUA_PATH}\;"
>>     LUA_CPATH="${LUA_CPATH}\;"
>> +    # XXX: 309-os.t checks os.getenv() function by examining of
> 
> Typo: examine <smth>, not examine of <smth>[2].
> 
>> +    # USERNAME or LOGNAME environment variable.
>> +    # These variables might not be set in the environment, so
>> +    # set one of them manually.
>> +    USERNAME="fperrad"
>>     ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
>>       --exec '${LUAJIT_TEST_COMMAND} -l profile_luajit21'
>>       ${LUA_TEST_FLAGS}
>> ===================================================================
>> 
>> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-March/022563.html
>> [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-March/022710.html
>> 
>> -- 
>> Best regards,
>> Sergey Kaplun
> 
> [1]: https://lists.tarantool.org/tarantool-patches/16E39C08-397B-4A38-953A-B9EB85CD9C8B@tarantool.org/T/#m9de07af72e06ac22b7540741b9e1c4ac485688bb <https://lists.tarantool.org/tarantool-patches/16E39C08-397B-4A38-953A-B9EB85CD9C8B@tarantool.org/T/#m9de07af72e06ac22b7540741b9e1c4ac485688bb>
> [2]: https://dictionary.cambridge.org/dictionary/english/examine <https://dictionary.cambridge.org/dictionary/english/examine>
> 
> -- 
> Best regards,
> IM


[-- Attachment #2: Type: text/html, Size: 72450 bytes --]