Re: [Tarantool-patches] [PATCH luajit 3/7] test: stop using utils.selfrun in tests

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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@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