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

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

Igor,

On 16.03.21, Igor Munkin wrote:
> Sergey,
> 
> On 15.03.21, Sergey Kaplun wrote:
> > Igor,
> > 
> > Thanks for the review!
> > 
> > On 15.03.21, Igor Munkin wrote:
> > > Sergey,
> > > 
> > > Thanks for the patch!
> > > 
> > > On 15.03.21, Sergey Kaplun wrote:
> 
> <snipped>
> 
> > > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > > > index 99471db..3a42f41 100644
> > > > --- a/test/CMakeLists.txt
> > > > +++ b/test/CMakeLists.txt
> > > > @@ -43,10 +43,12 @@ endif()
> > > >  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
> > > >  separate_arguments(LUAJIT_TEST_COMMAND)
> > > >  
> > > > -add_subdirectory(tarantool-tests)
> > > > +add_subdirectory(lua-Harness-tests)
> > > >  add_subdirectory(LuaJIT-tests)
> > > > +add_subdirectory(tarantool-tests)
> > > 
> > > Why did you choose this order?
> > 
> > Alphabetically, like in `ls` output. Feel free to propose your own.
> 
> To conform the alphabetical sort order LuaJIT-tests directory should go
> before lua-Harness-tests directory.

Agree. See the iteractive patch below. Branch is force-pushed.

===================================================================
diff --git a/.luacheckrc b/.luacheckrc
index a5f90b9..4e5dbdf 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -8,6 +8,6 @@ read_globals = { 'misc' }
 exclude_files = {
   'dynasm/',
   'src/',
-  'test/lua-Harness-tests/',
   'test/LuaJIT-tests/',
+  'test/lua-Harness-tests/',
 }
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 3a42f41..02fb2ed 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -43,13 +43,13 @@ endif()
 set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 separate_arguments(LUAJIT_TEST_COMMAND)
 
-add_subdirectory(lua-Harness-tests)
 add_subdirectory(LuaJIT-tests)
+add_subdirectory(lua-Harness-tests)
 add_subdirectory(tarantool-tests)
 
 add_custom_target(${PROJECT_NAME}-test DEPENDS
-  lua-Harness-tests
   LuaJIT-tests
+  lua-Harness-tests
   tarantool-tests
 )
 
===================================================================

> 
> > 
> 
> <snipped>
> 
> > > > diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
> > > > new file mode 100644
> > > > index 0000000..9b35e5a
> > > > --- /dev/null
> > > > +++ b/test/lua-Harness-tests/CMakeLists.txt
> > > > @@ -0,0 +1,49 @@
> > > > +# Test suite that has been added from lua-Harness test suite
> > > > +# in scope of https://github.com/tarantool/tarantool/issues/4473.
> > > > +
> > > > +# See the rationale in the root CMakeLists.txt
> > > > +cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
> > > > +
> > > > +find_program(PROVE prove)
> > > > +if(NOT PROVE)
> > > > +  message(WARNING "`prove' is not found, so lua-Harness-tests target is not generated")
> > > > +  return()
> > > > +endif()
> > > > +
> > > > +set(LUA_TEST_FLAGS --failures --shuffle)
> > > 
> > > Why did you drop TEST_DEPS variable containing the dependencies?
> > 
> > Sorry, don't get it. What do you mean?
> 
> If you look onto my iterative patch in the first review iteration,
> you'll find TEST_DEPS variable similar to the one in tarantool-tests
> CMakeLists.txt. I forgot to add it into CMakeLists.txt for LuaJIT-tests
> and remembered about it in this patch.

Sorry, I missed it while looks through your patch -- I supposed that
it does two things:
* removes tap renaming
* removes list of test usage, as far as 5040 is resolved

For now I don't understand, why do we need this TEST_DEPS?
What is its mission?

> 
> > 
> > > 
> > > > +if(CMAKE_VERBOSE_MAKEFILE)
> > > > +  list(APPEND LUA_TEST_FLAGS --verbose)
> > > > +endif()
> > > > +
> > > > +string(CONCAT LUA_PATH
> > > > +  "./?.lua\;"
> > > > +  "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;"
> > > > +  "${LUAJIT_SOURCE_DIR}/?.lua\;"
> > > > +)
> > > 
> > > There is not a word regarding such complex LUA_PATH configuration.
> > 
> > Ok, it can be the one long string, but, in my opinion, this format is
> > more readable, plus, as a bonus, with it line length is less than 120
> > characters. I'll join these lines into one if you insist.
> 
> This is not the issue (and might be much better that my approach in
> tarantool-tests and I will patch the test runners to make it conform to
> one style later). I'm talking about the purpose for each entry. E.g. why
> do you need both "./?.lua" and "${CMAKE_CURRENT_SOURCE_DIR}/?.lua"?  If
> you need "${LUAJIT_SOURCE_DIR}/?.lua" for jit.* modules, then why it's
> not required for other suites? Mention this explicitly, please.

Got it now, thanks!
Looks like it really is not necessary. See the iterative patch below:
Checked result in CI -- it is green, except known freebsd issue.

===================================================================
diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
index b844788..fd5c8ce 100644
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -15,16 +15,7 @@ if(CMAKE_VERBOSE_MAKEFILE)
   list(APPEND LUA_TEST_FLAGS --verbose)
 endif()
 
-string(CONCAT LUA_PATH
-  "./?.lua\;"
-  "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;"
-  "${LUAJIT_SOURCE_DIR}/?.lua\;"
-)
-
-string(CONCAT LUA_CPATH
-  "./?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
-  "${LUAJIT_SOURCE_DIR}/?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
-)
+set(LUA_PATH "./?.lua\;${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;")
 
 add_custom_target(lua-Harness-tests DEPENDS ${LUAJIT_TEST_BINARY})
 
@@ -33,7 +24,6 @@ add_custom_command(TARGET lua-Harness-tests
   COMMAND
   env
     LUA_PATH="${LUA_PATH}\;"
-    LUA_CPATH="${LUA_CPATH}\;"
     # XXX: 309-os.t checks os.getenv() function by examining of
     # USERNAME or LOGNAME environment variable.
     # These variables might not be set in the environment, so
===================================================================

> 
> > 
> > > 
> > > > +
> > > > +string(CONCAT LUA_CPATH
> > > > +  "./?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
> > > > +  "${LUAJIT_SOURCE_DIR}/?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
> > > > +)
> > > 
> > > Ditto.
> > > 
> 
> <snipped>
> 
> > > > -- 
> > > > 2.28.0
> > > > 
> > > 
> > > -- 
> > > Best regards,
> > > IM
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun