Re: [Tarantool-patches] [PATCH v2 luajit 0/5] Adapt lua-Harness test suite

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

On 17.03.21, Sergey Kaplun wrote:
> Igor,
> 
> Thanks for the review!
> 
> On 17.03.21, Igor Munkin wrote:
> > Sergey,
> > 
> > Thanks for the series! I've looked onto the updated version on your
> > branch and it is almost cool! I've polished it a bit: commit message,
> > typos, etc -- you can see the changes on my branch[1] (CI is green[2]).
> > If you're OK with them, I'll push the changeset to the trunk.
> 
> LGTM.
> What's about Sergos'es naming proposal here [1]?

Discussed offline: Sergos is OK with the current solution.

> 
> > 
> > Speaking about the changes in Tarantool, I almost OK with them except
> > the test/luajit-test-init.lua. Consider the comments below.
> 
> Sorry, I don't get how exactly to consider them, so I answer your
> questions here. If it necessary I'll push them to the branch (yours or
> mine).

Good, thanks! Considering your answers, I've polished the commit in
Tarantool repo. See the patch below:

================================================================================

From 2fa0e10e45f799419c341befba745286273d0c08 Mon Sep 17 00:00:00 2001
Message-Id: <2fa0e10e45f799419c341befba745286273d0c08.1615976372.git.imun@tarantool.org>
From: Sergey Kaplun <skaplun@tarantool.org>
Date: Thu, 11 Mar 2021 12:00:44 +0300
Subject: [PATCH] luajit: bump new version

LuaJIT submodule is bumped to introduce the following changes:
* test: disable 411-luajit of lua-Harness suite
* test: disable 241-standalone of lua-Harness suite
* test: set USERNAME env var for lua-Harness suite
* test: adjust lua-Harness tests that use dofile
* test: adjust lua-Harness suite to CMake machinery
* test: add lua-Harness test suite

Within this changeset lua-Harness suite[1] is added to Tarantool
testing. Considering Tarantool specific changes in runtime the suite
itself is adjusted in LuaJIT submodule.

However, Tarantool provides and unconditionally loads TAP module
conflicting with the one used in the new suite. Hence, the Tarantool
built-in module is "unloaded" in test/luajit-test-init.lua.

Furthermore, Tarantool provides UTF-8 support via another built-in
module. Its interfaces differ from the ones implemented in Lua5.3 and
moonjit. At the same time our LuaJIT fork provides no UTF-8 support, so
lua-Harness UTF-8 detector is simply confused with non-nil utf8 global
variable. As a result, utf8 is set to nil in test/luajit-test-init.lua.

There are also some tests launching Lua interpreter, so strict need to
be disabled for their child tests too. Hence `strict.off()` is added to
`progname` (i.e. arg[-1] considering the way Tarantool parses its CLI
arguments)  command used in these tests.

[1]: https://framagit.org/fperrad/lua-Harness/tree/a74be27/test_lua

Closes #5844
Part of #4473

Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>
Reviewed-by: Igor Munkin <imun@tarantool.org>
---
 cmake/luajit.cmake        |  9 ++++++---
 test/luajit-test-init.lua | 22 ++++++++++++++++++++++
 third_party/luajit        |  2 +-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index 1c05e085b..3d37164e8 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -40,16 +40,19 @@ set(LUAJIT_TEST_BINARY $<TARGET_FILE:tarantool> CACHE STRING
 set(LUAJIT_USE_TEST OFF CACHE BOOL
     "Generate <test> target" FORCE)
 
-# Enable internal LuaJIT assertions for Tarantool Debug build.
 # XXX: There is <strict> module enabled by default in Tarantool
 # built in Debug, so we need to tweak LuaJIT testing environment.
+# XXX: Also, this script "unloads" internal Tarantool's modules
+# and remove globals conflicting with LuaJIT test suites.
+set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua"
+    CACHE STRING "Lua code need to be run before tests are started" FORCE)
+
+# Enable internal LuaJIT assertions for Tarantool Debug build.
 if(CMAKE_BUILD_TYPE STREQUAL "Debug")
     set(LUAJIT_USE_APICHECK ON CACHE BOOL
         "Assertions for the Lua/C API" FORCE)
     set(LUAJIT_USE_ASSERT ON CACHE BOOL
         "Assertions for the whole LuaJIT VM" FORCE)
-    set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua"
-        CACHE STRING "Lua code need to be run before tests are started" FORCE)
 endif()
 
 # Valgrind can be used only with the system allocator. For more
diff --git a/test/luajit-test-init.lua b/test/luajit-test-init.lua
index bc4af9748..6d9b058eb 100644
--- a/test/luajit-test-init.lua
+++ b/test/luajit-test-init.lua
@@ -1,2 +1,24 @@
 -- Disable strict for Tarantool.
 require("strict").off()
