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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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/

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

Typo: s/simplification/simplification,/

> 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).

> 
> 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.

> +  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?

>    }, {
>      __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/../ .. /

>        -- 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/../ .. /

> +      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?

> +  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
> 

-- 
Best regards,
Sergey Kaplun