Re: [Tarantool-patches] [WIP luajit 00/15] Adapt LuaVela test suites

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

Sergey,

On 10.03.21, Sergey Kaplun wrote:
> Igor,
> 
> On 10.03.21, Igor Munkin wrote:
> > Sergey,
> > 
> > I've precisely reviewed patches 04-09 and they look good. I've polished
> > a bit[1] the series, so it can be applied to the trunk out of the order.
> > CI[2] is green.
> 
> I hardly wait for the other series.
> I've assembled all issues related to PUC-Rio-Lua-5.1 test suite inside
> this [1] epic.
> 
> > 
> > BTW, I would like to leave some major points related to my review.
> > 
> 
> <snipped>
> 
> > 2. I've introduced a new CMake option: LUAJIT_TEST_INIT. You can read
> > the description here[3]. It allows to enclose the partial LUA_INIT
> > emulation, implemented via '-e dofile[[${LUAJIT_TEST_INIT}]]' and
> > obliges user to set the name of the Lua script to be run prior to the
> > testing suite[4]. By default it does nothing by reading from /dev/null.
> 
> This part is good. Why do you omit +slow flag for this test suite?

I'm too bad in copy-pasting... Thanks for catching! Added all necessary
flags, squashed, force-pushed to the branch. Diff is below.

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

diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
index d4886a4..9cd76ee 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -7,5 +7,6 @@ add_custom_command(TARGET LuaJIT-tests
   COMMENT "Running LuaJIT-tests"
   COMMAND
     ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
+    +slow +ffi +bit +jit
   WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
 )

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

> 
> > 
> > 3. Since CLI flags can be used for LuaJIT testing now, <utils.selfrun>
> > need to be adjusted[5] to respect the passed flags in child process.
> > 
> > This part LGTM, so if nobody is against the version I've checked to the
> > branch[1], then I apply it to the trunk after your and Sergos approval.
> 
> As I see on the branch there is no check of CMAKE_BUILD_TYPE variable.
> We need to disable strict only for debug build, IINM.
> Feel free to ignore.

Agree. BTW, it allows to check that Tarantool is not built with <strict>
enabled by default in Release. Fixed, squashed, force-pushed to the
branch. Diff is below.

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

diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
index e25eeb3dd..1c05e085b 100644
--- a/cmake/luajit.cmake
+++ b/cmake/luajit.cmake
@@ -37,17 +37,19 @@ set(LUAJIT_SMART_STRINGS ON CACHE BOOL
     "Harder string hashing function" FORCE)
 set(LUAJIT_TEST_BINARY $<TARGET_FILE:tarantool> CACHE STRING
     "Lua implementation to be used for tests (tarantool)" 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)
 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.
 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

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

> 
> Otherwise, LGTM.

Added your tags to all patches:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

Furthermore, I believe it is worth to mention your origin in
LUAJIT_TEST_INIT idea, so I've added you as a co-author to the last
patch on the branch.

> 
> > 

<snipped>

> > 
> > [1]: https://github.com/tarantool/luajit/tree/imun/tarantool-test
> > [2]: https://github.com/tarantool/tarantool/tree/imun/luajit-test
> > [3]: https://github.com/tarantool/luajit/commit/9712cc9#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR291-R307
> > [4]: https://github.com/tarantool/tarantool/commit/03b04a7
> > [5]: https://github.com/tarantool/luajit/commit/72f87b2
> > 
> > -- 
> > Best regards,
> > IM
> 
> [1]: https://github.com/tarantool/tarantool/issues/5870
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM