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