[Tarantool-patches] [PATCH v2 luajit 3/5] test: adjust lua-Harness test suite for Tarantool
Sergey Ostanevich
sergos at tarantool.org
Tue Mar 16 17:51:08 MSK 2021
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 at 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 at tarantool.org <mailto:skaplun at 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 at tarantool.org <mailto:skaplun at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20210316/c10a97dc/attachment.htm>
More information about the Tarantool-patches
mailing list