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,

Thanks for the feedback!

On 16.03.21, Igor Munkin wrote:
> 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.

OK, looks like we came to agreement offline so I just repeate this here:

* All test chunks are `.PHONY`, so all tests will rerun always,
  even without test sources changing. That's why we need only build
  dependencies here.
* We are planing to make all test suites more consistent,
  so TEST_DEPS will be removed from Tarantool's suite too later.

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

Sorry, add corresponding comment:
===================================================================
diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
index a639553..336dc03 100644
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -15,6 +15,9 @@ if(CMAKE_VERBOSE_MAKEFILE)
   list(APPEND LUA_TEST_FLAGS --verbose)
 endif()
 
+# Tests create temporary files (see 303-package.t for example)
+# to require. Also, they require some files from original
+# test source directory.
 set(LUA_PATH "./?.lua\;${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;")
 
 add_custom_target(lua-Harness-tests DEPENDS ${LUAJIT_TEST_BINARY})
===================================================================

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

-- 
Best regards,
Sergey Kaplun