Re: [Tarantool-patches] [PATCH luajit 2/5] test: stop using utils.selfrun in tests

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.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