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

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

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

Great, thanks for explanation and comments update!

Sergos

> On 6 Apr 2021, at 21:32, Igor Munkin <imun@tarantool.org> wrote:
> 
> 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 <mailto: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 <mailto: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


[-- Attachment #2: Type: text/html, Size: 39891 bytes --]