Re: [Tarantool-patches] [PATCH v2 luajit 2/5] test: adjust lua-Harness suite for LuaJIT

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

Igor,

Thanks for the review!

On 15.03.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch!
> 
> This is why it's so important to take the suite intact at first: I
> didn't even notice these changes in the previous series and now I regret
> a bit regarding the solution with LUAJIT_TEST_INIT: for this case it
> occurs to be ugly a little. Nevertheless, I believe Francois would be
> interested in this use case, so it's worth to file a bug against
> lua-Harness, IMHO.

I don't think so -- usage [1] declares the way to launch the suite.
It is our problem, that we don't use it.

> 
> On 15.03.21, Sergey Kaplun wrote:
> > LUAJIT_TEST_COMMAND always extends an amount of line argument by two
> > causing child test failures.
> 
> I don't get the line above. I guess you mind something similar to this:
> | LUAJIT_TEST_COMMAND always extends <arg> table containing command line
> | arguments by two. It leads to child test failures, since Lua
> | interpreter name is misfetched from the table.

Updated commit message considering your proposal.

> 
> > So, a new universal way to detect program name is required.
> > 
> > This patch introduces the new function `get_lua_binary_name()` inside
> > lua-Harness tap.lua module to get binary for test execution correctly.
> 
> Side note: I doubt the function location is right, but I guess there is
> no other place that is required by all test chunks.
> 
> > 
> > The following tests are adjusted with the new function:
> > * 241-standalone.t
> > * 242-luac.t
> > * 301-basic.t
> > * 308-io.t
> > * 309-os.t
> > * 320-stdin.t
> > * 411-luajit.t
> > 
> > Part of tarantool/tarantool#5844
> > Part of tarantool/tarantool#4473
> > ---
> >  test/lua-Harness-tests/241-standalone.t | 2 +-
> >  test/lua-Harness-tests/242-luac.t       | 2 +-
> >  test/lua-Harness-tests/301-basic.t      | 2 +-
> >  test/lua-Harness-tests/308-io.t         | 2 +-
> >  test/lua-Harness-tests/309-os.t         | 2 +-
> >  test/lua-Harness-tests/320-stdin.t      | 2 +-
> >  test/lua-Harness-tests/411-luajit.t     | 2 +-
> >  test/lua-Harness-tests/tap.lua          | 9 +++++++++
> >  8 files changed, 16 insertions(+), 7 deletions(-)
> > 
> 
> <snipped>
> 
> > diff --git a/test/lua-Harness-tests/tap.lua b/test/lua-Harness-tests/tap.lua
> > index 08a99b8..5ce95e6 100644
> > --- a/test/lua-Harness-tests/tap.lua
> > +++ b/test/lua-Harness-tests/tap.lua
> > @@ -195,6 +195,15 @@ function todo (reason, count)
> >      todo_reason = reason
> >  end
> >  
> > +-- The last arg element is guaranteed name of tested binary.
> 
> Typo: s/last/least/.
> Typo: s/guaranteed name of tested/guaranteed to be the name of the tested/.

Thanks, update the branch. See the iterative patch below.
===================================================================
diff --git a/test/lua-Harness-tests/tap.lua b/test/lua-Harness-tests/tap.lua
index 5ce95e6..e527687 100644
--- a/test/lua-Harness-tests/tap.lua
+++ b/test/lua-Harness-tests/tap.lua
@@ -195,7 +195,8 @@ function todo (reason, count)
     todo_reason = reason
 end
 
--- The last arg element is guaranteed name of tested binary.
+-- The least arg element is guaranteed to be the name
+-- of the tested binary.
 function get_lua_binary_name ()
     local i = 0
     while arg[i] do
===================================================================

> 
> > +function get_lua_binary_name ()
> > +    local i = 0
> > +    while arg[i] do
> 
> You make an excess lookup to a _G for each iteration. It's better to
> pass <arg> as an argument to this function. Furthermore, it makes the
> signature clearer, so user knows you're using <arg> to detect the
> interpreter name.

This economy looks unnecessary -- there are only 6 calls to this
function from the whole suite. You can provide time measurement
to prove me wrong.
I disagreed that it makes signature clearer, because there is no way to
use something instead `arg` variable anyway. Please, give me an
example, when usage of custom array is preferable than usage `arg`
variable. If this function *always* uses `arg` variable only, why we
should move it outside this function?

> 
> > +        i = i - 1
> > +    end
> > +    return arg[i + 1]
> > +end
> > +
> >  --
> >  -- Copyright (c) 2009-2018 Francois Perrad
> >  --
> > -- 
> > 2.28.0
> > 
> 
> -- 
> Best regards,
> IM

[1]: https://fperrad.frama.io/lua-Harness/usage/

-- 
Best regards,
Sergey Kaplun