Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin <imun@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests
Date: Wed, 8 Apr 2020 02:28:55 +0300	[thread overview]
Message-ID: <20200407232855.GC5713@tarantool.org> (raw)
In-Reply-To: <0376106b-1398-f888-16a8-1498212a2915@tarantool.org>

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

  reply	other threads:[~2020-04-07 23:36 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-27 13:23 [Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C Igor Munkin
2020-03-27 13:23 ` [Tarantool-patches] [PATCH 1/4] luajit: bump new version Igor Munkin
2020-03-27 13:23 ` [Tarantool-patches] [PATCH 2/4] test: adjust luajit-tap testing machinery Igor Munkin
2020-04-05 19:32   ` Vladislav Shpilevoy
2020-04-07 23:05     ` Igor Munkin
2020-03-27 13:23 ` [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests Igor Munkin
2020-03-30 18:53   ` Igor Munkin
2020-04-05 19:32   ` Vladislav Shpilevoy
2020-04-07 23:28     ` Igor Munkin [this message]
2020-04-09 22:05       ` Vladislav Shpilevoy
2020-04-15  0:46         ` Igor Munkin
2020-03-27 13:23 ` [Tarantool-patches] [PATCH 4/4] test: enable luajit-tap:lj-flush-on-trace tests Igor Munkin
2020-03-30 18:53   ` Igor Munkin
2020-04-05 19:32   ` Vladislav Shpilevoy
2020-04-07 23:33     ` Igor Munkin
2020-04-09 22:05       ` Vladislav Shpilevoy
2020-04-15  0:47         ` Igor Munkin
2020-03-27 13:32 ` [Tarantool-patches] [PATCH 0/4] Enable LuaJIT tests written in C Igor Munkin
2020-03-28 11:18   ` Igor Munkin
2020-03-30 18:55     ` Igor Munkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200407232855.GC5713@tarantool.org \
    --to=imun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 3/4] test: enable luajit-tap:gh-4427-ffi-sandwich tests' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox