Re: [Tarantool-patches] [PATCH 1/2] box/console: handle multireturn in lua mode

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, Jan 14, 2020 at 03:54:12AM +0300, Alexander Turenko wrote:
> 
> Cited from the previous review (see [1]) and your answer (to discuss it
> here):
> 
> >> BTW, it is a bit unusual to use `opts` for a string value: is it worth
> >> to rename it to `mode`?
> >
> > Sasha, I left @opts variable untouched simply because we might
> > extend it in future (while for now it is only a single string).
> 
> I don't get the point, to be honest. This is the internal function and
> we can change it as we wish when we wish. However it is better when each
> incarnation corresponds to usual conventions. For `opts` I would expect
> that it is a key-value table, but `mode` can be anything (I have no
> assumptions for this name and so I'm motivated to look at the code).
> 
> Since this is the internal function and it has quite small amount of
> arguments I would not create a table and keep the argument being a
> string. I just think that `opts` name may be a bit misleading.
> 
> Anyway, it is quite minor thing considering that the function is easy
> and small.

Thanks, accounted. I'll rework the series, and address this name too.

> > diff --git a/test/app-tap/console-lua.skipcond b/test/app-tap/console-lua.skipcond
> > new file mode 100644
> > index 000000000..48e17903e
> > --- /dev/null
> > +++ b/test/app-tap/console-lua.skipcond
> > @@ -0,0 +1,7 @@
> > +import platform
> > +
> > +# Disabled on FreeBSD due to flaky fail #4271.
> > +if platform.system() == 'FreeBSD':
> > +    self.skip = 1
> > +
> > +# vim: set ft=python:
> 
> I would add the new test name to the issue to actually track it. However
> I verified it on FreeBSD 12.1 (prerelease version, it is near to my
> hands) and everything seems to be good. Let's remove this skipcond if
> you don't see actual fail on it.

OK

> > diff --git a/test/app-tap/console-lua.test.lua b/test/app-tap/console-lua.test.lua
> > new file mode 100755
> > index 000000000..d3271a369
> > --- /dev/null
> > +++ b/test/app-tap/console-lua.test.lua
> 
> As far as I see, our tests tend to use
> gh-dddd-short-description.test.lua name pattern for issue-related cases
> and name_with_several_words.test.lua for subsystem-related cases. So it
> seems logical to name it console_lua.test.lua (with underscore).

OK

> > @@ -0,0 +1,62 @@
> > +#!/usr/bin/env tarantool
> > +--
> > +-- vim: ts=4 sw=4 et
> > +
> > +local tap = require('tap')
> > +local console = require('console')
> > +local socket = require('socket')
> > +local yaml = require('yaml')
> > +local fiber = require('fiber')
> > +local ffi = require('ffi')
> > +local log = require('log')
> > +local fio = require('fio')
> 
> Some unused variables. Let's check the file with `luacheck -r $file`.

OK, thanks!

> > +
> > +-- Suppress console log messages
> > +log.level(4)
> 
> We don't call box.cfg() and so have no logs. This call seems to be
> unneeded.

Thanks

> > +local CONSOLE_SOCKET = fio.pathjoin(fio.cwd(), 'tarantool-test-console-lua.sock')
> 
> It is better to avoid long names for Unix sockets, because of
> UNIX_PATH_MAX limit (108 on Linux, while the full path can be quite
> long). However here we can use basename and it will definitely fit 108
> symbols. It works like so (yep, I prefer a short name anyway):

I took this code from another test case. Actually I wonder why we don't
use relative names instead.

> 
>  | local CONSOLE_SOCKET = 'console-lua.sock'
>  | local server = console.listen('unix/:./' .. CONSOLE_SOCKET)
>  | local client = socket.tcp_connect('unix/', CONSOLE_SOCKET)
> 
> > +os.remove(CONSOLE_SOCKET)
> 
> Nit: We can use our own fio.unlink() just to show better style: prefer
> fio, which is integrated to our event loop, when using tarantool.

OK

> 
> > +
> > +local EOL = ";"
> > +
> > +test = tap.test("console-lua")
> 
> test -> local test

Thanks!

> 
> It is not matter much when writing a test, but it is the rule of thumb
> to avoid global variables where possible.
> 
> > +
> > +test:plan(7)
> > +
> > +--
> > +-- Start console and connect to it
> > +local server = console.listen(CONSOLE_SOCKET)
> > +test:ok(server ~= nil, "console.listen started")
> > +
> > +local client = socket.tcp_connect("unix/", CONSOLE_SOCKET)
> > +local handshake = client:read{chunk = 128}
> 
> Nit: We usually use explicit parentheses even when a functions receives
> one table argument and the parentheses can be omitted. The only
> exception is box.cfg{<...>} and similar functions (e.g.
> json.cfg{<...>}).

OK

> 
> > +test:ok(string.match(handshake, '^Tarantool .*console') ~= nil, 'Handshake')
> > +test:ok(client ~= nil, "connect to console")
> > +
> > +--
> > +-- Change output mode to Lua
> > +client:write('\\set output lua\n')
> > +test:is(client:read(EOL), 'true' .. EOL, "set lua output mode")
> 
> I would split test preparation / clean up code from actual test cases.
> It also seems logical to me to use assert() for the preparation / clean
> up code and test:is() (test:ok() and so) for test cases itself. See
> example below.
> 
> Nit: single and double quotes are used inconsistently. Personally I
> prefer to use single quotes, when I don't need to write a single quote
> inside a string.

OK

> 
> > +
> > +--
> > +-- Write mulireturn statement
> > +client:write('a={1,2,3}\n')
> > +test:is(client:read(EOL), EOL, "declare array")
> > +
> > +client:write('1,2,nil,a\n')
> > +test:is(client:read(EOL), '1, 2, box.NULL, {1, 2, 3}' .. EOL, "multireturn numbers")
> 
> I would write test cases in a declarative way. This allows to
> concentrate on cases itself when reviewing or adding new cases. Look
> below for example.
> 
> From previous review (see [1]):
> 
> > I propose to add a case with nils at the end as well as simple 1, 2, 3
> > multireturn.

Ah, thanks! Seems I've got missed this point

> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013326.html
> 
> I meant '1, nil, nil, nil\n' or kinda. Your implementation (after the patch)
> works correctly: it does not cut trailing nils. I don't sure whether it is part
> of our contract, to be honest. But while we're tentative it is good to keep
> this property and add the test case.

Sure

> 
> > +
> > +--
> > +-- Disconect from console
> > +client:shutdown()
> > +client:close()
> > +
> > +--
> > +-- Stop console
> > +server:shutdown()
> > +server:close()
> > +fiber.sleep(0) -- workaround for gh-712: console.test.lua fails in Fedora
> 
> I don't understand why client:shutdown() and server:shutdown() are
> needed: they seem to be redundant. Also I don't see how fiber.sleep(0)
> affects the test. It seems it also don't needed here.

Again, copy-pasted from another our test, I thought it is needed due
to some specifics on our testing engine.

> 
> > +-- Check that admin console has been stopped
> > +test:isnil(socket.tcp_connect("unix/", CONSOLE_SOCKET), "console.listen stopped")
> > +
> > +test:check()
> > +os.exit(0)
> 
> Let's set non-zero exit status at failure:
> 
>  | os.exit(test:check() and 0 or 1)

OK

> 
> This way is more safe: even if a test harness will parse TAP13 output
> incorrectly it will catch the process exit code.

Thanks a huge for comments, Sasha! I'll rework the series and resend.