Re: [Tarantool-patches] [PATCH v2 06/10] test: support tarantool in lua-Harness

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

Max,

Thanks for the fixes! Just my two cents for the change below.

On 29.07.21, Максим Корякшин wrote:
> 
> Hi! Thanks for the review, Sergey!
> Here is the diff, which will add support of OOS build and will adjust
> comment in CMakeLists:
> ============================================================================================
> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
> index b9804033..90b4209a 100644
> --- a/test/lua-Harness-tests/CMakeLists.txt
> +++ b/test/lua-Harness-tests/CMakeLists.txt
> @@ -10,10 +10,10 @@ if(NOT PROVE)
>    return()
>  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\;${LUAJIT_SOURCE_DIR}/?.lua")
> +# Tests create temporary files (see 303-package.t and 411-luajit.t for
> +# example) to require. Also, they require some files from original test
> +# source directory.

We try to box our comments up to 66 symbols. This is not a strict rule
anymore, but if you can follow this, please do. Feel free to ignore.

> +set(LUA_PATH "./?.lua\;${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;${LUAJIT_SOURCE_DIR}/?.lua\;${CMAKE_BINARY_DIR}/src/?.lua")

There is LUAJIT_BINARY_DIR variable, so please use either both
LUAJIT_*_DIR or CMAKE_*_DIR/src for consistency. BTW, consider the
difference between CMAKE_*_DIR and PROJECT_*_DIR, so I'm sure you need
the latter one.

>  set(LUA_TEST_FLAGS --failures --shuffle)
>  if(CMAKE_VERBOSE_MAKEFILE)
> ============================================================================================
>  
>  
> > 

<snipped>

> > 

-- 
Best regards,
IM