[Tarantool-patches] [PATCH luajit 3/7] test: stop using utils.selfrun in tests
Sergey Kaplun
skaplun at tarantool.org
Wed Feb 15 11:08:16 MSK 2023
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',
> +})
>
> -- 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.
> + 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/gh-4427-ffi-sandwich/script.lua b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua
> new file mode 100644
> index 00000000..9ecd964e
> --- /dev/null
> +++ b/test/tarantool-tests/gh-4427-ffi-sandwich/script.lua
<snipped>
> diff --git a/test/tarantool-tests/lj-351-print-tostring-number.test.lua b/test/tarantool-tests/lj-351-print-tostring-number.test.lua
> index da5b31be..72a9ec2b 100644
> --- a/test/tarantool-tests/lj-351-print-tostring-number.test.lua
> +++ b/test/tarantool-tests/lj-351-print-tostring-number.test.lua
<snipped>
> diff --git a/test/tarantool-tests/lj-351-print-tostring-number/script.lua b/test/tarantool-tests/lj-351-print-tostring-number/script.lua
> new file mode 100644
> index 00000000..c3066f49
<snipped>
> diff --git a/test/tarantool-tests/lj-586-debug-non-string-error.test.lua b/test/tarantool-tests/lj-586-debug-non-string-error.test.lua
> index f02353fe..dcb730a2 100644
> --- a/test/tarantool-tests/lj-586-debug-non-string-error.test.lua
> +++ b/test/tarantool-tests/lj-586-debug-non-string-error.test.lua
<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.
> + env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') },
> + redirect = '2>&1',
> +})
<snipped>
> diff --git a/test/tarantool-tests/lj-flush-on-trace/script.lua b/test/tarantool-tests/lj-flush-on-trace/script.lua
> new file mode 100644
> index 00000000..d2c35534
> --- /dev/null
> +++ b/test/tarantool-tests/lj-flush-on-trace/script.lua
<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.
> + 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)?
> + 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.
> + return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '')
> + end
> + })
> end
>
> function M.skipcond(condition, message)
> --
> 2.30.2
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list