[Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C

[Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C

[Igor Munkin]

This series prepares the existing testing machinery to run LuaJIT tests
requiring libraries implemented in C and enables the existing ones:
* gh-4427-ffi-sandwich
* lj-flush-on-trace

Igor Munkin (4):
  luajit: bump new version
  test: adjust luajit-tap testing machinery
  test: enable luajit-tap:gh-4427-ffi-sandwich tests
  test: enable luajit-tap:lj-flush-on-trace tests

 test/CMakeLists.txt                        | 17 +++++++-----
 test/app-tap/gh-4427-ffi-sandwich.test.lua | 30 ++++++++++++++++++++++
 test/app-tap/lj-flush-on-trace.test.lua    | 30 ++++++++++++++++++++++
 third_party/luajit                         |  2 +-
 4 files changed, 72 insertions(+), 7 deletions(-)
 create mode 100755 test/app-tap/gh-4427-ffi-sandwich.test.lua
 create mode 100755 test/app-tap/lj-flush-on-trace.test.lua

-- 
2.25.0

[Tarantool-patches] [PATCH 1/4] luajit: bump new version

[Igor Munkin]

- jit: abort trace execution on JIT mode change
- jit: abort trace recording and execution for C API

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

This patch is just a reminder to bump luajit submodule.

 third_party/luajit | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/third_party/luajit b/third_party/luajit
index cf8475858..a96915f2d 160000
--- a/third_party/luajit
+++ b/third_party/luajit
@@ -1 +1 @@
-Subproject commit cf8475858861d28d1ea6d4de15e146370b3a24f2
+Subproject commit a96915f2d6d621d344e27ae21d5420c7fb0f1b3a
-- 
2.25.0

[Tarantool-patches] [PATCH 2/4] test: adjust luajit-tap testing machinery

[Igor Munkin]

This changeset makes possible to run luajit-tap tests requiring
libraries implemented in C:
* symlink to luajit test is created on configuration phase instead of
  build one.
* introduced a CMake function for building shared libraries required for
  luajit tests.

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/CMakeLists.txt | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 9b5df7dc5..00fce7270 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -13,6 +13,13 @@ function(build_module module files)
     endif(TARGET_OS_DARWIN)
 endfunction()
 
+function(build_lualib lib sources)
+    add_library(${lib} SHARED ${sources})
+    set_target_properties(${lib} PROPERTIES PREFIX "")
+    if(TARGET_OS_DARWIN)
+        set_target_properties(${lib} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
+    endif(TARGET_OS_DARWIN)
+endfunction()
 
 add_compile_flags("C;CXX"
     "-Wno-unused-parameter")
@@ -21,13 +28,9 @@ if(POLICY CMP0037)
     cmake_policy(SET CMP0037 OLD)
 endif(POLICY CMP0037)
 
-add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap
-    COMMAND ${CMAKE_COMMAND} -E create_symlink
+execute_process(COMMAND ${CMAKE_COMMAND} -E create_symlink
         ${PROJECT_SOURCE_DIR}/third_party/luajit/test
-        ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap
-        COMMENT Create a symlink for luajit test dir to fix out-of-source tests)
-add_custom_target(symlink_luajit_tests ALL
-   DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap)
+        ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap)
 
 add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/small
     COMMAND ${CMAKE_COMMAND} -E create_symlink
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 2/4] test: adjust luajit-tap testing machinery

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 27/03/2020 14:23, Igor Munkin wrote:
> This changeset makes possible to run luajit-tap tests requiring
> libraries implemented in C:
> * symlink to luajit test is created on configuration phase instead of
>   build one.

What is the difference? Is it really necessary for anything? To be
able to see CMake files in there?

> * introduced a CMake function for building shared libraries required for
>   luajit tests.
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/CMakeLists.txt | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 9b5df7dc5..00fce7270 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -13,6 +13,13 @@ function(build_module module files)
>      endif(TARGET_OS_DARWIN)
>  endfunction()
>  
> +function(build_lualib lib sources)

This function is introduced in tarantool, but required
for luajit. On the contrary, luajit is required for tarantool.
This is cyclic dependency, which makes impossible to run such
tests inside luajit. I think luajit tests should be kept self
sufficient. Otherwise it makes no sense to keep them in luajit
repository. If you anyway can't run them in there, and need to
create some test-wrappers from tarantool's side.

Alternative is to move them completely to tarantool. But current
way is not right. It is half of one and half of other solution.

> +    add_library(${lib} SHARED ${sources})
> +    set_target_properties(${lib} PROPERTIES PREFIX "")
> +    if(TARGET_OS_DARWIN)
> +        set_target_properties(${lib} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
> +    endif(TARGET_OS_DARWIN)
> +endfunction()

Re: [Tarantool-patches] [PATCH 2/4] test: adjust luajit-tap testing machinery

[Igor Munkin]

Vlad,

Thanks for your review!

On 05.04.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 27/03/2020 14:23, Igor Munkin wrote:
> > This changeset makes possible to run luajit-tap tests requiring
> > libraries implemented in C:
> > * symlink to luajit test is created on configuration phase instead of
> >   build one.
> 
> What is the difference? Is it really necessary for anything? To be
> able to see CMake files in there?

Exactly. I hoisted symlink creation to add Cmake subdirectories in the
further patches.

> 
> > * introduced a CMake function for building shared libraries required for
> >   luajit tests.
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  test/CMakeLists.txt | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index 9b5df7dc5..00fce7270 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -13,6 +13,13 @@ function(build_module module files)
> >      endif(TARGET_OS_DARWIN)
> >  endfunction()
> >  
> > +function(build_lualib lib sources)
> 
> This function is introduced in tarantool, but required
> for luajit. On the contrary, luajit is required for tarantool.
> This is cyclic dependency, which makes impossible to run such
> tests inside luajit. I think luajit tests should be kept self
> sufficient. Otherwise it makes no sense to keep them in luajit
> repository. If you anyway can't run them in there, and need to
> create some test-wrappers from tarantool's side.
> 
> Alternative is to move them completely to tarantool. But current
> way is not right. It is half of one and half of other solution.

We already moved them out from the tarantool repo in the scope of this
issue[1]. However, as you can see, we moved them only partially. This
fact also makes me frustrating and I'm going to rework LuaJIT test
infrastructure in the scope of the separate ticket[2].

> 
> > +    add_library(${lib} SHARED ${sources})
> > +    set_target_properties(${lib} PROPERTIES PREFIX "")
> > +    if(TARGET_OS_DARWIN)
> > +        set_target_properties(${lib} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup")
> > +    endif(TARGET_OS_DARWIN)
> > +endfunction()

[1]: https://github.com/tarantool/tarantool/issues/4478
[2]: https://github.com/tarantool/tarantool/issues/4862

-- 
Best regards,
IM

[Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests

[Igor Munkin]

The test is added as a CMake subdirectory and the runner executes
tarantool test.lua command via io.popen to check whether the platform
successfully finishes the execution or platform panic occurs.

Follow-up #4427

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/CMakeLists.txt                        |  1 +
 test/app-tap/gh-4427-ffi-sandwich.test.lua | 30 ++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100755 test/app-tap/gh-4427-ffi-sandwich.test.lua

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 00fce7270..bceea4567 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -59,6 +59,7 @@ add_subdirectory(app)
 add_subdirectory(app-tap)
 add_subdirectory(box)
 add_subdirectory(unit)
+add_subdirectory(luajit-tap/gh-4427-ffi-sandwich)
 
 # Move tarantoolctl config
 if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
new file mode 100755
index 000000000..602af88d4
--- /dev/null
+++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+
+local test = tap.test('gh-4427-ffi-sandwich')
+
+local cmd = string.gsub(
+  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
+  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
+
+local checks = {
+  { hotloop = 1, trigger = 1, success = true  },
+  { hotloop = 1, trigger = 2, success = false },
+}
+
+test:plan(#checks)
+
+for _, ch in pairs(checks) do
+  local res
+  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
+  for s in proc:lines('*l') do res = s end
+  assert(res, 'proc:lines failed')
+  if ch.success then
+    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
+  else
+    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
+  end
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests

[Igor Munkin]

Fixed the issue with shared library suffix. The diff is below:

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

diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
index 602af88d4..829c129a0 100755
--- a/test/app-tap/gh-4427-ffi-sandwich.test.lua
+++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
@@ -4,9 +4,13 @@ local tap = require('tap')
 
 local test = tap.test('gh-4427-ffi-sandwich')
 
-local cmd = string.gsub(
-  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
-  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
+local vars = {
+  PATH   = os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich',
+  SUFFIX = package.cpath:match('?.(%a+);'),
+}
+
+local cmd = string.gsub('LUA_CPATH=<PATH>/?.<SUFFIX> LD_LIBRARY_PATH=<PATH> '
+  .. 'tarantool 2>&1 <PATH>/test.lua %d %d', '%<(%w+)>', vars)
 
 local checks = {
   { hotloop = 1, trigger = 1, success = true  },

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

Also added a skipcond for FreeBSD:

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

diff --git a/test/app-tap/gh-4427-ffi-sandwich.skipcond
b/test/app-tap/gh-4427-ffi-sandwich.skipcond
new file mode 100644
index 000000000..2a2ec4d97
--- /dev/null
+++ b/test/app-tap/gh-4427-ffi-sandwich.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:

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

On 27.03.20, Igor Munkin wrote:
> The test is added as a CMake subdirectory and the runner executes
> tarantool test.lua command via io.popen to check whether the platform
> successfully finishes the execution or platform panic occurs.
> 
> Follow-up #4427
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/CMakeLists.txt                        |  1 +
>  test/app-tap/gh-4427-ffi-sandwich.test.lua | 30 ++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>  create mode 100755 test/app-tap/gh-4427-ffi-sandwich.test.lua
> 
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 00fce7270..bceea4567 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -59,6 +59,7 @@ add_subdirectory(app)
>  add_subdirectory(app-tap)
>  add_subdirectory(box)
>  add_subdirectory(unit)
> +add_subdirectory(luajit-tap/gh-4427-ffi-sandwich)
>  
>  # Move tarantoolctl config
>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> new file mode 100755
> index 000000000..602af88d4
> --- /dev/null
> +++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> @@ -0,0 +1,30 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test('gh-4427-ffi-sandwich')
> +
> +local cmd = string.gsub(
> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
> +
> +local checks = {
> +  { hotloop = 1, trigger = 1, success = true  },
> +  { hotloop = 1, trigger = 2, success = false },
> +}
> +
> +test:plan(#checks)
> +
> +for _, ch in pairs(checks) do
> +  local res
> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> +  for s in proc:lines('*l') do res = s end
> +  assert(res, 'proc:lines failed')
> +  if ch.success then
> +    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
> +  else
> +    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
> +  end
> +end
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests

[Vladislav Shpilevoy]

>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> new file mode 100755
> index 000000000..602af88d4
> --- /dev/null
> +++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> @@ -0,0 +1,30 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test('gh-4427-ffi-sandwich')
> +
> +local cmd = string.gsub(
> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
> +
> +local checks = {
> +  { hotloop = 1, trigger = 1, success = true  },
> +  { hotloop = 1, trigger = 2, success = false },

Why hotloop is needed, if it is always 1?

> +}
> +
> +test:plan(#checks)
> +
> +for _, ch in pairs(checks) do
> +  local res
> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> +  for s in proc:lines('*l') do res = s end

What is '*l'?

> +  assert(res, 'proc:lines failed')
> +  if ch.success then
> +    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
> +  else
> +    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
> +  end
> +end
> +
> +os.exit(test:check() and 0 or 1)

Oh God. So luajit already contains cyclic dependencies. It stores suite.ini
file in its test folder, which of course is useless for the luajit alone.

Since suite.ini file is here, why do you need to run tests from there in such
a complex way? Test-run should already be able to peek tests from there (and
it does). It probably does not work for your test only because you put it into
a folder. I propose you to flatten it, like it is done for function1.test.lua.
You still can keep issue name in form of 'gh-####-' in file names to keep them
related. They would be

    gh-4427-ffi-sandwich.test.lua
    gh-4427-ffi-sandwich.c

And after build:

+   gh-4427-ffi.sandwich.so/.dylib

Then you don't a need separate file in app-tap.

Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests

[Igor Munkin]

Vlad,

Thanks for your review!

On 05.04.20, Vladislav Shpilevoy wrote:
> >  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> > diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> > new file mode 100755
> > index 000000000..602af88d4
> > --- /dev/null
> > +++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> > @@ -0,0 +1,30 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local tap = require('tap')
> > +
> > +local test = tap.test('gh-4427-ffi-sandwich')
> > +
> > +local cmd = string.gsub(
> > +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> > +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
> > +
> > +local checks = {
> > +  { hotloop = 1, trigger = 1, success = true  },
> > +  { hotloop = 1, trigger = 2, success = false },
> 
> Why hotloop is needed, if it is always 1?

I decided to localize instance configuration values and make an explicit
hotloop parameter. Since the result of the tests depends on hotloop and
trigger values ratio, it seems more convenient for further maintainence
to store these params nearby each other.

> 
> > +}
> > +
> > +test:plan(#checks)
> > +
> > +for _, ch in pairs(checks) do
> > +  local res
> > +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> > +  for s in proc:lines('*l') do res = s end
> 
> What is '*l'?

Considering the doc[1] this option makes <lines> iterator read the
stream line by line (the options <lines> passes to its iterator function
are exactly the same as read accepts).

> 
> > +  assert(res, 'proc:lines failed')
> > +  if ch.success then
> > +    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
> > +  else
> > +    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
> > +  end
> > +end
> > +
> > +os.exit(test:check() and 0 or 1)
> 
> Oh God. So luajit already contains cyclic dependencies. It stores suite.ini
> file in its test folder, which of course is useless for the luajit alone.
> 
> Since suite.ini file is here, why do you need to run tests from there in such
> a complex way? Test-run should already be able to peek tests from there (and
> it does). It probably does not work for your test only because you put it into
> a folder. I propose you to flatten it, like it is done for function1.test.lua.
> You still can keep issue name in form of 'gh-####-' in file names to keep them
> related. They would be
> 
>     gh-4427-ffi-sandwich.test.lua
>     gh-4427-ffi-sandwich.c
> 
> And after build:
> 
> +   gh-4427-ffi.sandwich.so/.dylib
> 
> Then you don't a need separate file in app-tap.

A separate file in app-tap is a runner for the chunk in luajit-tap
tests. Since I need to test a platform failure, I don't know other way
than run one Lua script via io.popen from another one.

So if my math is OK we still have two Lua chunks and a C one:
* the Lua runner with io.popen and output analyzer
* the Lua script to be run by Tarantool instance
* the Dynamic library to be required by the latter Lua script and to be
  build from the corresponding C file

Could you please clarify your proposal a bit if I'm wrong or missing
something?

[1]: https://www.lua.org/manual/5.1/manual.html#pdf-file:read

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests

[Vladislav Shpilevoy]

Hi!

>>>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
>>> diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
>>> new file mode 100755
>>> index 000000000..602af88d4
>>> --- /dev/null
>>> +++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
>>> @@ -0,0 +1,30 @@
>>> +#!/usr/bin/env tarantool
>>> +
>>> +local tap = require('tap')
>>> +
>>> +local test = tap.test('gh-4427-ffi-sandwich')
>>> +
>>> +local cmd = string.gsub(
>>> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
>>> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
>>> +
>>> +local checks = {
>>> +  { hotloop = 1, trigger = 1, success = true  },
>>> +  { hotloop = 1, trigger = 2, success = false },
>>
>> Why hotloop is needed, if it is always 1?
> 
> I decided to localize instance configuration values and make an explicit
> hotloop parameter. Since the result of the tests depends on hotloop and
> trigger values ratio, it seems more convenient for further maintainence
> to store these params nearby each other.

After the private talk I know that hotloop is some kind of thing affecting
when to start recording a trace. It is worth leaving a comment on that.
And on what is trigger (about this I still don't know).

>>> +}
>>> +
>>> +test:plan(#checks)
>>> +
>>> +for _, ch in pairs(checks) do
>>> +  local res
>>> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
>>> +  for s in proc:lines('*l') do res = s end
>>
>> What is '*l'?
> 
> Considering the doc[1] this option makes <lines> iterator read the
> stream line by line (the options <lines> passes to its iterator function
> are exactly the same as read accepts).

In the [1] I see that lines() does not take any arguments. file:read()
does. file:lines() does not. Even for file:read() '*l' is default and
can be omitted.

>>> +  assert(res, 'proc:lines failed')
>>> +  if ch.success then
>>> +    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
>>> +  else
>>> +    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
>>> +  end
>>> +end
>>> +
>>> +os.exit(test:check() and 0 or 1)
>>
>> Oh God. So luajit already contains cyclic dependencies. It stores suite.ini
>> file in its test folder, which of course is useless for the luajit alone.
>>
>> Since suite.ini file is here, why do you need to run tests from there in such
>> a complex way? Test-run should already be able to peek tests from there (and
>> it does). It probably does not work for your test only because you put it into
>> a folder. I propose you to flatten it, like it is done for function1.test.lua.
>> You still can keep issue name in form of 'gh-####-' in file names to keep them
>> related. They would be
>>
>>     gh-4427-ffi-sandwich.test.lua
>>     gh-4427-ffi-sandwich.c
>>
>> And after build:
>>
>> +   gh-4427-ffi.sandwich.so/.dylib
>>
>> Then you don't a need separate file in app-tap.
> 
> A separate file in app-tap is a runner for the chunk in luajit-tap
> tests. Since I need to test a platform failure, I don't know other way
> than run one Lua script via io.popen from another one.
> 
> So if my math is OK we still have two Lua chunks and a C one:
> * the Lua runner with io.popen and output analyzer
> * the Lua script to be run by Tarantool instance
> * the Dynamic library to be required by the latter Lua script and to be
>   build from the corresponding C file
> 
> Could you please clarify your proposal a bit if I'm wrong or missing
> something?

The problem is that you have one test located in 2 folders and 3 files
now. I propose to make it just 3 files. We never do any test hierarchies.
Also test-run does not support nested folders with tests.

Strictly speaking, you can make it 2 files: C and Lua. In Lua file you
do io.popen on self with an argument, which would mean the file was started
not by test-run, and which would launch the 'platform failure test'.

In that way you will have all test code located in one place (not counting
C file).

    if arg[3] == 'do_test' then
        jit.opt, C, print, ...
        os.exit()
    end

    -- arg[1] - tarantool, arg[2] - this file
    io.popen(string.format('%s %s ...', arg[1], arg[2], 'do_test'))

    test:is(...)
    ...

> [1]: https://www.lua.org/manual/5.1/manual.html#pdf-file:read

Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests

[Igor Munkin]

Vlad,

Thanks for your review!

On 10.04.20, Vladislav Shpilevoy wrote:
> Hi!
> 
> >>>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> >>> diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> >>> new file mode 100755
> >>> index 000000000..602af88d4
> >>> --- /dev/null
> >>> +++ b/test/app-tap/gh-4427-ffi-sandwich.test.lua
> >>> @@ -0,0 +1,30 @@
> >>> +#!/usr/bin/env tarantool
> >>> +
> >>> +local tap = require('tap')
> >>> +
> >>> +local test = tap.test('gh-4427-ffi-sandwich')
> >>> +
> >>> +local cmd = string.gsub(
> >>> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> >>> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich')
> >>> +
> >>> +local checks = {
> >>> +  { hotloop = 1, trigger = 1, success = true  },
> >>> +  { hotloop = 1, trigger = 2, success = false },
> >>
> >> Why hotloop is needed, if it is always 1?
> > 
> > I decided to localize instance configuration values and make an explicit
> > hotloop parameter. Since the result of the tests depends on hotloop and
> > trigger values ratio, it seems more convenient for further maintainence
> > to store these params nearby each other.
> 
> After the private talk I know that hotloop is some kind of thing affecting
> when to start recording a trace. It is worth leaving a comment on that.
> And on what is trigger (about this I still don't know).

There are lots of comments in the corresponding C file regarding the
trigger parameter. Please let me know, if those mentions aren't enough.

> 
> >>> +}
> >>> +
> >>> +test:plan(#checks)
> >>> +
> >>> +for _, ch in pairs(checks) do
> >>> +  local res
> >>> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> >>> +  for s in proc:lines('*l') do res = s end
> >>
> >> What is '*l'?
> > 
> > Considering the doc[1] this option makes <lines> iterator read the
> > stream line by line (the options <lines> passes to its iterator function
> > are exactly the same as read accepts).
> 
> In the [1] I see that lines() does not take any arguments. file:read()
> does. file:lines() does not. Even for file:read() '*l' is default and
> can be omitted.

Agree, removed.

> 
> >>> +  assert(res, 'proc:lines failed')
> >>> +  if ch.success then
> >>> +    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
> >>> +  else
> >>> +    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
> >>> +  end
> >>> +end
> >>> +
> >>> +os.exit(test:check() and 0 or 1)
> >>
> >> Oh God. So luajit already contains cyclic dependencies. It stores suite.ini
> >> file in its test folder, which of course is useless for the luajit alone.
> >>
> >> Since suite.ini file is here, why do you need to run tests from there in such
> >> a complex way? Test-run should already be able to peek tests from there (and
> >> it does). It probably does not work for your test only because you put it into
> >> a folder. I propose you to flatten it, like it is done for function1.test.lua.
> >> You still can keep issue name in form of 'gh-####-' in file names to keep them
> >> related. They would be
> >>
> >>     gh-4427-ffi-sandwich.test.lua
> >>     gh-4427-ffi-sandwich.c
> >>
> >> And after build:
> >>
> >> +   gh-4427-ffi.sandwich.so/.dylib
> >>
> >> Then you don't a need separate file in app-tap.
> > 
> > A separate file in app-tap is a runner for the chunk in luajit-tap
> > tests. Since I need to test a platform failure, I don't know other way
> > than run one Lua script via io.popen from another one.
> > 
> > So if my math is OK we still have two Lua chunks and a C one:
> > * the Lua runner with io.popen and output analyzer
> > * the Lua script to be run by Tarantool instance
> > * the Dynamic library to be required by the latter Lua script and to be
> >   build from the corresponding C file
> > 
> > Could you please clarify your proposal a bit if I'm wrong or missing
> > something?
> 
> The problem is that you have one test located in 2 folders and 3 files
> now. I propose to make it just 3 files. We never do any test hierarchies.
> Also test-run does not support nested folders with tests.
> 
> Strictly speaking, you can make it 2 files: C and Lua. In Lua file you
> do io.popen on self with an argument, which would mean the file was started
> not by test-run, and which would launch the 'platform failure test'.
> 
> In that way you will have all test code located in one place (not counting
> C file).
> 
>     if arg[3] == 'do_test' then
>         jit.opt, C, print, ...
>         os.exit()
>     end
> 
>     -- arg[1] - tarantool, arg[2] - this file
>     io.popen(string.format('%s %s ...', arg[1], arg[2], 'do_test'))
> 
>     test:is(...)
>     ...

As we discussed, I reworked the tests and dropped all excess files in
app-tap directory. Since the changes also affects luajit repo, here are
diffs for both tarantool and luajit repos:

## tarantool/luajit:

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

diff --git a/test/gh-4427-ffi-sandwich.skipcond b/test/gh-4427-ffi-sandwich.skipcond
new file mode 100644
index 0000000..2a2ec4d
--- /dev/null
+++ b/test/gh-4427-ffi-sandwich.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:
diff --git a/test/gh-4427-ffi-sandwich.test.lua b/test/gh-4427-ffi-sandwich.test.lua
new file mode 100755
index 0000000..9d5e50f
--- /dev/null
+++ b/test/gh-4427-ffi-sandwich.test.lua
@@ -0,0 +1,49 @@
+#!/usr/bin/env tarantool
+
+if #arg == 0 then
+  require('utils').selfrun(arg, {
+    {
+      arg = {
+        1, -- hotloop (arg[1])
+        1, -- trigger (arg[2])
+      },
+      res = tostring(3), -- hotloop + trigger + 1
+      msg = 'Trace is aborted',
+    },
+    {
+      arg = {
+        1, -- hotloop (arg[1])
+        2, -- trigger (arg[2])
+      },
+      res = 'Lua VM re-entrancy is detected while executing the trace',
+      msg = 'Trace is recorded',
+    },
+  })
+end
+
+local cfg = {
+  hotloop = arg[1] or 1,
+  trigger = arg[2] or 1,
+}
+
+local ffi = require('ffi')
+local ffisandwich = ffi.load('libsandwich')
+ffi.cdef('int increment(struct sandwich *state, int i)')
+
+-- Save the current coroutine and set the value to trigger
+-- <increment> call the Lua routine instead of C implementation.
+local sandwich = require('libsandwich')(cfg.trigger)
+
+-- Depending on trigger and hotloop values the following contexts
+-- are possible:
+-- * if trigger <= hotloop -> trace recording is aborted
+-- * if trigger >  hotloop -> trace is recorded but execution
+--   leads to panic
+jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop))
+
+local res
+for i = 0, cfg.trigger + cfg.hotloop do
+  res = ffisandwich.increment(sandwich, i)
+end
+-- Check the resulting value if panic didn't occur earlier.
+print(res)
diff --git a/test/gh-4427-ffi-sandwich/libsandwich.c b/test/gh-4427-ffi-sandwich/libsandwich.c
index 2a24fb2..029758b 100644
--- a/test/gh-4427-ffi-sandwich/libsandwich.c
+++ b/test/gh-4427-ffi-sandwich/libsandwich.c
@@ -7,12 +7,12 @@ struct sandwich {
 	int trigger;  /* Trigger for switching to Lua call */
 };
 
-int ipp(struct sandwich *state, int i)
+int increment(struct sandwich *state, int i)
 {
 	if (i < state->trigger)
 		return i + 1;
 
-	/* Sandwich is triggered and Lua ipp function is called */
+	/* Sandwich is triggered and Lua increment function is called */
 	lua_pushnumber(state->L, state->ref);
 	lua_gettable(state->L, LUA_REGISTRYINDEX);
 	lua_pushnumber(state->L, i);
@@ -29,9 +29,9 @@ static int init(lua_State *L)
 	luaL_getmetatable(L, STRUCT_SANDWICH_MT);
 	lua_setmetatable(L, -2);
 
-	/* Lua ipp function to be called when sandwich is triggered */
+	/* Lua increment function to be called when sandwich is triggered */
 	if (luaL_dostring(L, "return function(i) return i + 1 end"))
-		luaL_error(L, "failed to translate Lua ipp function");
+		luaL_error(L, "failed to translate Lua increment function");
 
 	state->ref = luaL_ref(L, LUA_REGISTRYINDEX);
 	state->L = L;
@@ -43,7 +43,7 @@ static int fin(lua_State *L)
 {
 	struct sandwich *state = luaL_checkudata(L, 1, STRUCT_SANDWICH_MT);
 
-	/* Release the anchored ipp function */
+	/* Release the anchored increment function */
 	luaL_unref(L, LUA_REGISTRYINDEX, state->ref);
 	return 0;
 }
diff --git a/test/gh-4427-ffi-sandwich/test.lua b/test/gh-4427-ffi-sandwich/test.lua
deleted file mode 100644
index 3ccaee5..0000000
--- a/test/gh-4427-ffi-sandwich/test.lua
+++ /dev/null
@@ -1,26 +0,0 @@
-local cfg = {
-  hotloop = arg[1] or 1,
-  trigger = arg[2] or 1,
-}
-
-local ffi = require('ffi')
-local ffisandwich = ffi.load('libsandwich')
-ffi.cdef('int ipp(struct sandwich *state, int i)')
-
--- Save the current coroutine and set the value to trigger ipp
--- call the Lua routine instead of pure C implementation.
-local sandwich = require('libsandwich')(cfg.trigger)
-
--- Depending on trigger and hotloop values the following contexts
--- are possible:
--- * if trigger <= hotloop -> trace recording is aborted
--- * if trigger >  hotloop -> trace is recorded but execution
---   leads to panic
-jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop))
-
-local res
-for i = 0, cfg.trigger + cfg.hotloop do
-  res = ffisandwich.ipp(sandwich, i)
-end
--- Check the resulting value if panic didn't occur earlier.
-print(res)

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

## tarantool/tarantool:

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

diff --git a/test/app-tap/gh-4427-ffi-sandwich.skipcond b/test/app-tap/gh-4427-ffi-sandwich.skipcond
deleted file mode 100644
index 2a2ec4d97..000000000
--- a/test/app-tap/gh-4427-ffi-sandwich.skipcond
+++ /dev/null
@@ -1,7 +0,0 @@
-import platform
-
-# Disabled on FreeBSD due to #4819.
-if platform.system() == 'FreeBSD':
-    self.skip = 1
-
-# vim: set ft=python:
diff --git a/test/app-tap/gh-4427-ffi-sandwich.test.lua b/test/app-tap/gh-4427-ffi-sandwich.test.lua
deleted file mode 100755
index 829c129a0..000000000
--- a/test/app-tap/gh-4427-ffi-sandwich.test.lua
+++ /dev/null
@@ -1,34 +0,0 @@
-#!/usr/bin/env tarantool
-
-local tap = require('tap')
-
-local test = tap.test('gh-4427-ffi-sandwich')
-
-local vars = {
-  PATH   = os.getenv('BUILDDIR') .. '/test/luajit-tap/gh-4427-ffi-sandwich',
-  SUFFIX = package.cpath:match('?.(%a+);'),
-}
-
-local cmd = string.gsub('LUA_CPATH=<PATH>/?.<SUFFIX> LD_LIBRARY_PATH=<PATH> '
-  .. 'tarantool 2>&1 <PATH>/test.lua %d %d', '%<(%w+)>', vars)
-
-local checks = {
-  { hotloop = 1, trigger = 1, success = true  },
-  { hotloop = 1, trigger = 2, success = false },
-}
-
-test:plan(#checks)
-
-for _, ch in pairs(checks) do
-  local res
-  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
-  for s in proc:lines('*l') do res = s end
-  assert(res, 'proc:lines failed')
-  if ch.success then
-    test:is(tonumber(res), ch.hotloop + ch.trigger + 1)
-  else
-    test:is(res, 'Lua VM re-entrancy is detected while executing the trace')
-  end
-end
-
-os.exit(test:check() and 0 or 1)

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

I updated the upstream branches and also sent the v2 for both series:
* tarantool/luajit[1].
* tarantool/tarantool[2].

> 
> > [1]: https://www.lua.org/manual/5.1/manual.html#pdf-file:read

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015935.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015939.html

-- 
Best regards,
IM

[Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests

[Igor Munkin]

The test is added as a CMake subdirectory and the runner executes
tarantool test.lua command via io.popen to check whether the platform
successfully finishes the execution or platform panic occurs.

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 test/CMakeLists.txt                     |  1 +
 test/app-tap/lj-flush-on-trace.test.lua | 30 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100755 test/app-tap/lj-flush-on-trace.test.lua

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index bceea4567..0ae3843e3 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -60,6 +60,7 @@ add_subdirectory(app-tap)
 add_subdirectory(box)
 add_subdirectory(unit)
 add_subdirectory(luajit-tap/gh-4427-ffi-sandwich)
+add_subdirectory(luajit-tap/lj-flush-on-trace)
 
 # Move tarantoolctl config
 if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
new file mode 100755
index 000000000..70b7bd9a2
--- /dev/null
+++ b/test/app-tap/lj-flush-on-trace.test.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+
+local test = tap.test('lj-flush-on-trace')
+
+local cmd = string.gsub(
+  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
+  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
+
+local checks = {
+  { hotloop = 1, trigger = 1, success = true  },
+  { hotloop = 1, trigger = 2, success = false },
+}
+
+test:plan(#checks)
+
+for _, ch in pairs(checks) do
+  local res
+  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
+  for s in proc:lines('*l') do res = s end
+  assert(res, 'proc:lines failed')
+  if ch.success then
+    test:is(res, 'OK')
+  else
+    test:is(res, 'JIT mode change is detected while executing the trace')
+  end
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.25.0

Re: [Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests

[Igor Munkin]

Fixed the issue with shared library suffix. The diff is below:

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

diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
index 70b7bd9a2..ad6a484ed 100755
--- a/test/app-tap/lj-flush-on-trace.test.lua
+++ b/test/app-tap/lj-flush-on-trace.test.lua
@@ -4,9 +4,13 @@ local tap = require('tap')
 
 local test = tap.test('lj-flush-on-trace')
 
-local cmd = string.gsub(
-  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
-  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
+local vars = {
+  PATH   = os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace',
+  SUFFIX = package.cpath:match('?.(%a+);'),
+}
+
+local cmd = string.gsub('LUA_CPATH=<PATH>/?.<SUFFIX> LD_LIBRARY_PATH=<PATH> '
+  .. 'tarantool 2>&1 <PATH>/test.lua %d %d', '%<(%w+)>', vars)
 
 local checks = {
   { hotloop = 1, trigger = 1, success = true  },

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

Also added a skipcond for FreeBSD:

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

diff --git a/test/app-tap/lj-flush-on-trace.skipcond
b/test/app-tap/lj-flush-on-trace.skipcond
new file mode 100644
index 000000000..2a2ec4d97
--- /dev/null
+++ b/test/app-tap/lj-flush-on-trace.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:

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

On 27.03.20, Igor Munkin wrote:
> The test is added as a CMake subdirectory and the runner executes
> tarantool test.lua command via io.popen to check whether the platform
> successfully finishes the execution or platform panic occurs.
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>  test/CMakeLists.txt                     |  1 +
>  test/app-tap/lj-flush-on-trace.test.lua | 30 +++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>  create mode 100755 test/app-tap/lj-flush-on-trace.test.lua
> 
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index bceea4567..0ae3843e3 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -60,6 +60,7 @@ add_subdirectory(app-tap)
>  add_subdirectory(box)
>  add_subdirectory(unit)
>  add_subdirectory(luajit-tap/gh-4427-ffi-sandwich)
> +add_subdirectory(luajit-tap/lj-flush-on-trace)
>  
>  # Move tarantoolctl config
>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
> new file mode 100755
> index 000000000..70b7bd9a2
> --- /dev/null
> +++ b/test/app-tap/lj-flush-on-trace.test.lua
> @@ -0,0 +1,30 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test('lj-flush-on-trace')
> +
> +local cmd = string.gsub(
> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
> +
> +local checks = {
> +  { hotloop = 1, trigger = 1, success = true  },
> +  { hotloop = 1, trigger = 2, success = false },
> +}
> +
> +test:plan(#checks)
> +
> +for _, ch in pairs(checks) do
> +  local res
> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> +  for s in proc:lines('*l') do res = s end
> +  assert(res, 'proc:lines failed')
> +  if ch.success then
> +    test:is(res, 'OK')
> +  else
> +    test:is(res, 'JIT mode change is detected while executing the trace')
> +  end
> +end
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests

[Vladislav Shpilevoy]

>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
> new file mode 100755
> index 000000000..70b7bd9a2
> --- /dev/null
> +++ b/test/app-tap/lj-flush-on-trace.test.lua
> @@ -0,0 +1,30 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +
> +local test = tap.test('lj-flush-on-trace')
> +
> +local cmd = string.gsub(
> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
> +
> +local checks = {
> +  { hotloop = 1, trigger = 1, success = true  },
> +  { hotloop = 1, trigger = 2, success = false },
> +}
> +
> +test:plan(#checks)
> +
> +for _, ch in pairs(checks) do
> +  local res
> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> +  for s in proc:lines('*l') do res = s end
> +  assert(res, 'proc:lines failed')

This file is exactly the same as the other file for running a
luajit test in the previous commit. I propose you to move this to
a separate file, which would provide API to run arbitrary test via
io.popen. Unless this won't be dropped if you find a way to make
the new luajit tests runable via test-run as is.

> +  if ch.success then
> +    test:is(res, 'OK')
> +  else
> +    test:is(res, 'JIT mode change is detected while executing the trace')
> +  end
> +end
> +
> +os.exit(test:check() and 0 or 1)
> 

Re: [Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests

[Igor Munkin]

Vlad,

Thanks for your review!

On 05.04.20, Vladislav Shpilevoy wrote:
> >  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> > diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
> > new file mode 100755
> > index 000000000..70b7bd9a2
> > --- /dev/null
> > +++ b/test/app-tap/lj-flush-on-trace.test.lua
> > @@ -0,0 +1,30 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local tap = require('tap')
> > +
> > +local test = tap.test('lj-flush-on-trace')
> > +
> > +local cmd = string.gsub(
> > +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> > +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
> > +
> > +local checks = {
> > +  { hotloop = 1, trigger = 1, success = true  },
> > +  { hotloop = 1, trigger = 2, success = false },
> > +}
> > +
> > +test:plan(#checks)
> > +
> > +for _, ch in pairs(checks) do
> > +  local res
> > +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> > +  for s in proc:lines('*l') do res = s end
> > +  assert(res, 'proc:lines failed')
> 
> This file is exactly the same as the other file for running a
> luajit test in the previous commit. I propose you to move this to
> a separate file, which would provide API to run arbitrary test via
> io.popen. Unless this won't be dropped if you find a way to make
> the new luajit tests runable via test-run as is.

Sounds rational. They are not the same but quite similar (the difference
is only in the result values). I guess I can do it in scope of #4862[1]
if you're OK with it.

> 
> > +  if ch.success then
> > +    test:is(res, 'OK')
> > +  else
> > +    test:is(res, 'JIT mode change is detected while executing the trace')
> > +  end
> > +end
> > +
> > +os.exit(test:check() and 0 or 1)
> > 

[1]: https://github.com/tarantool/tarantool/issues/4862

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests

[Vladislav Shpilevoy]

Hi!

>>>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
>>> diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
>>> new file mode 100755
>>> index 000000000..70b7bd9a2
>>> --- /dev/null
>>> +++ b/test/app-tap/lj-flush-on-trace.test.lua
>>> @@ -0,0 +1,30 @@
>>> +#!/usr/bin/env tarantool
>>> +
>>> +local tap = require('tap')
>>> +
>>> +local test = tap.test('lj-flush-on-trace')
>>> +
>>> +local cmd = string.gsub(
>>> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
>>> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
>>> +
>>> +local checks = {
>>> +  { hotloop = 1, trigger = 1, success = true  },
>>> +  { hotloop = 1, trigger = 2, success = false },
>>> +}
>>> +
>>> +test:plan(#checks)
>>> +
>>> +for _, ch in pairs(checks) do
>>> +  local res
>>> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
>>> +  for s in proc:lines('*l') do res = s end
>>> +  assert(res, 'proc:lines failed')
>>
>> This file is exactly the same as the other file for running a
>> luajit test in the previous commit. I propose you to move this to
>> a separate file, which would provide API to run arbitrary test via
>> io.popen. Unless this won't be dropped if you find a way to make
>> the new luajit tests runable via test-run as is.
> 
> Sounds rational. They are not the same but quite similar (the difference
> is only in the result values).

At least first 24 lines of them match almost completely (except file
name to start with io.popen), that is more than half of each. I think
that part either should be simplified so as there is nothing to extract
already (my proposal about io.popen() of self may help, or may not), or
just extracted into a common file like

    test/app-tap/utils/utils.lua

You can extract even more if you pass expected output with 'checks'
array elements.

If you want to do that in scope of luajit tests rework, then add it to
the ticket description, please, so as not to forget.

> I guess I can do it in scope of #4862[1]
> if you're OK with it.
> 
>>
>>> +  if ch.success then
>>> +    test:is(res, 'OK')
>>> +  else
>>> +    test:is(res, 'JIT mode change is detected while executing the trace')
>>> +  end
>>> +end
>>> +
>>> +os.exit(test:check() and 0 or 1)
>>>
> 
> [1]: https://github.com/tarantool/tarantool/issues/4862
> 

Re: [Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests

[Igor Munkin]

Vlad,

Thanks for your comments!

On 10.04.20, Vladislav Shpilevoy wrote:
> Hi!
> 
> >>>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> >>> diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
> >>> new file mode 100755
> >>> index 000000000..70b7bd9a2
> >>> --- /dev/null
> >>> +++ b/test/app-tap/lj-flush-on-trace.test.lua
> >>> @@ -0,0 +1,30 @@
> >>> +#!/usr/bin/env tarantool
> >>> +
> >>> +local tap = require('tap')
> >>> +
> >>> +local test = tap.test('lj-flush-on-trace')
> >>> +
> >>> +local cmd = string.gsub(
> >>> +  'LUA_CPATH=$/?.so LD_LIBRARY_PATH=$ tarantool 2>&1 $/test.lua %d %d',
> >>> +  '%$', os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace')
> >>> +
> >>> +local checks = {
> >>> +  { hotloop = 1, trigger = 1, success = true  },
> >>> +  { hotloop = 1, trigger = 2, success = false },
> >>> +}
> >>> +
> >>> +test:plan(#checks)
> >>> +
> >>> +for _, ch in pairs(checks) do
> >>> +  local res
> >>> +  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
> >>> +  for s in proc:lines('*l') do res = s end
> >>> +  assert(res, 'proc:lines failed')
> >>
> >> This file is exactly the same as the other file for running a
> >> luajit test in the previous commit. I propose you to move this to
> >> a separate file, which would provide API to run arbitrary test via
> >> io.popen. Unless this won't be dropped if you find a way to make
> >> the new luajit tests runable via test-run as is.
> > 
> > Sounds rational. They are not the same but quite similar (the difference
> > is only in the result values).
> 
> At least first 24 lines of them match almost completely (except file
> name to start with io.popen), that is more than half of each. I think
> that part either should be simplified so as there is nothing to extract
> already (my proposal about io.popen() of self may help, or may not), or
> just extracted into a common file like
> 
>     test/app-tap/utils/utils.lua

utils.lua is introduced here[1].

> 
> You can extract even more if you pass expected output with 'checks'
> array elements.
> 
> If you want to do that in scope of luajit tests rework, then add it to
> the ticket description, please, so as not to forget.

As we discussed, I reworked the tests and dropped all excess files in
app-tap directory. Since the changes also affects luajit repo, here are
diffs for both tarantool and luajit repos:

## tarantool/luajit:

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

diff --git a/test/lj-flush-on-trace.skipcond b/test/lj-flush-on-trace.skipcond
new file mode 100644
index 0000000..2a2ec4d
--- /dev/null
+++ b/test/lj-flush-on-trace.skipcond
@@ -0,0 +1,7 @@
+import platform
+
+# Disabled on FreeBSD due to #4819.
+if platform.system() == 'FreeBSD':
+    self.skip = 1
+
+# vim: set ft=python:
diff --git a/test/lj-flush-on-trace/test.lua b/test/lj-flush-on-trace.test.lua
old mode 100644
new mode 100755
similarity index 53%
rename from test/lj-flush-on-trace/test.lua
rename to test/lj-flush-on-trace.test.lua
index ff6e0b6..0b3ccf4
--- a/test/lj-flush-on-trace/test.lua
+++ b/test/lj-flush-on-trace.test.lua
@@ -1,3 +1,26 @@
+#!/usr/bin/env tarantool
+
+if #arg == 0 then
+  require('utils').selfrun(arg, {
+    {
+      arg = {
+        1, -- hotloop (arg[1])
+        1, -- trigger (arg[2])
+      },
+      res = 'OK',
+      msg = 'Trace is aborted',
+    },
+    {
+      arg = {
+        1, -- hotloop (arg[1])
+        2, -- trigger (arg[2])
+      },
+      res = 'JIT mode change is detected while executing the trace',
+      msg = 'Trace is recorded',
+    },
+  })
+end
+
 local cfg = {
   hotloop = arg[1] or 1,
   trigger = arg[2] or 1,
@@ -7,8 +30,8 @@ local ffi = require('ffi')
 local ffiflush = ffi.load('libflush')
 ffi.cdef('void flush(struct flush *state, int i)')
 
--- Save the current coroutine and set the value to trigger ipp
--- call the Lua routine instead of pure C implementation.
+-- Save the current coroutine and set the value to trigger
+-- <flush> call the Lua routine instead of C implementation.
 local flush = require('libflush')(cfg.trigger)
 
 -- Depending on trigger and hotloop values the following contexts

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

## tarantool/tarantool:

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

diff --git a/test/app-tap/lj-flush-on-trace.skipcond b/test/app-tap/lj-flush-on-trace.skipcond
deleted file mode 100644
index 2a2ec4d97..000000000
--- a/test/app-tap/lj-flush-on-trace.skipcond
+++ /dev/null
@@ -1,7 +0,0 @@
-import platform
-
-# Disabled on FreeBSD due to #4819.
-if platform.system() == 'FreeBSD':
-    self.skip = 1
-
-# vim: set ft=python:
diff --git a/test/app-tap/lj-flush-on-trace.test.lua b/test/app-tap/lj-flush-on-trace.test.lua
deleted file mode 100755
index ad6a484ed..000000000
--- a/test/app-tap/lj-flush-on-trace.test.lua
+++ /dev/null
@@ -1,34 +0,0 @@
-#!/usr/bin/env tarantool
-
-local tap = require('tap')
-
-local test = tap.test('lj-flush-on-trace')
-
-local vars = {
-  PATH   = os.getenv('BUILDDIR') .. '/test/luajit-tap/lj-flush-on-trace',
-  SUFFIX = package.cpath:match('?.(%a+);'),
-}
-
-local cmd = string.gsub('LUA_CPATH=<PATH>/?.<SUFFIX> LD_LIBRARY_PATH=<PATH> '
-  .. 'tarantool 2>&1 <PATH>/test.lua %d %d', '%<(%w+)>', vars)
-
-local checks = {
-  { hotloop = 1, trigger = 1, success = true  },
-  { hotloop = 1, trigger = 2, success = false },
-}
-
-test:plan(#checks)
-
-for _, ch in pairs(checks) do
-  local res
-  local proc = io.popen(cmd:format(ch.hotloop, ch.trigger))
-  for s in proc:lines('*l') do res = s end
-  assert(res, 'proc:lines failed')
-  if ch.success then
-    test:is(res, 'OK')
-  else
-    test:is(res, 'JIT mode change is detected while executing the trace')
-  end
-end
-
-os.exit(test:check() and 0 or 1)

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

I updated the upstream branches and also sent the v2 for both series:
* tarantool/luajit[2].
* tarantool/tarantool[3].

> 
> > I guess I can do it in scope of #4862[1]
> > if you're OK with it.
> > 
> >>
> >>> +  if ch.success then
> >>> +    test:is(res, 'OK')
> >>> +  else
> >>> +    test:is(res, 'JIT mode change is detected while executing the trace')
> >>> +  end
> >>> +end
> >>> +
> >>> +os.exit(test:check() and 0 or 1)
> >>>
> > 
> > [1]: https://github.com/tarantool/tarantool/issues/4862
> > 

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015936.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015935.html
[3]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015939.html

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C

[Igor Munkin]

Sorry, forgot to add a ChangeLog entry.

On 27.03.20, Igor Munkin wrote:
> This series prepares the existing testing machinery to run LuaJIT tests
> requiring libraries implemented in C and enables the existing ones:
> * gh-4427-ffi-sandwich
> * lj-flush-on-trace
> 

@ChangeLog:
* "FFI sandwich"(*) detection is introduced. If sandwich is detected
  while trace recording the recording is aborted. The sandwich detected
  while mcode execution leads to the platform panic.
* luaJIT_setmode call is prohibited while mcode execution and leads to
  the platform panic.

(*) The following stack mix is called FFI sandwich.
    | Lua-FFI -> С routine -> Lua-C API -> Lua VM
    This sort of re-entrancy is explicitly not supported by LuaJIT
    compiler. For more info see gh-4427.

> Igor Munkin (4):
>   luajit: bump new version
>   test: adjust luajit-tap testing machinery
>   test: enable luajit-tap:gh-4427-ffi-sandwich tests
>   test: enable luajit-tap:lj-flush-on-trace tests
> 
>  test/CMakeLists.txt                        | 17 +++++++-----
>  test/app-tap/gh-4427-ffi-sandwich.test.lua | 30 ++++++++++++++++++++++
>  test/app-tap/lj-flush-on-trace.test.lua    | 30 ++++++++++++++++++++++
>  third_party/luajit                         |  2 +-
>  4 files changed, 72 insertions(+), 7 deletions(-)
>  create mode 100755 test/app-tap/gh-4427-ffi-sandwich.test.lua
>  create mode 100755 test/app-tap/lj-flush-on-trace.test.lua
> 
> -- 
> 2.25.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C

[Igor Munkin]

CI failed for OSX and FreeBSD on the introduced test. I'll investigate
the root cause (for now it looks like invalid paths for LD_LIBRARY_PATH
and LUA_CPATH).

On 27.03.20, Igor Munkin wrote:
> Sorry, forgot to add a ChangeLog entry.

And branch: https://github.com/tarantool/tarantool/tree/imun/ffi-sandwich

> 

<snipped>

> 
> -- 
> Best regards,
> IM

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C

[Igor Munkin]

There were two separate problems:
* MacOS dynamic libraries have a .dylib suffix instead of the .so one,
  so Lua libraries required for tests failed to be found. This issue is
  fixed (sent the diffs with the fix within a corresponding patches).
* Trace assembling fails with "failed to allocate mcode memory" on
  FreeBSD. I filed an issue[1] and added corresponding skipcond files to
  the patches.

Now CI is green[1].

On 28.03.20, Igor Munkin wrote:
> CI failed for OSX and FreeBSD on the introduced test. I'll investigate
> the root cause (for now it looks like invalid paths for LD_LIBRARY_PATH
> and LUA_CPATH).
> 

<snipped>

> 
> -- 
> Best regards,
> IM

[1]: https://gitlab.com/tarantool/tarantool/-/jobs/491398423

-- 
Best regards,
IM