[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