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

[Igor Munkin <imun@tarantool.org>]

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