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

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

Hi,

please see my comment below.

 >  test: refactored utils.exec.makecmd

Tarantool's developers guide asks for an imperative title; "refactored" 
is past tense.
Would `test: refactor utils.exec.makecmd` be better?

Sergey

On 11/2/26 02:59, 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
s/allow executing of the arbitrary commands/allow executing arbitrary 
commands/
> if the test object is supplied. See the example below.
>
> Initially this patch was prepared for tests in the follows 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.
  The new `test_data()` helper and the changed `makecmd()` object fields are
   not mentioned. Nothing in the tree reads the removed `LUABIN`/`SCRIPT`
   (checked), so the change is safe, but it is an interface change worth a
   line in the message.
>
> How to use new 'utils.exec.makecmd_exec()' to run arbitrary
> shell script:
s/arbitrary/an arbitrary/
>
> ```
> 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
> })
typo: s/t =/tap =/
>
> 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)
> ```

Usually we don't put code examples to the commit messages.


> ---
>   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)
> +  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)),
`PERMARGS` already starts each argument with a space while the template also
has a space before `<PERMARGS>`: `makecmd()` now builds ` luabin   script`
(three spaces) instead of the old two. Harmless for the shell, but the
command is printed in the diagnostics now, so the extra whitespace is
user-visible. Would letting one place own the separator -- e.g.
`PERMARGS = table.concat(perm_args, ' ')` with the template's space kept --
read simpler and produce a clean command line?
> +    t = opts and opts.t,
>     }, {
>       __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
`tap.diag()` treats its first argument as a format string. Passing
`'cmd: ' .. cmd` / `'output: ' .. output` means any `%` in the command or in
the child output either throws or is interpreted as a conversion. Minimal
reproduction:


luajit -e 'local e = dofile("./test/tarantool-tests/utils/exec.lua")
   local t = {diag = function(_, fmt, ...) return fmt:format(...) end}
   e.makecmd_exec("/bin/echo", {args = {"%"}, tap = t})("100%")'
luajit: ./test/tarantool-tests/utils/exec.lua:76: invalid option '% 100' 
to 'format'
stack traceback:
         [C]: in function 'diag'
         ./test/tarantool-tests/utils/exec.lua:76: in function 
<./test/tarantool-tests/utils/exec.lua:66>
         (command line):3: in main chunk
         [C]: at 0x560209573120


A test that prints `100%`, or a call like `script("'echo 100%'")`, will make
the utility error out instead of printing the diagnostic. `tap.lua` itself
uses the safe form (`test:diag("%s", test.name)`); would
`self.tap:diag('%s', 'cmd: ' .. cmd)` and
`self.tap:diag('%s', 'output: ' .. output)` fix it?


Also, I don't like mixing tap helpers with code helpers. I would replace 
diagnostics by assertions.

>         -- 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
> +      return output
>       end
>     })
>   end
>   
> +-- Makes a file name for running test's data file in
> +-- '%testname%' directory.
> +function M.test_data(arg, name)
> +  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

test/tarantool-tests/lj-962-stack-overflow-report.test.lua

Now that `utils.exec.test_data()` exists, this test still computes the
script path manually with `arg[0]:gsub('%.test%.lua$', '/script.lua')`.
Would switching it to `test_data(arg, 'script.lua')` keep the script-file
naming convention in one place? This is the only remaining manual 
occurrence.