Re: [Tarantool-patches] [PATCH luajit v3 1/2] test: refactored utils.exec.makecmd

[Evgeniy Temirgaleev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey! Thanks for review!

Please, see my answers. Fixes are made and force pushed.

On 11/3/26 2:55 PM, Sergey Kaplun wrote:
> Hi, Evgeniy!
> Thanks for the patch!
> Please consider my comments below.
> 
> On 02.11.26, Evgeniy Temirgaleev wrote:
>> From: Temir Galeev <temir.galeev@bk.ru>
>>
>> The 'utils.exec.makecmd()' was refactored to allow executing of the
>> arbitrary commands. Also, the diagnostic messages may be enabled
>> if the test object is supplied. See the example below.
>>
>> Initially this patch was prepared for tests in the follows up
> 
> Typo: s/follows up/follow up/
> 
Fixed.

>> 'Improve CLI signal handling on POSIX' patch. After the test
>> simplification the shell script helper was wiped out, but the
> 
> Typo: s/simplification/simplification,/
> 
Fixed.

>> refactoring was already done.
> 
> This refactoring looks OK, but the second patch doesn't use the newly
> introduced function. So it is questionable: do we need it right now?
> I don't understand what benefits will we have with it (since the testing
> of CLI is a unique case, as you can see).
> 
The other tests uses the 'utils.exec.makecmd()' func. The refactoring 
makes it's code more clear to understand and in consequence more easy to 
use.

We can postpone the export of 'makecmd_exec()' function until it will be 
really need, but the refactoring is a part of work which is wasteful to 
reject due to it is done already.

>>
>> How to use new 'utils.exec.makecmd_exec()' to run arbitrary
>> shell script:
>>
>> ```
>> local tap = require('tap')
>> local test = tap.test('example')
>> local utils = require('utils')
>> local script = utils.exec.makecmd_exec('/bin/sh', {
>>    args = {'-c'},
>>    redirect = '2>&1',
>>    t = test, -- shows cmd and it's output for diagnostic
>> })
>>
>> test:plan(2)
>> local output
>> output = script("'echo 1'")
>> test:is(output, '1')
>> output = script("'echo 2'")
>> test:is(output, '23') -- see the on-failure output
>> test:done(true)
>> ```
>> ---
>>   test/tarantool-tests/utils/exec.lua | 56 +++++++++++++++++++++++------
>>   1 file changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua
>> index 49f8ef06..b435e084 100644
>> --- a/test/tarantool-tests/utils/exec.lua
>> +++ b/test/tarantool-tests/utils/exec.lua
>> @@ -43,33 +43,67 @@ local function makeenv(tabenv)
>>     return table.concat(flatenv, ' ')
>>   end
>>   
>> --- <makecmd> creates a command that runs %testname%/script.lua by
>> --- <LUAJIT_TEST_BINARY> with the given environment, launch options
>> --- and CLI arguments. The function yields an object (i.e. table)
>> --- with the aforementioned parameters. To launch the command just
>> --- call the object.
>> -function M.makecmd(arg, opts)
>> +-- <makecmd_exec> creates a command that runs given binary
>> +-- with the given environment, launch options and zero or
>> +-- more permanent arguments.
>> +-- The function yields an object (i.e. table) with the
>> +-- aforementioned parameters. To launch the command just call
>> +-- the object. Additional arguments which may vary from call
>> +-- to call are the `__call` args.
>> +-- Pass tap.test() object as opts.t for diagnostic.
>> +function M.makecmd_exec(bin, opts)
> 
> This refactoring looks OK, but the second patch doesn't use the newly
> introduced function. So it is questionable: do we need it right now?
> If not, I suppose the debug adjustment (and default script value) may be
> done in the scope of the old `makecmd()` helper.
> 
The same.

>> +  assert(type(bin) == 'string')
>> +  assert(opts == nil or type(opts) == 'table')
>> +  local perm_args = opts and opts.args or {}
>> +  assert(type(perm_args) == 'table')
>>     return setmetatable({
>> -    LUABIN = M.luacmd(arg),
>> -    SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'),
>> +    BIN = bin,
>>       ENV = opts and makeenv(opts.env) or '',
>>       REDIRECT = opts and opts.redirect or '',
>> +    PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)),
>> +    t = opts and opts.t,
> 
> Maybe name it tap instead?
> 
Renamed.

