From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounces@dev.tarantool.org>
Received: from [87.239.111.99] (localhost [127.0.0.1])
	by dev.tarantool.org (Postfix) with ESMTP id EB49C6EC44;
	Fri,  6 Nov 2026 17:37:45 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EB49C6EC44
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev;
	t=1793975866; bh=JtSVHMisgct8oBETbCzt5ZfUreqXltCPIX5V7mxdwY0=;
	h=Date:To:Cc:References:In-Reply-To:Subject:List-Id:
	 List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe:
	 From:Reply-To:From;
	b=wUvL09eDV3dQ+8OfLRSdnMOcg1caS0A7m6v7cVXF66pvHZHzmrFxKuct8e8DyCVnP
	 ocMvrKl8YqFL3JhD8vTX/3mXagjNjaVF+ZAUUX53dZCQA0ZTUnnWS/gHre9P0XCnU9
	 zemGj1OBYVOrixhPn931omIsZifjMvshLENF/Aks=
Received: from send103.i.mail.ru (send103.i.mail.ru [89.221.237.198])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 7D69E6EC44
 for <tarantool-patches@dev.tarantool.org>;
 Fri,  6 Nov 2026 17:37:44 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7D69E6EC44
Received: by exim-smtp-5cc8b577fd-dstcs with esmtpa (envelope-from
 <sergeyb@tarantool.org>)
 id 1xPL4Y-00000000AxK-1yCy; Fri, 06 Nov 2026 17:37:43 +0300
Message-ID: <bfc85556-6038-4d41-91f5-66471a08a69c@tarantool.org>
Date: Fri, 6 Nov 2026 17:37:36 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Evgeniy Temirgaleev <e.temirgaleev@tarantool.org>,
 Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
References: <cover.1793574664.git.e.temirgaleev@tarantool.org>
 <51f5c0e5d584e825054581da9152a39df9fd9eb9.1793574664.git.e.temirgaleev@tarantool.org>
Content-Language: en-US
In-Reply-To: <51f5c0e5d584e825054581da9152a39df9fd9eb9.1793574664.git.e.temirgaleev@tarantool.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
X-Mailru-Src: smtp
X-4EC0790: 10
X-7564579A: B8F34718100C35BD
X-77F55803: 4F1203BC0FB41BD9615418BB48D65B569100DA7D02E8E3AD9A1D27F24E8F2F3B182A05F5380850404C228DA9ACA6FE27E8075E51B36072193DE06ABAFEAF6705522B83781726F1A1DA372D2B19622D46A7427CD01E6BCEC9
X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72F22E6DC541F75D9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006376A27C8034CCD05E58638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B268EA6304EFBD13B62E070BE324C7D3C4B63DE92AC9177C4DF6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C06030C3405640F6718941B15DA834481F9449624AB7ADAF372E808ACE2090B5E14AD6D5ED66289B5278DA827A17800CE76631511D42670FFE2EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7BB1D8AFE8C29F51C731C566533BA786AA5CC5B56E945C8DA
X-C1DE0DAB: 0D63561A33F958A5701D88A81E5754F55002B1117B3ED6964F1B15C132CD4663715D9AB585B0EB043610D81D389A125CDE35189EBF2DEA28FEA14CD2CD220BB99C5DF10A05D560A9880EC71AF561E0AAD9143641EC25BB396F7CE96358175CB5
X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE1918E10F71CB4DF9F96AB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D659A6D97C43DD83139DD631B44240419A3AC1F19ED889BA177F2E7CB12501D321FDC23BFF6239F51756B8341EE9D5BE9A0A832F5D7EC4C753A08D5A8AA4B97602DDAF3A8FBD6F7A8D8A6536EB022892E5344C41F94D744909CE2512F26BEC029E55448553D2254B8D95CD72808BE417F3B9E0E7457915DAA85F
X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXOcIETfglQORZ0zpDET4Zrk3igikrdHlWBTj2hRYM+BB3KofJT2Itw4=
X-Mailru-Sender: 689FA8AB762F73937C9FA53A4753B313CB2A5BA2FC1F08C6AA6621343CFCDB6EEE87603D0CEBB3EFEF86D5F70DA33880E41E8EF7A07863ECB274557F927329BE2DDF8182D28ACDB545BD1C3CC395C826B4A721A3011E896F
X-Mras: Ok
Subject: Re: [Tarantool-patches] [PATCH luajit v3 1/2] test: refactored
 utils.exec.makecmd
X-BeenThere: tarantool-patches@dev.tarantool.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
From: Sergey Bronnikov via Tarantool-patches
 <tarantool-patches@dev.tarantool.org>
Reply-To: Sergey Bronnikov <sergeyb@tarantool.org>
Errors-To: tarantool-patches-bounces@dev.tarantool.org
Sender: "Tarantool-patches" <tarantool-patches-bounces@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.