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
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'sOK for me). This commit is a new one and you created it from twoseparate commits in the previous version. So it's just a new feedbackfor 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'stoo hard (at least for me) to consider all issues faced while adoptingthese 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 thinkwhether it can be done in a better way (better doesn't mean *you* madeit 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 sendLuaJIT-test-cleanup patchset. The last time I was asking was rightbefore you sent WIP series. LuaJIT related patches per se were ready tobe merged; the only issue was disabling strict for it. Additionally I'veadjusted tarantool-tests runner to use LUAJIT_TEST_COMMAND to make alltests be run by unified way.Then I asked you to split two remaining suites into two series so I canlook on them separately. Hence, I was OK with the general approach, butwas 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 doneafter 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>
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 rootproblem relates to the approach used in the patched tests: the .t filesare kinda wrapper, containing the basic checks (or not, e.g. UTF-8 testchunk), 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 isambiguous; and I guess nothing stops one from using relative paths (butnot symlink, I think) here. Furthermore, the naming doesn't representthe purpose: as I mentioned above this dofile is needed to implementspecific 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 issueexists, then the paths should be resolved in CMake the way similar tothe 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>
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 doubtabout 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[2]: https://dictionary.cambridge.org/dictionary/english/examine-- Best regards,IM