[Tarantool-patches] [PATCH luajit 0/2] Enable CLI-related tests in lua-Harness

[Tarantool-patches] [PATCH luajit 0/2] Enable CLI-related tests in lua-Harness

[Igor Munkin via Tarantool-patches]

There are two reasons to apply this series to the trunk:

1. The positive one: Tarantool supports -b and -j options to use LuaJIT
modules since the commit bf8b76a4dfc9dd62d4131e90e2ae5d83843b6630 ("lua:
proxy -j and -b flags"), so the related tests from lua-Harness suite can
be partially (since -O is still not implemented in Tarantool) enabled.

2. The negative one: Tarantool diff-based tests for CLI interfaces are
hard to maintain if any change occurrs in LuaJIT modules, since the
aforementioned tests implement dumb comparison of the output, produced
by the current CLI version against the expected one, provided by the
.result file. Hence, to rule the tests related to LuaJIT CLI interface
in a more convenient way, the corresponding tests should be moved from
the tests in Tarantool repository to the tests in LuaJIT repository. The
branch enables the nice checks implemented in the lua-Harness suite; the
PR #9726 removes the unmaintainable diff-based tests in Tarantool repo.

The changeset is split into the two patches. One can find more reasoning
within the particular commit message.

Branch: https://github.com/tarantool/luajit/tree/imun/enable-tarantool-cli-tests-in-lua-Harness
Tarantool PR(*): https://github.com/tarantool/tarantool/pull/9726

(*) There is a Tarantool-related patch on top of the corresponding
LuaJIT bump.

Igor Munkin (2):
  test: introduce routine to build error message
  test: enable CLI-related lua-Harness tests back

 test/lua-Harness-tests/241-standalone.t |  2 +-
 test/lua-Harness-tests/411-luajit.t     | 23 +++++++++++++++++------
 2 files changed, 18 insertions(+), 7 deletions(-)

-- 
2.39.2

[Tarantool-patches] [PATCH luajit 1/2] test: introduce routine to build error message

[Igor Munkin via Tarantool-patches]

LuaJIT modules yields the plain errors, but LuaJIT binary adds 'luajit:'
prefix to it, so 411-luajit.t test in lua-Harness suite expects the
error message with the aforementioned prefix in the corresponding
assertions. At the same time, Tarantool prepends nothing to the error
produced by LuaJIT module.

To tweak the pattern to be used within the 411-luajit.t chunk, the
auxiliary error building function is introduced in this patch.

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/lua-Harness-tests/411-luajit.t | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t
index 6cfd6837..1b2da729 100755
--- a/test/lua-Harness-tests/411-luajit.t
+++ b/test/lua-Harness-tests/411-luajit.t
@@ -31,6 +31,15 @@ if not jit or ujit or _TARANTOOL then
     skip_all("only with LuaJIT")
 end
 
+-- XXX: Unfortunately, Lua patterns do not support optional
+-- capture groups, so the helper below implements poor man's
+-- optional capture groups for the patters matching LuaJIT CLI
+-- error messages.
+local function errbuild(message)
+    local eprefix = _TARANTOOL and "" or "[^:]+: "
+    return table.concat({"^", eprefix, message})
+end
+
 local lua = _retrieve_progname()
 
 if not pcall(io.popen, lua .. [[ -e "a=1"]]) then
@@ -158,13 +167,13 @@ f = io.popen(cmd)
 if compiled_with_jit then
     equals(f:read'*l', 'Hello World', "-jon")
 else
-    matches(f:read'*l', "^[^:]+: JIT compiler permanently disabled by build option", "no jit")
+    matches(f:read'*l', errbuild("JIT compiler permanently disabled by build option"), "no jit")
 end
 f:close()
 
 cmd = lua .. " -j bad hello-411.lua 2>&1"
 f = io.popen(cmd)
-matches(f:read'*l', "^[^:]+: unknown luaJIT command or jit%.%* modules not installed", "-j bad")
+matches(f:read'*l', errbuild("unknown luaJIT command or jit%.%* modules not installed"), "-j bad")
 f:close()
 
 if compiled_with_jit then
@@ -190,12 +199,12 @@ if compiled_with_jit then
 
     cmd = lua .. " -O+bad hello-411.lua 2>&1"
     f = io.popen(cmd)
-    matches(f:read'*l', "^[^:]+: unknown or malformed optimization flag '%+bad'", "-O+bad")
+    matches(f:read'*l', errbuild("unknown or malformed optimization flag '%+bad'"), "-O+bad")
     f:close()
 else
     cmd = lua .. " -O0 hello-411.lua 2>&1"
     f = io.popen(cmd)
-    matches(f:read'*l', "^[^:]+: attempt to index a nil value")
+    matches(f:read'*l', errbuild("attempt to index a nil value"))
     f:close()
 end
 
-- 
2.39.2

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce routine to build error message

[Maxim Kokryashkin via Tarantool-patches]

Hi, Igor!
Thanks for the patch!
LGTM, except for a few comments below.

On Sun, Feb 25, 2024 at 10:41:06AM +0000, Igor Munkin wrote:
> LuaJIT modules yields the plain errors, but LuaJIT binary adds 'luajit:'
Typo: s/yields/yield/
> prefix to it, so 411-luajit.t test in lua-Harness suite expects the
> error message with the aforementioned prefix in the corresponding
> assertions. At the same time, Tarantool prepends nothing to the error
Typo: s/prepends/adds/
Or, alternatively, you can say:
| Tarantool doesn't prepend errors produced by the LuaJIT module with
| anything.
> produced by LuaJIT module.
Typo: s/LuaJIT/the LuaJIT/
>
> To tweak the pattern to be used within the 411-luajit.t chunk, the
> auxiliary error building function is introduced in this patch.
>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/lua-Harness-tests/411-luajit.t | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t
> index 6cfd6837..1b2da729 100755
> --- a/test/lua-Harness-tests/411-luajit.t
> +++ b/test/lua-Harness-tests/411-luajit.t
> @@ -31,6 +31,15 @@ if not jit or ujit or _TARANTOOL then
>      skip_all("only with LuaJIT")
>  end
>
> +-- XXX: Unfortunately, Lua patterns do not support optional
> +-- capture groups, so the helper below implements poor man's
> +-- optional capture groups for the patters matching LuaJIT CLI
Typo: s/patters/patterns/
> +-- error messages.
> +local function errbuild(message)
> +    local eprefix = _TARANTOOL and "" or "[^:]+: "
IMO, `errprefix` or `err_prefix` would be better.
Feel free to ignore, though.
> +    return table.concat({"^", eprefix, message})
> +end
> +
>  local lua = _retrieve_progname()
>
>  if not pcall(io.popen, lua .. [[ -e "a=1"]]) then
> @@ -158,13 +167,13 @@ f = io.popen(cmd)
>  if compiled_with_jit then
>      equals(f:read'*l', 'Hello World', "-jon")
>  else
> -    matches(f:read'*l', "^[^:]+: JIT compiler permanently disabled by build option", "no jit")
> +    matches(f:read'*l', errbuild("JIT compiler permanently disabled by build option"), "no jit")
>  end
>  f:close()
>
>  cmd = lua .. " -j bad hello-411.lua 2>&1"
>  f = io.popen(cmd)
> -matches(f:read'*l', "^[^:]+: unknown luaJIT command or jit%.%* modules not installed", "-j bad")
> +matches(f:read'*l', errbuild("unknown luaJIT command or jit%.%* modules not installed"), "-j bad")
>  f:close()
>
>  if compiled_with_jit then
> @@ -190,12 +199,12 @@ if compiled_with_jit then
>
>      cmd = lua .. " -O+bad hello-411.lua 2>&1"
>      f = io.popen(cmd)
> -    matches(f:read'*l', "^[^:]+: unknown or malformed optimization flag '%+bad'", "-O+bad")
> +    matches(f:read'*l', errbuild("unknown or malformed optimization flag '%+bad'"), "-O+bad")
>      f:close()
>  else
>      cmd = lua .. " -O0 hello-411.lua 2>&1"
>      f = io.popen(cmd)
> -    matches(f:read'*l', "^[^:]+: attempt to index a nil value")
> +    matches(f:read'*l', errbuild("attempt to index a nil value"))
>      f:close()
>  end
>
> --
> 2.39.2
>

Re: [Tarantool-patches] [PATCH luajit 1/2] test: introduce routine to build error message

[Igor Munkin via Tarantool-patches]

Max,

Thanks for your review! I've fixed all the nits you've left, the branch
is force-pushed.

On 25.02.24, Maxim Kokryashkin wrote:
> Hi, Igor!
> Thanks for the patch!
> LGTM, except for a few comments below.
> 
> On Sun, Feb 25, 2024 at 10:41:06AM +0000, Igor Munkin wrote:
> > LuaJIT modules yields the plain errors, but LuaJIT binary adds 'luajit:'
> Typo: s/yields/yield/

Fixed, thanks!

> > prefix to it, so 411-luajit.t test in lua-Harness suite expects the
> > error message with the aforementioned prefix in the corresponding
> > assertions. At the same time, Tarantool prepends nothing to the error
> Typo: s/prepends/adds/
> Or, alternatively, you can say:
> | Tarantool doesn't prepend errors produced by the LuaJIT module with
> | anything.

Thanks, chose yours wording.

> > produced by LuaJIT module.
> Typo: s/LuaJIT/the LuaJIT/

Fixed by your wording.

> >
> > To tweak the pattern to be used within the 411-luajit.t chunk, the
> > auxiliary error building function is introduced in this patch.
> >
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  test/lua-Harness-tests/411-luajit.t | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t
> > index 6cfd6837..1b2da729 100755
> > --- a/test/lua-Harness-tests/411-luajit.t
> > +++ b/test/lua-Harness-tests/411-luajit.t
> > @@ -31,6 +31,15 @@ if not jit or ujit or _TARANTOOL then
> >      skip_all("only with LuaJIT")
> >  end
> >
> > +-- XXX: Unfortunately, Lua patterns do not support optional
> > +-- capture groups, so the helper below implements poor man's
> > +-- optional capture groups for the patters matching LuaJIT CLI
> Typo: s/patters/patterns/

Fixed, thanks!

> > +-- error messages.
> > +local function errbuild(message)
> > +    local eprefix = _TARANTOOL and "" or "[^:]+: "
> IMO, `errprefix` or `err_prefix` would be better.
> Feel free to ignore, though.

No problem, fixed. The diff is below:

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

diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t
index 414e6a57..40241db3 100755
--- a/test/lua-Harness-tests/411-luajit.t
+++ b/test/lua-Harness-tests/411-luajit.t
@@ -36,8 +36,8 @@ end
 -- optional capture groups for the patters matching LuaJIT CLI
 -- error messages.
 local function errbuild(message)
-    local eprefix = _TARANTOOL and "" or "[^:]+: "
-    return table.concat({"^", eprefix, message})
+    local errprefix = _TARANTOOL and "" or "[^:]+: "
+    return table.concat({"^", errprefix, message})
 end
 
 local lua = _retrieve_progname()

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

> > +    return table.concat({"^", eprefix, message})
> > +end
> > +
> >  local lua = _retrieve_progname()
> >
> >  if not pcall(io.popen, lua .. [[ -e "a=1"]]) then

<snipped>

> > --
> > 2.39.2
> >

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 2/2] test: enable CLI-related lua-Harness tests back

