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

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

Thanks for the comments! Please see my answers and fixes.

The fixes was force pushed to the branch.

New changes:
- two tests refactored to use utils.exec;
- one of them uses new utils.exec.makecmd_exec() -> example removed from 
commit message;
- args type checks in the 'exec' module slightly refactored.

On 11/6/26 5:37 PM, Sergey Bronnikov wrote:
> 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?
> 
Fixed.

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

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

>>
>> How to use new 'utils.exec.makecmd_exec()' to run arbitrary
>> shell script:
> s/arbitrary/an arbitrary/
Removed.

>>
>> ```
>> 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 =/
Removed.

>>
>> 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.
> 
> 
Yes, it was an exceptional case. Removed due to the example appears in 
the code (refactored 
'test/tarantool-tests/lj-586-debug-non-string-error.test.lua' test).

>> ---
>>   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?
Yes! Done.

>> +    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?
Fixed in new `makecmd_tap_trace()` helper.

> 
> Also, I don't like mixing tap helpers with code helpers.
It's a point. Made a common tracer plus `makecmd_tap_trace()` helper to 
redirect the data to tap output.

> I would replace diagnostics by assertions.
> 
Assertions may clog the test code, and it seems they are not helpful for 
any case. I think, they may be omitted for generated commands for test 
purposes (we expect the `makecmd_exec()` and the test code is made 
good). But the generated commands may be helpful in the log for the new 
reader, which tries to understand the tests. Or the reader must have a 
possibility to enable such log lines easy for discovery purposes.

>>         -- 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.
Refactored this test to use `makecmd()` function.

Also refactored 
'test/tarantool-tests/lj-586-debug-non-string-error.test.lua' to use 
common `makecmd_exec()` function.

---

Rewritten commit message:

test: refactor utils.exec.makecmd

The 'utils.exec.makecmd()' was refactored. The common
'utils.exec.makecmd_exec()' added to allow executing arbitrary
commands and the 'makecmd()' is implemented on top of it.
The command object's 'LUABIN' field renamed to 'BIN', the
'SCRIPT' field with script filename replaced with 'PERMARGS'
field, containing all command arguments, which are passed on
every call, and the 'trace' callback option is added for
additional processing of the generated command and it
output.

Also added the 'makecmd_tap_trace()' and 'test_data()' helpers.

Test [1] was refactored to use 'makecmd()' as usual.
Test [2] was refactored to use common 'makecmd_exec()'.

1. test/tarantool-tests/lj-962-stack-overflow-report.test.lua
2. test/tarantool-tests/lj-586-debug-non-string-error.test.lua

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

---

diff --git a/test/tarantool-tests/lj-586-debug-non-string-error.test.lua 
b/test/tarantool-tests/lj-586-debug-non-string-error.test.lua
index 67869708..570ea484 100644
--- a/test/tarantool-tests/lj-586-debug-non-string-error.test.lua
+++ b/test/tarantool-tests/lj-586-debug-non-string-error.test.lua
@@ -6,20 +6,18 @@ test:plan(1)

  -- XXX: This is quite an exotic test case in that point of view
  -- that testing the debug interactive interface always ends with
--- sending commands to another instance via stdin. However, the
--- module with test helpers lacks the suitable routine.
--- `utils.makecmd()` doesn't fit for this, since `debug.debug()`
--- captures `io.stdin` and waits at `fgets()` in debug busy loop.
--- As it's already mentioned, such tests are not usual, so there
--- is no need to introduce a new helper to utils module (at least
--- for now). Hence, we use `utils.luacmd()` function to obtain the
--- interpreter command, run `debug.debug()` with it, send two
--- commands via pipeline to the launched process, capture its
--- stdout and stderr and test the resulting output against the
--- expected one.
+-- sending commands to another instance via stdin.
+-- Since `debug.debug()` captures `io.stdin` and waits at
+-- `fgets()` in debug busy loop, we use `utils.exec.luacmd()`
+-- function to obtain the interpreter command, run
+-- `debug.debug()` with it, send two commands from the
+-- `utils.exec.test_data('commands.lua')` file redirecting it to
+-- the process stdin, capture its stdout and stderr and test the
+-- resulting output against the expected one.
  --
  -- Debugger prompt.
  local ldb = 'lua_debug> '
+-- The magic number is passed via MAGIC environment variable.
  local magic = 42
  local luabin = utils.exec.luacmd(arg)
  local stderr = {
@@ -37,11 +35,13 @@ local stderr = {
  -- errors are yielded before the magic given to `print()` command
  local expected = table.concat(stderr)..magic

-local cmd = ([[cat <<EOF | %s -e 'debug.debug()' 2>&1
-error({})
-print(%d)
-EOF]]):format(luabin, magic)
-
-local res = io.popen(cmd):read('*all'):gsub('%s+$', '')
+local dbg_commands_file = utils.exec.test_data(arg, 'commands.lua')
+local command = utils.exec.makecmd_exec(luabin, {
+  env = {MAGIC = magic},
+  redirect = ('2>&1 <%s'):format(dbg_commands_file),
+  args = {'-e', "'debug.debug()'"},
+  trace = utils.exec.makecmd_tap_trace(test),
+})
+local res = command()
  test:ok(res == expected, 'handle non-string error in debug.debug()')
  test:done(true)
diff --git 
a/test/tarantool-tests/lj-586-debug-non-string-error/commands.lua 
b/test/tarantool-tests/lj-586-debug-non-string-error/commands.lua
new file mode 100644
index 00000000..a27aaee7
--- /dev/null
+++ b/test/tarantool-tests/lj-586-debug-non-string-error/commands.lua
@@ -0,0 +1,2 @@
+error({})
+print(tonumber(os.getenv('MAGIC')))
diff --git a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua 
b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
index 848f4ead..14ffea10 100644
--- a/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
+++ b/test/tarantool-tests/lj-962-stack-overflow-report.test.lua
@@ -4,8 +4,8 @@ local tap = require('tap')
  local test = tap.test('lj-962-stack-overflow-report')
  test:plan(1)

-local LUABIN = require('utils').exec.luabin(arg)
-local SCRIPT = arg[0]:gsub('%.test%.lua$', '/script.lua')
-local output = io.popen(LUABIN .. ' 2>&1 ' .. SCRIPT, 'r'):read('*all')
+local utils = require('utils')
+local script = utils.exec.makecmd(arg, {redirect = '2>&1'})
+local output = script()
  test:like(output, 'stack overflow', 'stack overflow reported correctly')
  test:done(true)
diff --git a/test/tarantool-tests/utils/exec.lua 
b/test/tarantool-tests/utils/exec.lua
index b202d384..1b79bd63 100644
--- a/test/tarantool-tests/utils/exec.lua
+++ b/test/tarantool-tests/utils/exec.lua
@@ -22,6 +22,7 @@ end

  local function makeenv(tabenv)
    if tabenv == nil then return '' end
+  assert(type(tabenv) == 'table')
    local flatenv = {}
    for var, value in pairs(tabenv) do
      if var == 'LD_PRELOAD' then
@@ -50,18 +51,23 @@ 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.tap for diagnostic.
+-- Pass opts.trace function(event, value) for additional
+-- command/output processing.
  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 {}
+  opts = opts or {}
+  assert(type(opts) == 'table')
+  local redirect = opts.redirect or ''
+  assert(type(redirect) == 'string')
+  local perm_args = opts.args or {}
    assert(type(perm_args) == 'table')
+  assert(opts.trace == nil or type(opts.trace) == 'function')
    return setmetatable({
      BIN = bin,
-    ENV = opts and makeenv(opts.env) or '',
-    REDIRECT = opts and opts.redirect or '',
-    PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)),
-    tap = opts and opts.tap,
+    ENV = makeenv(opts.env),
+    REDIRECT = redirect,
+    PERMARGS = table.concat(perm_args, ' '),
+    trace = opts.trace,
    }, {
      __call = function(self, ...)
        -- This line just makes the command for <io.popen> by the
@@ -73,17 +79,23 @@ function M.makecmd_exec(bin, opts)
        --    the resulting command.
        local cmd = ('<ENV> <BIN> <REDIRECT> 
<PERMARGS>'):gsub('%<(%w+)>', self)
                    .. (' %s'):rep(select('#', ...)):format(...)
-      if self.tap ~= nil then self.tap:diag('cmd: ' .. cmd) end
+      if self.trace ~= nil then self.trace('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.tap ~= nil then self.tap:diag('output: ' .. output) end
+      if self.trace ~= nil then self.trace('output', output) end
        return output
      end
    })
  end

+-- Make a trace function for `makecmd_exec` script, which produces
+-- tap test diagnostic messages with the command and it's output.
+function M.makecmd_tap_trace(tap)
+  return function(ev, val) tap:diag('%s: %s', ev, val) end
+end
+
  -- Makes a file name for running test's data file in
  -- '%testname%' directory.
  function M.test_data(arg, name)
@@ -102,7 +114,7 @@ function M.makecmd(arg, opts)
      env = opts and opts.env or nil,
      redirect = opts and opts.redirect or nil,
      args = {script},
-    tap = opts and opts.tap or nil,
+    trace = opts and opts.trace or nil,
    })
  end

-- 
--
Best regards,
Evgeniy Temirgaleev