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 fixes!

LGTM, except a few nits below.
On 29.07.21, Максим Корякшин wrote:
> 
> Hi! Thanks for the review, Sergey!
> Here is the diff, which adds a comment explaining the reason why the
> discussed assertion was masked:
> ======================================================================
> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t
> index bdd95514..742765d4 100755
> --- a/test/lua-Harness-tests/241-standalone.t
> +++ b/test/lua-Harness-tests/241-standalone.t
> @@ -112,6 +112,8 @@ f = io.popen(cmd)
>  equals(f:read'*l', 'Hello World', "redirect")
>  f:close()
> +-- FIXME Tarantool interactive mode misbehaviour has been found on

Typo: s/FIXME/FIXME:/

Nit: Also I suppose, that "has been found" can be dropped.
Feel free to ignore.

> +-- FreeBSD (for more info see #6231).

Typo: s/for more info see/for more info, see/

>  if jit.os ~= 'BSD' then
>      cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"
>      f = io.popen(cmd)
> ======================================================================
>  
> And here is the new commit message with your comments in mind:
> ======================================================================
>     test: disable interactive mode assertion on BSD
>  
>     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
>  
> ======================================================================
> > 
> >> 
> >>>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
> >> 

-- 
Best regards,
Sergey Kaplun