On 16 Mar 2021, at 15:02, Sergey Kaplun <skaplun@tarantool.org> wrote: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