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

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

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.

> 

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

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

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