Re: [Tarantool-patches] [PATCH luajit 3/7] test: stop using utils.selfrun in tests

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

Hi, Igor!
Thanks for the patch!
Generally LGTM, but please consider my entreaties about comments below.

On 13.02.23, Igor Munkin wrote:
> Unfortunately, <utils.selfrun> is too complex to be maintained, so the
> corresponding tests are split into two files: the test itself and the
> script to be run by the test. As a result of the patch <utils.makecmd>
> helper is introduced: it inherits some approaches from <utils.selfrun>,
> but it's considered for more general use.
> 
> Relates to tarantool/tarantool#8252
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---

<snipped>

> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index dd02130c..f4795db0 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -3,52 +3,43 @@ local utils = require('utils')

<snipped>

> --- Save the current coroutine and set the value to trigger
> --- <increment> call the Lua routine instead of C implementation.
> -local sandwich = require('libsandwich')(cfg.trigger)
> +local script = utils.makecmd(arg, {

It will be nice to drop a comment here and below, that `makecmd()`
searches for <%testname%/script.lua>.

> +  -- TODO: Leave another toxic comment regarding SIP on macOS.

Minor: This TODO is unnecessary, so feel free to delete this line.

> +  env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
> +  redirect = '2>&1',
> +})
>  
>  -- Depending on trigger and hotloop values the following contexts
>  -- are possible:
>  -- * if trigger <= hotloop -> trace recording is aborted
>  -- * if trigger >  hotloop -> trace is recorded but execution
>  --   leads to panic
> -jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop))
> +local hotloop = 1
> +local cases = {
> +  abort = {
> +    trigger = hotloop,
> +    expected = '#4427 still works',

Side note: We may provide this message from here to avoid it copy-pasting in
<gh-4427-ffi-sandwich/script.lua>, but this reduces the test
readability... At least, we need to call format twice in the test's
payload, so for better good is not to do it.

> +    test = 'is',
> +    message = 'Trace is aborted',
> +  },
> +  panic = {
> +    trigger = hotloop + 1,
> +    expected = 'Lua VM re%-entrancy is detected while executing the trace',
> +    test = 'like',
> +    message = 'Trace is compiled',
> +  },
> +}

<snipped>

> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua
> new file mode 100644
> index 00000000..9ecd964e
> --- /dev/null
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-351-print-tostring-number.test.lua b/test/tarantool-tests/lj-351-print-tostring-number.test.lua
> index da5b31be..72a9ec2b 100644
> --- a/test/tarantool-tests/lj-351-print-tostring-number.test.lua
> +++ b/test/tarantool-tests/lj-351-print-tostring-number.test.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-351-print-tostring-number/script.lua b/test/tarantool-tests/lj-351-print-tostring-number/script.lua
> new file mode 100644
> index 00000000..c3066f49

<snipped>

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

<snipped>

> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index c46b93f0..cf92757c 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua

<snipped>

> --- Save the current coroutine and set the value to trigger
> --- <flush> call the Lua routine instead of C implementation.
> -local flush = require('libflush')(cfg.trigger)
> +local script = utils.makecmd(arg, {
> +  -- TODO: Leave another toxic comment regarding SIP on macOS.

Minor: This TODO is unnecessary, so feel free to delete this line.

> +  env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
> +  redirect = '2>&1',
> +})

<snipped>

> diff --git a/test/tarantool-tests/lj-flush-on-trace/script.lua b/test/tarantool-tests/lj-flush-on-trace/script.lua
> new file mode 100644
> index 00000000..d2c35534
> --- /dev/null
> +++ b/test/tarantool-tests/lj-flush-on-trace/script.lua

<snipped>

> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index eb11d40d..41a7c22a 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua

<snipped>

> +function M.makecmd(arg, opts)

Please, add the comment about script location, it's good thing to
commment, IMHO.

> +  return setmetatable({
> +    LUABIN = M.luacmd(arg),
> +    SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'),
> +    ENV = opts and makeenv(opts.env) or '',

Is this option is used only for DYLD_LIBRARY_PATH (as I can see for now)?
Should we drop the comment about that here (that this is the main
reason)?

> +    REDIRECT = opts and opts.redirect or '',
> +  }, {
> +    __call = function(self, ...)
> +      local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self)
> +                  .. (' %s'):rep(select('#', ...)):format(...)

It's good to comment that we just format arguments to strings for the
script invocaiton.

> +      return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
> +    end
> +  })
>  end
>  
>  function M.skipcond(condition, message)
> -- 
> 2.30.2
> 

-- 
Best regards,
Sergey Kaplun