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

Tue Mar 16 13:51:42 MSK 2021

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>
> 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>
> 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
[2]: https://dictionary.cambridge.org/dictionary/english/examine

-- 
Best regards,
IM