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,

Thanks for the fixes!

On 16.03.21, Sergey Kaplun wrote:
> Igor,
> 
> On 16.03.21, Igor Munkin wrote:
> > Sergey,
> > 

<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

Yeah, that's my fault.

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

To control the target dependencies: the binary and test chunks. However,
it was required when *-tests targets were not .PHONY, and now it's more
for self-check that nothing global is broken.

> 
> > 
> > > 
> > > > 
> > > > > +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\;")

Still not a word, why do you need both "./?.lua" and
"${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
> ===================================================================
> 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM