From: Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 luajit 1/5] test: add lua-Harness test suite
Date: Tue, 16 Mar 2021 16:50:20 +0300 [thread overview]
Message-ID: <545E6B8A-6451-4E5C-8E53-D59781CA9D13@tarantool.org> (raw)
In-Reply-To: <20210316120233.GB1155@root>
[-- Attachment #1: Type: text/plain, Size: 6181 bytes --]
Hi!
Thanks for the patch!
LGTM.
Sergos
> 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
[-- Attachment #2: Type: text/html, Size: 32519 bytes --]
next prev parent reply other threads:[~2021-03-16 13:52 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-15 15:29 [Tarantool-patches] [PATCH v2 luajit 0/5] Adapt " Sergey Kaplun via Tarantool-patches
2021-03-15 15:29 ` [Tarantool-patches] [PATCH v2 luajit 1/5] test: add " Sergey Kaplun via Tarantool-patches
2021-03-15 17:37 ` Igor Munkin via Tarantool-patches
2021-03-15 18:10 ` Sergey Kaplun via Tarantool-patches
2021-03-15 21:06 ` Igor Munkin via Tarantool-patches
2021-03-16 9:38 ` Sergey Kaplun via Tarantool-patches
2021-03-16 11:08 ` Igor Munkin via Tarantool-patches
2021-03-16 12:02 ` Sergey Kaplun via Tarantool-patches
2021-03-16 13:50 ` Sergey Ostanevich via Tarantool-patches [this message]
2021-03-15 15:29 ` [Tarantool-patches] [PATCH v2 luajit 2/5] test: adjust lua-Harness suite for LuaJIT Sergey Kaplun via Tarantool-patches
2021-03-15 17:39 ` Igor Munkin via Tarantool-patches
2021-03-15 18:33 ` Sergey Kaplun via Tarantool-patches
2021-03-15 21:27 ` Igor Munkin via Tarantool-patches
2021-03-16 14:25 ` Sergey Ostanevich via Tarantool-patches
2021-03-15 15:29 ` [Tarantool-patches] [PATCH v2 luajit 3/5] test: adjust lua-Harness test suite for Tarantool Sergey Kaplun via Tarantool-patches
2021-03-15 17:44 ` Igor Munkin via Tarantool-patches
2021-03-16 6:01 ` Sergey Kaplun via Tarantool-patches
2021-03-16 10:51 ` Igor Munkin via Tarantool-patches
2021-03-16 14:51 ` Sergey Ostanevich via Tarantool-patches
2021-03-16 14:59 ` Sergey Kaplun via Tarantool-patches
2021-03-15 19:12 ` Igor Munkin via Tarantool-patches
2021-03-15 15:29 ` [Tarantool-patches] [PATCH v2 luajit 4/5] test: disable 241-standalone of lua-Harness suite Sergey Kaplun via Tarantool-patches
2021-03-16 14:51 ` Sergey Ostanevich via Tarantool-patches
2021-03-15 15:29 ` [Tarantool-patches] [PATCH v2 luajit 5/5] test: disable 411-luajit " Sergey Kaplun via Tarantool-patches
2021-03-16 14:52 ` Sergey Ostanevich via Tarantool-patches
2021-03-17 0:46 ` [Tarantool-patches] [PATCH v2 luajit 0/5] Adapt lua-Harness test suite Igor Munkin via Tarantool-patches
2021-03-17 7:32 ` Sergey Kaplun via Tarantool-patches
2021-03-17 10:27 ` Igor Munkin via Tarantool-patches
2021-03-17 10:31 ` Sergey Kaplun via Tarantool-patches
2021-03-17 16:49 ` Igor Munkin via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=545E6B8A-6451-4E5C-8E53-D59781CA9D13@tarantool.org \
--to=tarantool-patches@dev.tarantool.org \
--cc=sergos@tarantool.org \
--cc=skaplun@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v2 luajit 1/5] test: add lua-Harness test suite' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox