[Tarantool-patches] [PATCH v2 luajit 2/5] test: adjust lua-Harness suite for LuaJIT
skaplun at tarantool.org
Mon Mar 15 21:33:30 MSK 2021
Thanks for the review!
On 15.03.21, Igor Munkin wrote:
> 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  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(-)
> > 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
@@ -195,7 +195,8 @@ function todo (reason, count)
todo_reason = reason
--- 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,
More information about the Tarantool-patches