Re: [Tarantool-patches] [PATCH luajit 2/3] test: make utils.selfrun usage easier

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

Hi!

LGTM with some ignorable nits below.

Sergos


> On 5 Apr 2021, at 20:11, Igor Munkin <imun@tarantool.org> wrote:
> 
> Previous implementation of <utils.selfrun> implicitly obliges the
> developer to pass the arguments to the test script for the second run.
> This has been unnoticed, since the existing tests are invoked for two
> different cases, so the script arguments were kinda obligatory, but they
> are not required in a general case.
> 
> As a result the function signals the testing system that the script is
> being run the second time via TEST_SELFRUN environment variable.
> 

To me it looks like it was an infinite recursion, that was interrupted
only by chance in case a test does not tolerate the reentrance. 


> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> .../gh-4427-ffi-sandwich.test.lua             | 51 +++++++++----------
> .../lj-flush-on-trace.test.lua                | 51 +++++++++----------
> test/tarantool-tests/utils.lua                |  7 +++
> 3 files changed, 57 insertions(+), 52 deletions(-)
> 
> diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> index 56363608..64df5dbd 100644
> --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> @@ -1,31 +1,30 @@
> -if #arg == 0 then
> -
> -  local utils = require('utils')
> -
> -  -- Disabled on *BSD due to #4819.
> -  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> -
> -  utils.selfrun(arg, {
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        1, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is aborted',
> -      res = tostring(3), -- hotloop + trigger + 1
> -      test = 'is',
> +local utils = require('utils')
> +
> +-- Disabled on *BSD due to #4819.
> +utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +
> +utils.selfrun(arg, {
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      1, -- trigger (arg[2])
>     },
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        2, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is recorded',
> -      res = 'Lua VM re%-entrancy is detected while executing the trace',
> -      test = 'like',
> +    msg = 'Trace is aborted',
> +    res = tostring(3), -- hotloop + trigger + 1
> +    test = 'is',
> +  },
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      2, -- trigger (arg[2])
>     },
> -  })
> -end
> +    msg = 'Trace is recorded',
> +    res = 'Lua VM re%-entrancy is detected while executing the trace',
> +    test = 'like',
> +  },
> +})
> +
> +----- Test payload. ------------------------------------------------------------
> 
> local cfg = {
>   hotloop = arg[1] or 1,
> diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> index 46fee533..edc5cf61 100644
> --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
> @@ -1,31 +1,30 @@
> -if #arg == 0 then
> -
> -  local utils = require('utils')
> -
> -  -- Disabled on *BSD due to #4819.
> -  utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> -
> -  utils.selfrun(arg, {
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        1, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is aborted',
> -      res = 'OK',
> -      test = 'is',
> +local utils = require('utils')
> +
> +-- Disabled on *BSD due to #4819.
> +utils.skipcond(jit.os == 'BSD', 'Disabled due to #4819')
> +
> +utils.selfrun(arg, {
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      1, -- trigger (arg[2])
>     },
> -    {
> -      arg = {
> -        1, -- hotloop (arg[1])
> -        2, -- trigger (arg[2])
> -      },
> -      msg = 'Trace is recorded',
> -      res = 'JIT mode change is detected while executing the trace',
> -      test = 'like',
> +    msg = 'Trace is aborted',
> +    res = 'OK',
> +    test = 'is',
> +  },
> +  {
> +    arg = {
> +      1, -- hotloop (arg[1])
> +      2, -- trigger (arg[2])
>     },
> -  })
> -end
> +    msg = 'Trace is recorded',
> +    res = 'JIT mode change is detected while executing the trace',
> +    test = 'like',
> +  },
> +})
> +
> +----- Test payload. ------------------------------------------------------------
> 
> local cfg = {
>   hotloop = arg[1] or 1,
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index aebbf6ac..d2dd71b0 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -14,6 +14,12 @@ local function luacmd(args)
> end
> 
> function M.selfrun(arg, checks)
> +  -- If TEST_SELFRUN is set, just execute the test payload below

To make a symmetry to the next comment: "we’re running the io.popen version
of the test, proceed with test payload execution past "

> +  -- <selfrun> call, ...
> +  if os.getenv('TEST_SELFRUN') then return end
> +
> +  -- ... otherwise run this chunk via <io.popen>.

I would add that the test payload won’t be run and the io.popen results
will be reported instead.

> +
>   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
> 
>   test:plan(#checks)
> @@ -28,6 +34,7 @@ function M.selfrun(arg, checks)
>   local cmd = string.gsub('LUA_PATH="<PATH>/?.lua;$LUA_PATH" ' ..
>                           'LUA_CPATH="<PATH>/?.<SUFFIX>;$LUA_CPATH" ' ..
>                           'LD_LIBRARY_PATH=<PATH>:$LD_LIBRARY_PATH ' ..
> +                          'TEST_SELFRUN=1' ..
>                           '<LUABIN> 2>&1 <SCRIPT>', '%<(%w+)>', vars)
> 
>   for _, ch in pairs(checks) do
> -- 
> 2.25.0
>