[Igor Munkin via Tarantool-patches]

Tarantool supports -b and -j options to use LuaJIT modules since the
commit bf8b76a4dfc9dd62d4131e90e2ae5d83843b6630 ("lua: proxy -j and -b
flags"), so 241-standalone.t and 411-luajit.t tests in lua-Harness
suite, disabled in the commit 39a4db500db2619359c2c9474be016360252060d
("test: support Tarantool in lua-Harness"), can be enabled back.

However, -O options is still not implemented in Tarantool, so the
related part in 411-luajit.t test chunk is still disabled.

Follows up #5541

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/lua-Harness-tests/241-standalone.t | 2 +-
 test/lua-Harness-tests/411-luajit.t     | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t
index 5b353491..aa011d3b 100755
--- a/test/lua-Harness-tests/241-standalone.t
+++ b/test/lua-Harness-tests/241-standalone.t
@@ -29,7 +29,7 @@ L<https://www.lua.org/manual/5.4/manual.html#7>
 --]]
 
 require'test_assertion'
-local has_bytecode = not ujit and not ravi and not _TARANTOOL
+local has_bytecode = not ujit and not ravi
 local has_error52 = _VERSION >= 'Lua 5.2'
 local has_error53 = _VERSION >= 'Lua 5.3'
 local has_opt_E = _VERSION >= 'Lua 5.2' or (jit and not _TARANTOOL)
diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t
index 1b2da729..414e6a57 100755
--- a/test/lua-Harness-tests/411-luajit.t
+++ b/test/lua-Harness-tests/411-luajit.t
@@ -27,7 +27,7 @@ See L<https://luajit.org/running.html>
 require'test_assertion'
 local profile = require'profile'
 
-if not jit or ujit or _TARANTOOL then
+if not jit or ujit then
     skip_all("only with LuaJIT")
 end
 
@@ -176,7 +176,9 @@ f = io.popen(cmd)
 matches(f:read'*l', errbuild("unknown luaJIT command or jit%.%* modules not installed"), "-j bad")
 f:close()
 
-if compiled_with_jit then
+if _TARANTOOL then
+    skip("-O is not yet implemented in Tarantool")
+elseif compiled_with_jit then
     cmd = lua .. " -O hello-411.lua"
     f = io.popen(cmd)
     equals(f:read'*l', 'Hello World', "-O")
-- 
2.39.2

Re: [Tarantool-patches] [PATCH luajit 2/2] test: enable CLI-related lua-Harness tests back

[Maxim Kokryashkin via Tarantool-patches]

Hi, Igor!
Thanks for the patch!
LGTM after fixing comments below.
On Sun, Feb 25, 2024 at 10:41:07AM +0000, Igor Munkin wrote:
> Tarantool supports -b and -j options to use LuaJIT modules since the
> commit bf8b76a4dfc9dd62d4131e90e2ae5d83843b6630 ("lua: proxy -j and -b
> flags"), so 241-standalone.t and 411-luajit.t tests in lua-Harness
Typo: s/in lua-Harness/in the lua-Harness/
> suite, disabled in the commit 39a4db500db2619359c2c9474be016360252060d
> ("test: support Tarantool in lua-Harness"), can be enabled back.
>
> However, -O options is still not implemented in Tarantool, so the
Typo: s/is still/are still/
> related part in 411-luajit.t test chunk is still disabled.
Typo: s/in/in the/
>
> Follows up #5541
Typo: s/#5541/tarantool/tarantool#5541/
>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/lua-Harness-tests/241-standalone.t | 2 +-
>  test/lua-Harness-tests/411-luajit.t     | 6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/test/lua-Harness-tests/241-standalone.t b/test/lua-Harness-tests/241-standalone.t
> index 5b353491..aa011d3b 100755
> --- a/test/lua-Harness-tests/241-standalone.t
> +++ b/test/lua-Harness-tests/241-standalone.t
> @@ -29,7 +29,7 @@ L<https://www.lua.org/manual/5.4/manual.html#7>
>  --]]
>
>  require'test_assertion'
> -local has_bytecode = not ujit and not ravi and not _TARANTOOL
> +local has_bytecode = not ujit and not ravi
>  local has_error52 = _VERSION >= 'Lua 5.2'
>  local has_error53 = _VERSION >= 'Lua 5.3'
>  local has_opt_E = _VERSION >= 'Lua 5.2' or (jit and not _TARANTOOL)
> diff --git a/test/lua-Harness-tests/411-luajit.t b/test/lua-Harness-tests/411-luajit.t
> index 1b2da729..414e6a57 100755
> --- a/test/lua-Harness-tests/411-luajit.t
> +++ b/test/lua-Harness-tests/411-luajit.t
> @@ -27,7 +27,7 @@ See L<https://luajit.org/running.html>
>  require'test_assertion'
>  local profile = require'profile'
>
> -if not jit or ujit or _TARANTOOL then
> +if not jit or ujit then
>      skip_all("only with LuaJIT")
>  end
>
> @@ -176,7 +176,9 @@ f = io.popen(cmd)
>  matches(f:read'*l', errbuild("unknown luaJIT command or jit%.%* modules not installed"), "-j bad")
>  f:close()
>
> -if compiled_with_jit then
> +if _TARANTOOL then
> +    skip("-O is not yet implemented in Tarantool")
> +elseif compiled_with_jit then
>      cmd = lua .. " -O hello-411.lua"
>      f = io.popen(cmd)
>      equals(f:read'*l', 'Hello World', "-O")
> --
> 2.39.2
>

