<HTML><BODY><div>Hi! Thanks for the review, Sergey!</div><div>Here is the diff, which adds a comment explaining the reason why the</div><div>discussed assertion was masked:</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 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()</div><div>+-- FIXME Tarantool interactive mode misbehaviour has been found 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)</div><div>======================================================================</div></div><div> </div><div>And here is the new commit message with your comments in mind:</div><div>======================================================================</div><div><div> test: disable interactive mode assertion on BSD</div><div> </div><div> 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.</div><div> </div><div> [1]: <a href="https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130">https://framagit.org/fperrad/lua-Harness/-/blob/master/test_lua/241-standalone.t#L115-130</a></div><div> </div><div> Part of tarantool/tarantool#5970<br> Part of tarantool/tarantool#4473<br> Relates to tarantool/tarantool#6231<br> </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> <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_16273669561804292072_BODY">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</div></div></div></div></blockquote><div> </div></div></blockquote></div></blockquote></BODY></HTML>