[Tarantool-patches] [PATCH v2 10/10] test: disable test/lua-Harness-tests/241-standalone.t on FreeBSD
Sergey Kaplun
skaplun at tarantool.org
Thu Jul 29 12:45:00 MSK 2021
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 at 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
More information about the Tarantool-patches
mailing list