>>     }, {
>>       __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).
>> +      --    given to the command constructor (e.g. bin, 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> <BIN> <REDIRECT> <PERMARGS>'):gsub('%<(%w+)>', self)
>>                     .. (' %s'):rep(select('#', ...)):format(...)
>> +      if self.t ~= nil then self.t:diag('cmd: '..cmd) end
> 
> Minor: IMO, `if self.t then` is more readable, since we don't want to check
> its type but only its absence. Feel free to ignore.
> 
> 
> Minor: s/../ .. /
> 
I try to follow Tarantool's "Lua style guide" [1], where implicit 
casting to bool is prohibited.

1. 
https://www.tarantool.io/en/doc/latest/contributing/lua_style_guide/#idioms-and-patterns

Dots: fixed.

>>         -- Trim both leading and trailing whitespace from the output
>>         -- produced by the child process.
>> -      return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
>> +      local output = io.popen(cmd):
>> +        read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
>> +      if self.t ~= nil then self.t:diag('output: '..output) end
> 
> Minor: IMO, `if self.t then` is more readable, since we don't want to
> check its type but only its absence. Feel free to ignore.
> Minor: s/../ .. /
> 
The same.

>> +      return output
>>       end
>>     })
>>   end
>>   
>> +-- Makes a file name for running test's data file in
>> +-- '%testname%' directory.
>> +function M.test_data(arg, name)
> 
> Minor: `test_data()` sounds too general. Maybe `scriptname()` instead?
> 
When we need to store some other input data for the test, will we rename 
or will we make one more function?

>> +  local fname, matches = arg[0]:gsub('%.test%.lua$', '/'..name)
>> +  assert(matches == 1, 'unexpected test filename')
>> +  return fname
>> +end
>> +
>> +-- <makecmd> creates a command that runs %testname%/script.lua by
>> +-- <LUAJIT_TEST_BINARY> with the given environment, launch options
>> +-- and CLI arguments.
>> +function M.makecmd(arg, opts)
>> +  local luabin = M.luacmd(arg)
>> +  local script = opts and opts.script or M.test_data(arg, 'script.lua')
>> +  return M.makecmd_exec(luabin, {
>> +    env = opts and opts.env or nil,
>> +    redirect = opts and opts.redirect or nil,
>> +    args = {script},
>> +    t = opts and opts.t or nil,
>> +  })
>> +end
>> +
>>   return M
>> -- 
>> 2.49.0
>>
> 

---

test: refactored utils.exec.makecmd
...
Initially this patch was prepared for tests in the follow up
'Improve CLI signal handling on POSIX' patch. After the test
simplification, the shell script helper was wiped out, but the
refactoring was already done.
...

---

diff --git a/test/tarantool-tests/utils/exec.lua 
b/test/tarantool-tests/utils/exec.lua
index b435e084..b202d384 100644
--- a/test/tarantool-tests/utils/exec.lua
+++ b/test/tarantool-tests/utils/exec.lua
@@ -50,7 +50,7 @@ end
  -- aforementioned parameters. To launch the command just call
  -- the object. Additional arguments which may vary from call
  -- to call are the `__call` args.
--- Pass tap.test() object as opts.t for diagnostic.
+-- Pass tap.test() object as opts.tap for diagnostic.
  function M.makecmd_exec(bin, opts)
    assert(type(bin) == 'string')
    assert(opts == nil or type(opts) == 'table')
@@ -61,7 +61,7 @@ function M.makecmd_exec(bin, opts)
      ENV = opts and makeenv(opts.env) or '',
      REDIRECT = opts and opts.redirect or '',
      PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)),
-    t = opts and opts.t,
+    tap = opts and opts.tap,
    }, {
      __call = function(self, ...)
        -- This line just makes the command for <io.popen> by the
@@ -73,12 +73,12 @@ function M.makecmd_exec(bin, opts)
        --    the resulting command.
        local cmd = ('<ENV> <BIN> <REDIRECT> 
<PERMARGS>'):gsub('%<(%w+)>', self)
                    .. (' %s'):rep(select('#', ...)):format(...)
-      if self.t ~= nil then self.t:diag('cmd: '..cmd) end
+      if self.tap ~= nil then self.tap:diag('cmd: ' .. cmd) end
        -- Trim both leading and trailing whitespace from the output
        -- produced by the child process.
        local output = io.popen(cmd):
          read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
-      if self.t ~= nil then self.t:diag('output: '..output) end
+      if self.tap ~= nil then self.tap:diag('output: ' .. output) end
        return output
      end
    })
@@ -102,7 +102,7 @@ function M.makecmd(arg, opts)
      env = opts and opts.env or nil,
      redirect = opts and opts.redirect or nil,
      args = {script},
-    t = opts and opts.t or nil,
+    tap = opts and opts.tap or nil,
    })
  end

-- 
--
Best regards,
Evgeniy Temirgaleev