Re: [Tarantool-patches] [PATCH luajit 2/2] test: enable CLI-related lua-Harness tests back

[Igor Munkin via Tarantool-patches]

Max,

Thanks for the patch!

On 25.02.24, Maxim Kokryashkin wrote:
> Hi, Igor!
> Thanks for the patch!
> LGTM after fixing comments below.
> On Sun, Feb 25, 2024 at 10:41:07AM +0000, Igor Munkin wrote:
> > Tarantool supports -b and -j options to use LuaJIT modules since the
> > commit bf8b76a4dfc9dd62d4131e90e2ae5d83843b6630 ("lua: proxy -j and -b
> > flags"), so 241-standalone.t and 411-luajit.t tests in lua-Harness
> Typo: s/in lua-Harness/in the lua-Harness/

Fixed, thanks!

> > suite, disabled in the commit 39a4db500db2619359c2c9474be016360252060d
> > ("test: support Tarantool in lua-Harness"), can be enabled back.
> >
> > However, -O options is still not implemented in Tarantool, so the
> Typo: s/is still/are still/

There is another typo: s/options/option/. Fixed, thanks!

> > related part in 411-luajit.t test chunk is still disabled.
> Typo: s/in/in the/

Fixed, thanks!

> >
> > Follows up #5541
> Typo: s/#5541/tarantool/tarantool#5541/

Oops, fixed, thanks!

Side note: you can use another separator to avoid escaping the /. E.g.
braces: s{#5541}{tarantool/tarantool#5541}. However, I'm not a robot, so
the comment is still clear to me ;)

> >
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  test/lua-Harness-tests/241-standalone.t | 2 +-
> >  test/lua-Harness-tests/411-luajit.t     | 6 ++++--
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >

<snipped>

> > --
> > 2.39.2
> >

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/2] test: enable CLI-related lua-Harness tests back

[Igor Munkin via Tarantool-patches]

Max,

Thanks for your **review**! I've fixed all the nits you've left, the branch
is force-pushed.

Side note: I guess I made very little patches and merged too many series
for the last year considering the typo below...

On 25.02.24, Igor Munkin via Tarantool-patches wrote:
> Max,
> 
> Thanks for the patch!
                 ^^^^^
> 

<snipped>

> 
> Oops, fixed, thanks!
> 
> Side note: you can use another separator to avoid escaping the /. E.g.
> braces: s{#5541}{tarantool/tarantool#5541}. However, I'm not a robot, so
                                                       ^^^^^^^^^^^^^^^
                                                       ... or am I?
> the comment is still clear to me ;)
> 

<snipped>

> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 0/2] Enable CLI-related tests in lua-Harness

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the series, LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 0/2] Enable CLI-related tests in lua-Harness

[Sergey Kaplun via Tarantool-patches]

Igor,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.0 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/9737
[2]: https://github.com/tarantool/tarantool/pull/9738
[3]: https://github.com/tarantool/tarantool/pull/9739

-- 
Best regards,
Sergey Kaplun