[Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests
Igor Munkin
imun at tarantool.org
Wed Apr 15 03:46:45 MSK 2020
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
More information about the Tarantool-patches
mailing list