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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergos,

Thanks for your review!

On 06.04.21, Sergey Ostanevich wrote:
> Hi!
> 
> LGTM with some ignorable nits below.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

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

Precisely. Honestly, it was a "soft" requirement for such tests (as I
have written above), but considering your attempts with implementing
tests for JIT machinery, I've finally realized this problem.

> 
> 
> > 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(-)

<snipped>

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

I've adjusted the comments the following way:

================================================================================

diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index 9bdb71ec..c0403cf1 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -25,11 +25,15 @@ local function unshiftenv(variable, value, sep)
 end
 
 function M.selfrun(arg, checks)
-  -- If TEST_SELFRUN is set, just execute the test payload below
-  -- <selfrun> call, ...
+  -- If TEST_SELFRUN is set, it means the test has been run via
+  -- <io.popen>, so just return from this routine and proceed
+  -- the execution to the test payload, ...
   if os.getenv('TEST_SELFRUN') then return end
 
-  -- ... otherwise run this chunk via <io.popen>.
+  -- ... otherwise initialize <tap>, setup testing environment
+  -- and run this chunk via <io.popen> for each case in <checks>.
+  -- XXX: The function doesn't return back from this moment. It
+  -- checks whether all assertions are fine and exits.
 
   local test = tap.test(arg[0]:match('/?(.+)%.test%.lua'))
 

================================================================================

> 
> > +
> >   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
> > 
> 

-- 
Best regards,
IM