+
+-- XXX: lua-Harness test suite uses it's own tap.lua module
+-- that conflicts with the Tarantool's one.
+package.loaded.tap = nil
+-- XXX: lua-Harness test suite checks that utf8 module presents
+-- only in Lua5.3 or moonjit.
+utf8 = nil
+
+-- There are some tests launching Lua interpreter, so strict need
+-- to be disabled for the child tests too. Hence `strict.off()` is
+-- added to `progname` command used in these tests.
+-- Unlike LuaJIT, Tarantool doesn't store the given CLI flags in
+-- `arg`, so the table has the following layout:
+-- * arg[-1] -- the binary name
+-- * arg[0]  -- the script name
+-- * arg[N]  -- the script argument for all N in [1, #arg]
+-- The former one can be used to adjust the command to be used in
+-- child tests.
+-- XXX: Quotes types are important.
+-- XXX: luacheck thinks that `arg` is read-only global variable.
+-- luacheck: no global
+arg[-1] = arg[-1]..' -e "require[[strict]].off()"'
diff --git a/third_party/luajit b/third_party/luajit
index 75e6e90dd..eefe18886 160000
--- a/third_party/luajit
+++ b/third_party/luajit
@@ -1 +1 @@
-Subproject commit 75e6e90ddfbebbb5ff36d99afc1becf9e53249d6
+Subproject commit eefe18886813ab0872471588f02b277eb370d25d
-- 
2.25.0

================================================================================

Furthermore, I've squashed two last commits in LuaJIT repo into one.
Here is the patch:

================================================================================

From eefe18886813ab0872471588f02b277eb370d25d Mon Sep 17 00:00:00 2001
Message-Id: <eefe18886813ab0872471588f02b277eb370d25d.1615976764.git.imun@tarantool.org>
From: Sergey Kaplun <skaplun@tarantool.org>
Date: Tue, 2 Mar 2021 18:15:37 +0300
Subject: [PATCH] test: disable LuaJIT CLI tests in lua-Harness suite

This patch disables 241-standalone.t and 411-luajit.t from lua-Harness
test suite, because some flags in LuaJIT and Tarantool work differently,
or they are not supported in Tarantool at all. For example, -i, -b, -j.
See tarantool/tarantool#5541.

Resolves tarantool/tarantool#5844
Part of tarantool/tarantool#4473

Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>
Reviewed-by: Igor Munkin <imun@tarantool.org>
---
 .../{241-standalone.t => 241-standalone.t.disabled}               | 0
 test/lua-Harness-tests/{411-luajit.t => 411-luajit.t.disabled}    | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename test/lua-Harness-tests/{241-standalone.t => 241-standalone.t.disabled} (100%)
 rename test/lua-Harness-tests/{411-luajit.t => 411-luajit.t.disabled} (100%)

diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t.disabled
similarity index 100%
rename from test/lua-Harness-tests/241-standalone.t
rename to test/lua-Harness-tests/241-standalone.t.disabled
diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t.disabled
similarity index 100%
rename from test/lua-Harness-tests/411-luajit.t
rename to test/lua-Harness-tests/411-luajit.t.disabled
-- 
2.25.0


================================================================================

If you're OK with everything above, I'll push the series to the trunk.

> 

<snipped>

> > 
> > On 15.03.21, Sergey Kaplun wrote:
> > > In this patchset lua-Harness test suite is adapted for the LuaJIT fork
> > > and Tarantool.
> > > 
> > > Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-5844-adapt-lua-harness-test-suite
> > > Tarantool's branch for tests:
> > > https://github.com/tarantool/tarantool/tree/skaplun/gh-5844-adapt-lua-harness-test-suite
> > > Issues:
> > > * https://github.com/tarantool/tarantool/issues/5844
> > > * https://github.com/tarantool/tarantool/issues/5473
> > > 
> > > Changes in v2:
> > > * glanced commit message for the first patch
> > > * dropped module renaming
> > > * separate suite introduction and adjustment
> > > * dropped unnecessary commits with disabled tests
> > > 
> > > Mergen Imeev (1):
> > >   test: add lua-Harness test suite
> > > 
> > > Sergey Kaplun (4):
> > >   test: adjust lua-Harness suite for LuaJIT
> > >   test: adjust lua-Harness test suite for Tarantool
> > >   test: disable 241-standalone of lua-Harness suite
> > >   test: disable 411-luajit of lua-Harness suite
> > > 
> > 
> > <snipped>
> > 
> > > 
> > > -- 
> > > 2.28.0
> > 
> > [1]: https://github.com/tarantool/luajit/tree/imun/gh-5844-adapt-lua-harness-test-suite
> > [2]: https://github.com/tarantool/tarantool/tree/imun/gh-5844-adapt-lua-harness-test-suite
> > 
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-March/022733.html
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM