Re: [Tarantool-patches] [PATCH v2 10/10] test: disable test/lua-Harness-tests/241-standalone.t on FreeBSD

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

Hi!
Thanks for the patch!

LGTM, except a few nits below.

On 26.07.21, Максим Корякшин wrote:
> 
> Sorry for the several emails, I have encountered some issues with the email client.
>  
> Anyway, here is the diff, that masks only one particular assertion instead of masking all of them:
> ========================================================================
> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t
> index d5373b9f..bdd95514 100755
> --- a/test/lua-Harness-tests/241-standalone.t
> +++ b/test/lua-Harness-tests/241-standalone.t
> @@ -51,10 +51,6 @@ if not pcall(io.popen, lua .. [[ -e "a=1"]]) then
>      skip_all "io.popen not supported"
>  end
> -if jit.os == 'BSD' then
> -    skip_all "BSD is not supported yet"
> -end
> -
>  plan'no_plan'
>  diag(lua)
> @@ -116,22 +112,24 @@ f = io.popen(cmd)
>  equals(f:read'*l', 'Hello World', "redirect")
>  f:close()
> -cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"
> -f = io.popen(cmd)
> -matches(f:read'*l', banner, "-i")
> -if ujit then
> -    matches(f:read'*l', '^JIT:')
> -end
> -if ravi then
> -    matches(f:read'*l', '^Copyright %(C%)')
> -    matches(f:read'*l', '^Portions Copyright %(C%)')
> -    matches(f:read'*l', '^Options')
> -end
> -if _TARANTOOL then
> -    matches(f:read'*l', "^type 'help' for interactive help")
> +if jit.os ~= 'BSD' then

Nit: Feel free to drop a comment here why this part is disabled for
FreeBSD (maybe with FIXME or XXX label).

> +    cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"
> +    f = io.popen(cmd)
> +    matches(f:read'*l', banner, "-i")
> +    if ujit then
> +        matches(f:read'*l', '^JIT:')
> +    end
> +    if ravi then
> +        matches(f:read'*l', '^Copyright %(C%)')
> +        matches(f:read'*l', '^Portions Copyright %(C%)')
> +        matches(f:read'*l', '^Options')
> +    end
> +    if _TARANTOOL then
> +        matches(f:read'*l', "^type 'help' for interactive help")
> +    end
> +    equals(f:read'*l', 'Hello World')
> +    f:close()
>  end
> -equals(f:read'*l', 'Hello World')
> -f:close()
>  cmd = lua .. [[ -e"a=1" -e "print(a)"]]
>  f = io.popen(cmd)
> ========================================================================
>  
> And here is the fixed commit message:
> ========================================================================
>     test: disable 241-standalone.t on FreeBSD

Nit: Strictly saying the patch disables only interactive mode part with
input from the file.

>  
>     Tarantool interactive mode misbehaviour has been found on
>     FreeBSD (for more info see tarantool/tarantool#6231). Hence, this[1]
>     particular assertion is masked for FreeBSD until the mentioned issue
>     resolves.
>  
>     [1]:  https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130
>  
>     Part of tarantool/tarantool#5970
>     Part of tarantool/tarantool#4473
>     Relates to tarantool/tarantool#6231
>  
> ========================================================================
>   
> >Понедельник, 26 июля 2021, 15:32 +03:00 от Максим Корякшин <m.kokryashkin@tarantool.org>:
> > 
> >diff:
> > 
> > 
> >> 
> >>>Max,
> >>>
> >>>Thanks for the patch! Please consider the comments below.
> >>>
> >>>At first, commit subject exceeds 50 symbols and the message exceeds 72
> >>>symbols. Please check this patch against our guidelines[1].
> >>>
> >>>On 20.07.21, Maxim Kokryashkin wrote:
> >>>> Tarantool interactive mode misbehaviour has been found on FreeBSD (for more info see #6231).
> >>>
> >>>The issue is mentioned the wrong way. The right format is
> >>>tarantool/tarantool#6231.
> >>>
> >>>> Hence, this particular assertion is masked for FreeBSD until the mentioned issue resolves.
> >>>
> >>>Strictly saying, you skipped all assertions (not only those using broken
> >>>interactive mode), not the particular one.
> >>>
> >>>>
> >>>> Resolves #5970
> >>>
> >>>This patch doesn't resolve the issue, since Tarantool profile introduced
> >>>in "[PATCH v2 06/10] test: support tarantool in lua-Harness" is not used
> >>>by Tarantool CI. Hence this is only "Part of" patch.
> >>>
> >>>> Part of #4473
> >>>
> >>>BTW, both issues are also mentioned the wrong way. The right format is
> >>>tarantool/tarantool#<issue-number> for both cases (see the previous
> >>>patches in the series for the examples).
> >>>
> >>>It's also worth to add "Relates to tarantool/tarantool#6231".
> >>>
> >>>> ---
> >>>> Additional info:  https://github.com/tarantool/tarantool/issues/5970#issuecomment-883253400
> >>>>
> >>>> test/lua-Harness-tests/241-standalone.t | 4 ++++
> >>>> 1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t
> >>>> index afbcf5b8..d5373b9f 100755
> >>>> --- a/test/lua-Harness-tests/241-standalone.t
> >>>> +++ b/test/lua-Harness-tests/241-standalone.t
> >>>> @@ -51,6 +51,10 @@ if not pcall(io.popen, lua .. [[ -e "a=1"]]) then
> >>>> skip_all "io.popen not supported"
> >>>> end
> >>>>
> >>>> +if jit.os == 'BSD' then
> >>>> + skip_all "BSD is not supported yet"
> >>>> +end
> >>>
> >>>At first, please add a comment with the rationale for this change.
> >>>Furthermore, it's better to skip only assertions using interactive mode
> >>>and run other assertions related to CLI behaviour.
> >>>
> >>>> +
> >>>> plan'no_plan'
> >>>> diag(lua)
> >>>>
> >>>> --
> >>>> 2.32.0
> >>>>
> >>>
> >>>[1]:  https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/
> >>>
> >>>--
> >>>Best regards,
> >>>IM
> >> 
> > 
>  

-- 
Best regards,
Sergey Kaplun