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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!

Thanks for the patch and fixes!

LGTM, except a few nits regarding to the commit message.

On 26.07.21, Максим Корякшин wrote:
> 
> Thanks for the review, Igor!
> Here is the new commit message, considering your comments:
> =================================================
>     test: support tarantool cli in lua-Harness
>  
>     The patch[1] from lua-Harness suite adjusts some checks in

Typo: s/from lua-Harness suite/the from lua-Harness suite/

>     lua-Harness tests, so they are compatible with Tarantool now.
>  
>     As a result, the introduced assertion for Tarantool error message fails
>     on MacOS, since getopt_long(3) yields the error message without single
>     quotes wrapping the flag. Hence, this commit also includes fix for that

Typo: s/fix/the fix/

>     from patch[2] in the trunk.

Typo: s/patch/the patch/

>  
>     [1]: https://framagit.org/fperrad/lua-Harness/-/commit/1da5b1b
>     [2]:  https://framagit.org/fperrad/lua-Harness/-/commit/a0532c5
>  
>     Part of tarantool/tarantool#5970
>     Part of tarantool/tarantool#4473
>  
> =================================================
>  
>   
> >Понедельник, 26 июля 2021, 0:37 +03:00 от Igor Munkin <imun@tarantool.org>:
> > 
> >Max,
> >
> >Thanks for the patch! The changes are fine, but please consider the
> >comments below regarding the commit message.
> >
> >On 20.07.21, Maxim Kokryashkin wrote:
> >> The patch[1] from lua-Harness suite adds some specific checks to
> >> lua-Harness tests, so they are compatible with Tarantool now.
> >
> >Strictly saying, no *new* checks are *added*, but the *existing* ones
> >are *adjusted* considering Tarantool specifics.
> >
> >>
> >> Considering this, commit d4e12d7ac28e3bc857d30971dd77deec66a67297('test:
> >> disable LuaJIT CLI tests in lua-Harness suite'), which disabled
> >> tarantool cli tests, can be superseded.
> >
> >No, it can't. The mentioned commit is superseded by "[PATCH v2 06/10]
> >test: support tarantool in lua-Harness" in this series.
> >
> >>
> >> As a result, the introduced assertion for Tarantool error message fails
> >> on MacOS, since getopt_long(3) yields the error message without single
> >> quotes wrapping the flag. Hence, this commit also includes fix for that
> >> from patch[2] in the trunk.
> >>
> >> [1]:  https://framagit.org/fperrad/lua-Harness/-/commit/1da5b1b3
> >
> >Minor: 7 digits are enough. This is not critical, but just odd to have
> >two similar links with the different number of digits, so feel free to
> >ignore this nit.
> >
> >> [2]:  https://framagit.org/fperrad/lua-Harness/-/commit/a0532c5
> >
> >Please mention the related issues: #5970 and #4473.
> >
> >> ---
> >> Additional information on issue with MacOS:
> >>  https://github.com/tarantool/tarantool/issues/5970#issuecomment-880158605
> >>
> >> NOTICE: tests for this separate commit are failing on FreeBSD
> >>
> >> test/lua-Harness-tests/241-standalone.t | 79 +++++++++++++++++++------
> >> 1 file changed, 61 insertions(+), 18 deletions(-)
> >>
> >
> ><snipped>
> >
> >> --
> >> 2.32.0
> >>
> >
> >--
> >Best regards,
> >IM
>  

-- 
Best regards,
Sergey Kaplun