[Tarantool-patches] [PATCH luajit 3/7] test: stop using utils.selfrun in tests
Igor Munkin
imun at tarantool.org
Mon Feb 27 12:16:08 MSK 2023
Sergey,
Thanks for your review! See my answers below.
On 15.02.23, Sergey Kaplun wrote:
> Hi, Igor!
> Thanks for the patch!
> Generally LGTM, but please consider my entreaties about comments below.
>
> On 13.02.23, Igor Munkin wrote:
> > Unfortunately, <utils.selfrun> is too complex to be maintained, so the
> > corresponding tests are split into two files: the test itself and the
> > script to be run by the test. As a result of the patch <utils.makecmd>
> > helper is introduced: it inherits some approaches from <utils.selfrun>,
> > but it's considered for more general use.
> >
> > Relates to tarantool/tarantool#8252
> >
> > Signed-off-by: Igor Munkin <imun at tarantool.org>
> > ---
>
> <snipped>
>
> > diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > index dd02130c..f4795db0 100644
> > --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
> > @@ -3,52 +3,43 @@ local utils = require('utils')
>
> <snipped>
>
> > --- 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)
> > +local script = utils.makecmd(arg, {
>
> It will be nice to drop a comment here and below, that `makecmd()`
> searches for <%testname%/script.lua>.
>
> > + -- TODO: Leave another toxic comment regarding SIP on macOS.
>
> Minor: This TODO is unnecessary, so feel free to delete this line.
>
> > + env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
> > + redirect = '2>&1',
> > +})
Fixed both cases above. You can find the diff below:
================================================================================
diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
index ff3eaf01..ad06c329 100644
--- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
+++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua
@@ -6,8 +6,23 @@ local test = tap.test('gh-4427-ffi-sandwich'):skipcond({
test:plan(2)
+-- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
+-- with the given environment, launch options and CLI arguments.
local script = require('utils').makecmd(arg, {
- -- TODO: Leave another toxic comment regarding SIP on macOS.
+ -- XXX: Apple tries their best to "protect their users from
+ -- malware". As a result SIP (see the link[1] below) has been
+ -- designed and released. Now, Apple developers are so
+ -- protected, that they can load nothing being not installed in
+ -- the system, since the environment is sanitized before the
+ -- child process is launched. In particular, environment
+ -- variables starting with DYLD_ and LD_ are unset for child
+ -- process. For more info, see the docs[2] below.
+ --
+ -- The environment variable below is used by FFI machinery to
+ -- find the proper shared library.
+ --
+ -- [1]: https://support.apple.com/en-us/HT204899
+ -- [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
redirect = '2>&1',
})
================================================================================
> >
> > -- 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 hotloop = 1
> > +local cases = {
> > + abort = {
> > + trigger = hotloop,
> > + expected = '#4427 still works',
>
> Side note: We may provide this message from here to avoid it copy-pasting in
> <gh-4427-ffi-sandwich/script.lua>, but this reduces the test
> readability... At least, we need to call format twice in the test's
> payload, so for better good is not to do it.
Yeah, there is no silver bullet for this, unfortunately. This is lesser
evil in our case, IMHO.
>
> > + test = 'is',
> > + message = 'Trace is aborted',
> > + },
> > + panic = {
> > + trigger = hotloop + 1,
> > + expected = 'Lua VM re%-entrancy is detected while executing the trace',
> > + test = 'like',
> > + message = 'Trace is compiled',
> > + },
> > +}
<snipped>
> > diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
> > index c46b93f0..cf92757c 100644
> > --- a/test/tarantool-tests/lj-flush-on-trace.test.lua
> > +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
>
> <snipped>
>
> > --- 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)
> > +local script = utils.makecmd(arg, {
> > + -- TODO: Leave another toxic comment regarding SIP on macOS.
>
> Minor: This TODO is unnecessary, so feel free to delete this line.
Added the same comments as for the case above. The diff is below:
================================================================================
diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua
index e157622a..a355c688 100644
--- a/test/tarantool-tests/lj-flush-on-trace.test.lua
+++ b/test/tarantool-tests/lj-flush-on-trace.test.lua
@@ -6,8 +6,23 @@ local test = tap.test('lj-flush-on-trace'):skipcond({
test:plan(2)
+-- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
+-- with the given environment, launch options and CLI arguments.
local script = require('utils').makecmd(arg, {
- -- TODO: Leave another toxic comment regarding SIP on macOS.
+ -- XXX: Apple tries their best to "protect their users from
+ -- malware". As a result SIP (see the link[1] below) has been
+ -- designed and released. Now, Apple developers are so
+ -- protected, that they can load nothing being not installed in
+ -- the system, since the environment is sanitized before the
+ -- child process is launched. In particular, environment
+ -- variables starting with DYLD_ and LD_ are unset for child
+ -- process. For more info, see the docs[2] below.
+ --
+ -- The environment variable below is used by FFI machinery to
+ -- find the proper shared library.
+ --
+ -- [1]: https://support.apple.com/en-us/HT204899
+ -- [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html
env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
redirect = '2>&1',
})
================================================================================
>
> > + env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
> > + redirect = '2>&1',
> > +})
<snipped>
> > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> > index eb11d40d..41a7c22a 100644
> > --- a/test/tarantool-tests/utils.lua
> > +++ b/test/tarantool-tests/utils.lua
>
> <snipped>
>
> > +function M.makecmd(arg, opts)
>
> Please, add the comment about script location, it's good thing to
> commment, IMHO.
Added the comment to clear the usage. The diff is below:
================================================================================
diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
index 4fb66600..6a9645ca 100644
--- a/test/tarantool-tests/utils.lua
+++ b/test/tarantool-tests/utils.lua
@@ -52,6 +52,11 @@ local function makeenv(tabenv)
return table.concat(flatenv, ' ')
end
+-- <makecmd> creates a command that runs %testname%/script.lua by
+-- <LUAJIT_TEST_BINARY> with the given environment, launch options
+-- and CLI arguments. The function yields an object (i.e. table)
+-- with the aforementioned parameters. To launch the command just
+-- call the object.
function M.makecmd(arg, opts)
return setmetatable({
LUABIN = M.luacmd(arg),
================================================================================
>
> > + return setmetatable({
> > + LUABIN = M.luacmd(arg),
> > + SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'),
> > + ENV = opts and makeenv(opts.env) or '',
>
> Is this option is used only for DYLD_LIBRARY_PATH (as I can see for now)?
> Should we drop the comment about that here (that this is the main
> reason)?
Not really. This is just a general approach to tweak the child process
environment. And yes, it's used only for DYLD_LIBRARY_PATH for now, but
I want to create a general interface to run commands in tests.
>
> > + REDIRECT = opts and opts.redirect or '',
> > + }, {
> > + __call = function(self, ...)
> > + local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self)
> > + .. (' %s'):rep(select('#', ...)):format(...)
>
> It's good to comment that we just format arguments to strings for the
> script invocaiton.
Well, yeah... my Perl background is coming out ;)
Anyway, here is the comment:
================================================================================
diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
index ac04c01d..f5e08043 100644
--- a/test/tarantool-tests/tap.lua
+++ b/test/tarantool-tests/tap.lua
@@ -60,8 +65,17 @@ function M.makecmd(arg, opts)
REDIRECT = opts and opts.redirect or '',
}, {
__call = function(self, ...)
+ -- This line just makes the command for <io.popen> by the
+ -- following steps:
+ -- 1. Replace the placeholders with the corresponding values
+ -- given to the command constructor (e.g. script, env)
+ -- 2. Join all CLI arguments given to the __call metamethod
+ -- 3. Concatenate the results of step 1 and step 2 to obtain
+ -- the resulting command.
local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self)
.. (' %s'):rep(select('#', ...)):format(...)
+ -- Trim both leading and trailing whitespace from the output
+ -- produced by the child process.
return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
end
})
================================================================================
>
> > + return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
> > + end
> > + })
> > end
> >
> > function M.skipcond(condition, message)
> > --
> > 2.30.2
> >
>
> --
> Best regards,
> Sergey Kaplun
--
Best regards,
IM
More information about the Tarantool-patches
mailing list