<HTML><BODY><div>Hi!</div><div>Here is the fix for the comment:</div><div>======================================================================</div><div><div>diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t<br>index 742765d4..f77696d9 100755<br>--- a/test/lua-Harness-tests/241-standalone.t<br>+++ b/test/lua-Harness-tests/241-standalone.t<br>@@ -112,8 +112,8 @@ f = io.popen(cmd)<br> equals(f:read'*l', 'Hello World', "redirect")<br> f:close()</div><div>--- FIXME Tarantool interactive mode misbehaviour has been found on<br>--- FreeBSD (for more info see #6231).<br>+-- FIXME: Tarantool interactive mode misbehaviour on<br>+-- FreeBSD (for more info, see #6231).<br> if jit.os ~= 'BSD' then<br> cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"<br> f = io.popen(cmd)<br>======================================================================</div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16275519720895073921_BODY">Hi, thanks for the fixes!<br><br>LGTM, except a few nits below.<br>On 29.07.21, Максим Корякшин wrote:<br>><br>> Hi! Thanks for the review, Sergey!<br>> Here is the diff, which adds a comment explaining the reason why the<br>> discussed assertion was masked:<br>> ======================================================================<br>> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t<br>> index bdd95514..742765d4 100755<br>> --- a/test/lua-Harness-tests/241-standalone.t<br>> +++ b/test/lua-Harness-tests/241-standalone.t<br>> @@ -112,6 +112,8 @@ f = io.popen(cmd)<br>> equals(f:read'*l', 'Hello World', "redirect")<br>> f:close()<br>> +-- FIXME Tarantool interactive mode misbehaviour has been found on<br><br>Typo: s/FIXME/FIXME:/<br><br>Nit: Also I suppose, that "has been found" can be dropped.<br>Feel free to ignore.<br><br>> +-- FreeBSD (for more info see #6231).<br><br>Typo: s/for more info see/for more info, see/<br><br>> if jit.os ~= 'BSD' then<br>> cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"<br>> f = io.popen(cmd)<br>> ======================================================================<br>> <br>> And here is the new commit message with your comments in mind:<br>> ======================================================================<br>> test: disable interactive mode assertion on BSD<br>> <br>> Tarantool interactive mode misbehaviour has been found on<br>> FreeBSD (for more info see tarantool/tarantool#6231). Hence, this[1]<br>> particular assertion is masked for FreeBSD until the mentioned issue<br>> resolves.<br>> <br>> [1]: <a href="https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130" target="_blank">https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130</a><br>> <br>> Part of tarantool/tarantool#5970<br>> Part of tarantool/tarantool#4473<br>> Relates to tarantool/tarantool#6231<br>> <br>> ======================================================================<br>> > <br>> >> <br>> >>>Hi!<br>> >>>Thanks for the patch!<br>> >>><br>> >>>LGTM, except a few nits below.<br>> >>><br>> >>>On 26.07.21, Максим Корякшин wrote:<br>> >>>><br>> >>>> Sorry for the several emails, I have encountered some issues with the email client.<br>> >>>> <br>> >>>> Anyway, here is the diff, that masks only one particular assertion instead of masking all of them:<br>> >>>> ========================================================================<br>> >>>> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t<br>> >>>> index d5373b9f..bdd95514 100755<br>> >>>> --- a/test/lua-Harness-tests/241-standalone.t<br>> >>>> +++ b/test/lua-Harness-tests/241-standalone.t<br>> >>>> @@ -51,10 +51,6 @@ if not pcall(io.popen, lua .. [[ -e "a=1"]]) then<br>> >>>> skip_all "io.popen not supported"<br>> >>>> end<br>> >>>> -if jit.os == 'BSD' then<br>> >>>> - skip_all "BSD is not supported yet"<br>> >>>> -end<br>> >>>> -<br>> >>>> plan'no_plan'<br>> >>>> diag(lua)<br>> >>>> @@ -116,22 +112,24 @@ f = io.popen(cmd)<br>> >>>> equals(f:read'*l', 'Hello World', "redirect")<br>> >>>> f:close()<br>> >>>> -cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"<br>> >>>> -f = io.popen(cmd)<br>> >>>> -matches(f:read'*l', banner, "-i")<br>> >>>> -if ujit then<br>> >>>> - matches(f:read'*l', '^JIT:')<br>> >>>> -end<br>> >>>> -if ravi then<br>> >>>> - matches(f:read'*l', '^Copyright %(C%)')<br>> >>>> - matches(f:read'*l', '^Portions Copyright %(C%)')<br>> >>>> - matches(f:read'*l', '^Options')<br>> >>>> -end<br>> >>>> -if _TARANTOOL then<br>> >>>> - matches(f:read'*l', "^type 'help' for interactive help")<br>> >>>> +if jit.os ~= 'BSD' then<br>> >>><br>> >>>Nit: Feel free to drop a comment here why this part is disabled for<br>> >>>FreeBSD (maybe with FIXME or XXX label).<br>> >>><br>> >>>> + cmd = lua .. " -i hello-241.lua < hello-241.lua 2>&1"<br>> >>>> + f = io.popen(cmd)<br>> >>>> + matches(f:read'*l', banner, "-i")<br>> >>>> + if ujit then<br>> >>>> + matches(f:read'*l', '^JIT:')<br>> >>>> + end<br>> >>>> + if ravi then<br>> >>>> + matches(f:read'*l', '^Copyright %(C%)')<br>> >>>> + matches(f:read'*l', '^Portions Copyright %(C%)')<br>> >>>> + matches(f:read'*l', '^Options')<br>> >>>> + end<br>> >>>> + if _TARANTOOL then<br>> >>>> + matches(f:read'*l', "^type 'help' for interactive help")<br>> >>>> + end<br>> >>>> + equals(f:read'*l', 'Hello World')<br>> >>>> + f:close()<br>> >>>> end<br>> >>>> -equals(f:read'*l', 'Hello World')<br>> >>>> -f:close()<br>> >>>> cmd = lua .. [[ -e"a=1" -e "print(a)"]]<br>> >>>> f = io.popen(cmd)<br>> >>>> ========================================================================<br>> >>>> <br>> >>>> And here is the fixed commit message:<br>> >>>> ========================================================================<br>> >>>> test: disable 241-standalone.t on FreeBSD<br>> >>><br>> >>>Nit: Strictly saying the patch disables only interactive mode part with<br>> >>>input from the file.<br>> >>><br>> >>>> <br>> >>>> Tarantool interactive mode misbehaviour has been found on<br>> >>>> FreeBSD (for more info see tarantool/tarantool#6231). Hence, this[1]<br>> >>>> particular assertion is masked for FreeBSD until the mentioned issue<br>> >>>> resolves.<br>> >>>> <br>> >>>> [1]: <a href="https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130" target="_blank">https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130</a><br>> >>>> <br>> >>>> Part of tarantool/tarantool#5970<br>> >>>> Part of tarantool/tarantool#4473<br>> >>>> Relates to tarantool/tarantool#6231<br>> >>>> <br>> >>>> ========================================================================<br>> >>>> <br>> >>>> >Понедельник, 26 июля 2021, 15:32 +03:00 от Максим Корякшин < <a href="/compose?To=m.kokryashkin@tarantool.org">m.kokryashkin@tarantool.org</a> >:<br>> >>>> > <br>> >>>> >diff:<br>> >>>> > <br>> >>>> > <br>> >>>> >> <br>> >>>> >>>Max,<br>> >>>> >>><br>> >>>> >>>Thanks for the patch! Please consider the comments below.<br>> >>>> >>><br>> >>>> >>>At first, commit subject exceeds 50 symbols and the message exceeds 72<br>> >>>> >>>symbols. Please check this patch against our guidelines[1].<br>> >>>> >>><br>> >>>> >>>On 20.07.21, Maxim Kokryashkin wrote:<br>> >>>> >>>> Tarantool interactive mode misbehaviour has been found on FreeBSD (for more info see #6231).<br>> >>>> >>><br>> >>>> >>>The issue is mentioned the wrong way. The right format is<br>> >>>> >>>tarantool/tarantool#6231.<br>> >>>> >>><br>> >>>> >>>> Hence, this particular assertion is masked for FreeBSD until the mentioned issue resolves.<br>> >>>> >>><br>> >>>> >>>Strictly saying, you skipped all assertions (not only those using broken<br>> >>>> >>>interactive mode), not the particular one.<br>> >>>> >>><br>> >>>> >>>><br>> >>>> >>>> Resolves #5970<br>> >>>> >>><br>> >>>> >>>This patch doesn't resolve the issue, since Tarantool profile introduced<br>> >>>> >>>in "[PATCH v2 06/10] test: support tarantool in lua-Harness" is not used<br>> >>>> >>>by Tarantool CI. Hence this is only "Part of" patch.<br>> >>>> >>><br>> >>>> >>>> Part of #4473<br>> >>>> >>><br>> >>>> >>>BTW, both issues are also mentioned the wrong way. The right format is<br>> >>>> >>>tarantool/tarantool#<issue-number> for both cases (see the previous<br>> >>>> >>>patches in the series for the examples).<br>> >>>> >>><br>> >>>> >>>It's also worth to add "Relates to tarantool/tarantool#6231".<br>> >>>> >>><br>> >>>> >>>> ---<br>> >>>> >>>> Additional info: <a href="https://github.com/tarantool/tarantool/issues/5970#issuecomment-883253400" target="_blank">https://github.com/tarantool/tarantool/issues/5970#issuecomment-883253400</a><br>> >>>> >>>><br>> >>>> >>>> test/lua-Harness-tests/241-standalone.t | 4 ++++<br>> >>>> >>>> 1 file changed, 4 insertions(+)<br>> >>>> >>>><br>> >>>> >>>> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t<br>> >>>> >>>> index afbcf5b8..d5373b9f 100755<br>> >>>> >>>> --- a/test/lua-Harness-tests/241-standalone.t<br>> >>>> >>>> +++ b/test/lua-Harness-tests/241-standalone.t<br>> >>>> >>>> @@ -51,6 +51,10 @@ if not pcall(io.popen, lua .. [[ -e "a=1"]]) then<br>> >>>> >>>> skip_all "io.popen not supported"<br>> >>>> >>>> end<br>> >>>> >>>><br>> >>>> >>>> +if jit.os == 'BSD' then<br>> >>>> >>>> + skip_all "BSD is not supported yet"<br>> >>>> >>>> +end<br>> >>>> >>><br>> >>>> >>>At first, please add a comment with the rationale for this change.<br>> >>>> >>>Furthermore, it's better to skip only assertions using interactive mode<br>> >>>> >>>and run other assertions related to CLI behaviour.<br>> >>>> >>><br>> >>>> >>>> +<br>> >>>> >>>> plan'no_plan'<br>> >>>> >>>> diag(lua)<br>> >>>> >>>><br>> >>>> >>>> --<br>> >>>> >>>> 2.32.0<br>> >>>> >>>><br>> >>>> >>><br>> >>>> >>>[1]: <a href="https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/" target="_blank">https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/</a><br>> >>>> >>><br>> >>>> >>>--<br>> >>>> >>>Best regards,<br>> >>>> >>>IM<br>> >>>> >> <br>> >>>> > <br>> >>>> <br>> >>><br>> >>>--<br>> >>>Best regards,<br>> >>>Sergey Kaplun<br>> >> <br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></div></blockquote></BODY></HTML>