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

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

Igor,

On 16.03.21, Igor Munkin wrote:
> Sergey,
> 
> On 16.03.21, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review!
> > 
> > On 15.03.21, Igor Munkin wrote:
> > > Sergey,
> > > 

OK, thanks for clarification!

<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 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.

Reformulate like the following:

| This patch makes out-of-source execution lua-Harness suite tests that
| using `dofile()` for specific checks mentioned in corresponding
| *.t files correct.

> 
> > 
> >     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?

`make_specific_checks()` is good for me. See the iterative patch below:

===================================================================
diff --git a/test/lua-Harness-tests/101-boolean.t b/test/lua-Harness-tests/101-boolean.t
index b9a769f..c26b276 100755
--- a/test/lua-Harness-tests/101-boolean.t
+++ b/test/lua-Harness-tests/101-boolean.t
@@ -114,7 +114,7 @@ error_like(function () local a = true; a[1] = 1; end,
            "index")
 
 if has_op53 then
-    dofile_fullpath'lexico53/boolean.t'
+    make_specific_checks'lexico53/boolean.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/102-function.t b/test/lua-Harness-tests/102-function.t
index 2858640..c49afc5 100755
--- a/test/lua-Harness-tests/102-function.t
+++ b/test/lua-Harness-tests/102-function.t
@@ -193,7 +193,7 @@ t[print] = true
 ok(t[print])
 
 if has_op53 then
-    dofile_fullpath'lexico53/function.t'
+    make_specific_checks'lexico53/function.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/103-nil.t b/test/lua-Harness-tests/103-nil.t
index 1e7c134..87a1c3b 100755
--- a/test/lua-Harness-tests/103-nil.t
+++ b/test/lua-Harness-tests/103-nil.t
@@ -114,7 +114,7 @@ error_like(function () local a = nil; a[1] = 1; end,
            "index")
 
 if has_op53 then
-    dofile_fullpath'lexico53/nil.t'
+    make_specific_checks'lexico53/nil.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/104-number.t b/test/lua-Harness-tests/104-number.t
index affd1a4..f5b81e3 100755
--- a/test/lua-Harness-tests/104-number.t
+++ b/test/lua-Harness-tests/104-number.t
@@ -233,7 +233,7 @@ error_like(function () local a = 3.14; a[1] = 1; end,
            "index")
 
 if has_op53 then
-    dofile_fullpath'lexico53/number.t'
+    make_specific_checks'lexico53/number.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/105-string.t b/test/lua-Harness-tests/105-string.t
index f571520..184deab 100755
--- a/test/lua-Harness-tests/105-string.t
+++ b/test/lua-Harness-tests/105-string.t
@@ -264,7 +264,7 @@ error_like(function () a = 'text'; a[1] = 1; end,
            "index")
 
 if has_op53 then
-    dofile_fullpath'lexico53/string.t'
+    make_specific_checks'lexico53/string.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/106-table.t b/test/lua-Harness-tests/106-table.t
index b1a1027..667b9c8 100755
--- a/test/lua-Harness-tests/106-table.t
+++ b/test/lua-Harness-tests/106-table.t
@@ -122,7 +122,7 @@ error_like(function () t = {}; t[0/0] = 42 end,
            "table index is NaN")
 
 if has_op53 then
-    dofile_fullpath'lexico53/table.t'
+    make_specific_checks'lexico53/table.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/107-thread.t b/test/lua-Harness-tests/107-thread.t
index 2f332d7..5c5bf22 100755
--- a/test/lua-Harness-tests/107-thread.t
+++ b/test/lua-Harness-tests/107-thread.t
@@ -122,7 +122,7 @@ t[co] = true
 ok(t[co])
 
 if has_op53 then
-    dofile_fullpath'lexico53/thread.t'
+    make_specific_checks'lexico53/thread.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/108-userdata.t b/test/lua-Harness-tests/108-userdata.t
index cc4134b..48a5f60 100755
--- a/test/lua-Harness-tests/108-userdata.t
+++ b/test/lua-Harness-tests/108-userdata.t
@@ -119,7 +119,7 @@ t[u] = true
 ok(t[u])
 
 if has_op53 then
-    dofile_fullpath'lexico53/userdata.t'
+    make_specific_checks'lexico53/userdata.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/203-lexico.t b/test/lua-Harness-tests/203-lexico.t
index e5e89ed..a4f06a3 100755
--- a/test/lua-Harness-tests/203-lexico.t
+++ b/test/lua-Harness-tests/203-lexico.t
@@ -118,19 +118,19 @@ do
 end
 
 if _VERSION >= 'Lua 5.2' or jit then
-    dofile_fullpath('lexico52/lexico.t')
+    make_specific_checks('lexico52/lexico.t')
 end
 
 if _VERSION >= 'Lua 5.3' or luajit21 then
-    dofile_fullpath('lexico53/lexico.t')
+    make_specific_checks('lexico53/lexico.t')
 end
 
 if _VERSION >= 'Lua 5.4' then
