<HTML><BODY><div>Hi, Igor!</div><div>Thanks for the patch!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Понедельник, 27 февраля 2023, 21:07 +03:00 от Igor Munkin <imun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16775212200343519074_BODY">Sergey,<br><br>Thanks for your review! See my answes below.<br><br>On 27.02.23, Sergey Kaplun wrote:<br>> Hi, Igor!<br>> Thanks for the fixes!<br>> LGTM, just a few minor nits below.<br>><br>> On 27.02.23, Igor Munkin wrote:<br>> > Unfortunately, <utils.selfrun> is too complex to be maintained, so the<br>> > corresponding tests are split into two files: the test itself and the<br>> > script to be run by the test. As a result of the patch <utils.makecmd><br>> > helper is introduced: it inherits some approaches from <utils.selfrun>,<br>> > but it's considered for more general use.<br>> ><br>> > Signed-off-by: Igor Munkin <<a href="/compose?To=imun@tarantool.org">imun@tarantool.org</a>><br>> > ---<br>> > .../gh-4427-ffi-sandwich.test.lua | 88 +++++++++++--------<br>> > .../gh-4427-ffi-sandwich/script.lua | 25 ++++++<br>> > .../lj-351-print-tostring-number.test.lua | 34 +++----<br>> > .../lj-351-print-tostring-number/script.lua | 9 ++<br>> > .../lj-586-debug-non-string-error.test.lua | 2 +-<br>> > .../lj-flush-on-trace.test.lua | 87 ++++++++++--------<br>> > .../lj-flush-on-trace/script.lua | 23 +++++<br>> > test/tarantool-tests/utils.lua | 80 +++++++----------<br>> > 8 files changed, 200 insertions(+), 148 deletions(-)<br>> > create mode 100644 test/tarantool-tests/gh-4427-ffi-sandwich/script.lua<br>> > create mode 100644 test/tarantool-tests/lj-351-print-tostring-number/script.lua<br>> > create mode 100644 test/tarantool-tests/lj-flush-on-trace/script.lua<br>> ><br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua<br>> > new file mode 100644<br>> > index 00000000..9ecd964e<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua<br>><br>> <snipped><br>><br>> > +jit.opt.start("3", string.format("hotloop=%d", hotloop))<br>><br>> Do we need set level 3 direct here?<br>> Also, I suggest to use single quotes according to our coding style in<br>> tests.<br><br>I don't remember, so removed. Quotes are fixed. The diff is below:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua<br>index 9ecd964e..a7445e92 100644<br>--- a/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua<br>+++ b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua<br>@@ -14,7 +14,7 @@ local sandwich = require('libsandwich')(trigger)<br> -- * if trigger <= hotloop -> trace recording is aborted<br> -- * if trigger > hotloop -> trace is recorded but execution<br> -- leads to panic<br>-jit.opt.start("3", string.format("hotloop=%d", hotloop))<br>+jit.opt.start(string.format('hotloop=%d', hotloop))<br> <br> local res<br> for i = 0, hotloop + trigger do<br><br>================================================================================<br><br>><br>> > +<br><br><snipped><br><br>> > diff --git a/test/tarantool-tests/lj-flush-on-trace/script.lua b/test/tarantool-tests/lj-flush-on-trace/script.lua<br>> > new file mode 100644<br>> > index 00000000..d2c35534<br>> > --- /dev/null<br>> > +++ b/test/tarantool-tests/lj-flush-on-trace/script.lua<br>><br>> <snipped><br>><br>> > +jit.opt.start("3", string.format("hotloop=%d", hotloop))<br>><br>> Do we need set level 3 direct here?<br>> Also, I suggest to use single quotes according to our coding style in<br>> tests.<br><br>Ditto.<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/lj-flush-on-trace/script.lua b/test/tarantool-tests/lj-flush-on-trace/script.lua<br>index d2c35534..d25c4a33 100644<br>--- a/test/tarantool-tests/lj-flush-on-trace/script.lua<br>+++ b/test/tarantool-tests/lj-flush-on-trace/script.lua<br>@@ -14,7 +14,7 @@ local flush = require('libflush')(trigger)<br> -- * if trigger <= hotloop -> trace recording is aborted<br> -- * if trigger > hotloop -> trace is recorded but execution<br> -- leads to panic<br>-jit.opt.start("3", string.format("hotloop=%d", hotloop))<br>+jit.opt.start(string.format('hotloop=%d', hotloop))<br> <br> for i = 0, trigger + hotloop do<br> ffiflush.flush(flush, i)<br><br>================================================================================<br><br>><br>> > +<br>><br>> <snipped><br>><br>> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua<br>> > index eb11d40d..8355149b 100644<br>> > --- a/test/tarantool-tests/utils.lua<br>> > +++ b/test/tarantool-tests/utils.lua<br>><br>> <snipped><br>><br>> > + SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'),<br>> > + ENV = opts and makeenv(opts.env) or '',<br>> > + REDIRECT = opts and opts.redirect or '',<br>> > + }, {<br>> > + __call = function(self, ...)<br>> > + -- This line just makes the command for <io.popen> by the<br>> > + -- following steps:<br>> > + -- 1. Replace the placeholders with the corresponding values<br>> > + -- given to the command constructor (e.g. script, env)<br>> > + -- 2. Join all CLI arguments given to the __call metamethod<br>> > + -- 3. Concatenate the results of step 1 and step 2 to obtain<br>> > + -- the resulting command.<br>><br>> Extra dot at the end of the sentence (or missing dots for previous<br>> bullets).<br><br>Thanks, fixed! The diff is below:<br><br>================================================================================<br><br>diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua<br>index 83716bb3..8c1538d6 100644<br>--- a/test/tarantool-tests/utils.lua<br>+++ b/test/tarantool-tests/utils.lua<br>@@ -68,8 +68,8 @@ function M.makecmd(arg, opts)<br> -- This line just makes the command for <io.popen> by the<br> -- following steps:<br> -- 1. Replace the placeholders with the corresponding values<br>- -- given to the command constructor (e.g. script, env)<br>- -- 2. Join all CLI arguments given to the __call metamethod<br>+ -- given to the command constructor (e.g. script, env).<br>+ -- 2. Join all CLI arguments given to the __call metamethod.<br> -- 3. Concatenate the results of step 1 and step 2 to obtain<br> -- the resulting command.<br> local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self)<br><br>================================================================================<br><br>><br>> > + local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self)<br>> > + .. (' %s'):rep(select('#', ...)):format(...)<br>> > + -- Trim both leading and trailing whitespace from the output<br>> > + -- produced by the child process.<br>> > + return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '')<br>> > + end<br>> > + })<br>> > end<br>> ><br>> > function M.skipcond(condition, message)<br>> > --<br>> > 2.30.2<br>> ><br>><br>> --<br>> Best regards,<br>> Sergey Kaplun<br><br>--<br>Best regards,<br>IM</div></div></div></div></blockquote><div> </div></BODY></HTML>