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 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! > >> > > > >> >> Patch for test directory tweak. Adjusted considering [2]. >> >> =================================================================== >> commit a84980334dce27243a25508e3bb8fd491689e552 >> Author: Sergey Kaplun > >> 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 , 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: > or . 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 >> > > > >> =================================================================== >> >> Patch for mocking environment variable in CMake. >> >> =================================================================== >> commit 483508b0a7863efabcde6d232ab9af2033e0011f >> Author: Sergey Kaplun > >> 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 , not examine of [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 , not examine of [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