-    dofile_fullpath('lexico54/lexico.t')
+    make_specific_checks('lexico54/lexico.t')
 end
 
 if jit and pcall(require, 'ffi') then
-    dofile_fullpath('lexicojit/lexico.t')
+    make_specific_checks('lexicojit/lexico.t')
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/231-metatable.t b/test/lua-Harness-tests/231-metatable.t
index c5684d2..97ac542 100755
--- a/test/lua-Harness-tests/231-metatable.t
+++ b/test/lua-Harness-tests/231-metatable.t
@@ -589,7 +589,7 @@ do
 end
 
 if has_anno_toclose then
-    dofile_fullpath'lexico54/metatable.t'
+    make_specific_checks'lexico54/metatable.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/301-basic.t b/test/lua-Harness-tests/301-basic.t
index 460a02f..f92d744 100755
--- a/test/lua-Harness-tests/301-basic.t
+++ b/test/lua-Harness-tests/301-basic.t
@@ -843,7 +843,7 @@ do -- xpcall
 end
 
 if jit and pcall(require, 'ffi') then
-    dofile_fullpath('lexicojit/basic.t')
+    make_specific_checks('lexicojit/basic.t')
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/305-utf8.t b/test/lua-Harness-tests/305-utf8.t
index 18c57d8..6c12538 100755
--- a/test/lua-Harness-tests/305-utf8.t
+++ b/test/lua-Harness-tests/305-utf8.t
@@ -40,9 +40,9 @@ if not utf8 then
     nok(has_utf8, "no has_utf8")
 else
     plan'no_plan'
-    dofile_fullpath'lexico53/utf8.t'
+    make_specific_checks'lexico53/utf8.t'
     if _VERSION >= 'Lua 5.4' then
-        dofile_fullpath'lexico54/utf8.t'
+        make_specific_checks'lexico54/utf8.t'
     end
     done_testing()
 end
diff --git a/test/lua-Harness-tests/404-ext.t b/test/lua-Harness-tests/404-ext.t
index e48d91a..a799c75 100755
--- a/test/lua-Harness-tests/404-ext.t
+++ b/test/lua-Harness-tests/404-ext.t
@@ -158,7 +158,7 @@ end
 
 -- thread.exdata
 if pcall(require, 'ffi') and (profile.openresty or jit.version:match'moonjit') then
-    dofile_fullpath'lexicojit/ext.t'
+    make_specific_checks'lexicojit/ext.t'
 end
 
 done_testing()
diff --git a/test/lua-Harness-tests/tap.lua b/test/lua-Harness-tests/tap.lua
index 37b9df7..a8454ae 100644
--- a/test/lua-Harness-tests/tap.lua
+++ b/test/lua-Harness-tests/tap.lua
@@ -209,7 +209,7 @@ end
 -- relative paths below become invalid. Hence, we need to
 -- prepend the directory from the script (i.e. test) name to
 -- the auxiliary files to be loaded and executed.
-function dofile_fullpath (filename)
+function make_specific_checks (filename)
     return dofile(arg[0]:gsub('([^/]+)%.t$', '') .. filename)
 end
 
===================================================================

> 
> >     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.

Looks like all is fine:

| $ pwd
| /home/burii/builds_workspace/luajit/gh-5844-adapt-lua-harness-test-suite
| $ rm -rf third_party/luajit/
| $ cd third_party/
| $ ln -s $HOME/builds_workspace/luajit/gh-5844-adapt-lua-harness-test-suite/ ~/builds_workspace/tarantool/gh-5844-adapt-lua-harness-test-suite/third_party/luajit
| $ cd ../build/
| $ cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_BACKTRACE=ON .. && make -j && make LuaJIT-test
| <snipped>
| All tests successful.

> 
> >     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 doubt
> about word formation you applied to "force".

Removed confusing "forcify". See the whole patch below.

> 
> > 
> >     309-os.t checks `os.getenv()` function by examining of
> 
> Typo: examine <smth>, not examine of <smth>[2].

Fixed.

> 
> >     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/.

Fixed.

> 
> >     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].

Fixed. See the full commit below:

===================================================================
commit 6f42397be1bd1a4d96a639d92da658180f270091 (HEAD)
Author: Sergey Kaplun <skaplun@tarantool.org>
Date:   Mon Mar 15 22:08:07 2021 +0300

    test: set USERNAME env var for lua-Harness

    309-os.t checks `os.getenv()` function by examining
    USERNAME or LOGNAME environment variable.
    These variables might not be set in the environment, that leads to test
    failure.

    This patchs sets explicitly USERNAME environment variable for
    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 336dc03..1061c79 100644
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -27,6 +27,11 @@ add_custom_command(TARGET lua-Harness-tests
   COMMAND
   env
     LUA_PATH="${LUA_PATH}\;"
+    # XXX: 309-os.t checks os.getenv() function by examining
+    # 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}
===================================================================

> 
> > +    # 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

-- 
Best regards,
Sergey Kaplun