From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 688D346970E for ; Tue, 14 Jan 2020 03:54:12 +0300 (MSK) Date: Tue, 14 Jan 2020 03:54:12 +0300 From: Alexander Turenko Message-ID: <20200114005412.7zdb74fd5kn35l4v@tkn_work_nb> References: <20191225160853.1407-1-gorcunov@gmail.com> <20191225160853.1407-2-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191225160853.1407-2-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH 1/2] box/console: handle multireturn in lua mode List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml The code is okay. I have a couple of comments about the test. Sorry if it is too strict: as usual I wrote everything I have to say and let you decide whether it is worth to do. Some of comments are just about my personal taste: feel free to ignore such kind of notes. I would rewrite the test in a declarative way, see my variant at end of the email. Apply it if you like the approach, otherwise apply comments below on top of the existing test. Two notes about the proposed test: * It is based on the first patch, does not include the test case from the second one. * It assumes that 'before' and 'after' fields of a test case may be tables when more then one statement necessary to prepare a test case and/or to clean it up. BTW, should not we also verify block output? It does not matter much for this particular problem, but since we're going to extend the test with more cases we can start to run each case in both modes. I implemented it in my variant of the test just in case. WBR, Alexander Turenko. On Wed, Dec 25, 2019 at 07:08:52PM +0300, Cyrill Gorcunov wrote: > Currently we handle only first member of multireturn statement. > Fix it processing each element separately. > > n.b.: While at this file add vim settings. > > | tarantool> \set output lua > | true; > | tarantool> 1,2,3,4 > | 1, 2, 3, 4; > > Fixes #4604 > > Reported-by: Alexander Turenko > Signed-off-by: Cyrill Gorcunov > --- > src/box/lua/console.lua | 38 ++++++++++++------- > test/app-tap/console-lua.skipcond | 7 ++++ > test/app-tap/console-lua.test.lua | 62 +++++++++++++++++++++++++++++++ > 3 files changed, 94 insertions(+), 13 deletions(-) > create mode 100644 test/app-tap/console-lua.skipcond > create mode 100755 test/app-tap/console-lua.test.lua > > diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua > index d4d8ec984..a4a36c625 100644 > --- a/src/box/lua/console.lua > +++ b/src/box/lua/console.lua > @@ -1,4 +1,6 @@ > -- console.lua -- internal file > +-- > +-- vim: ts=4 sw=4 et > > local serpent = require('serpent') > local internal = require('console') > @@ -75,28 +77,38 @@ local serpent_map_symbols = function(tag, head, body, tail, level) > return tag..head..body..tail > end > > -output_handlers["lua"] = function(status, opts, ...) > - -- > - -- If no data present at least EOS should be put, > - -- otherwise wire readers won't be able to find > - -- where the end of string is. > - if not ... then > - return output_eos["lua"] > - end > +-- > +-- Format a Lua value. > +local function format_lua_value(value, opts) > for k,v in pairs(lua_map_direct_symbols) do > - if k == ... then > - return v .. output_eos["lua"] > + if k == value then > + return v > end > end > local serpent_opts = { > custom = serpent_map_symbols, > comment = false, > - nocode = true, > + nocode = true, > } > if opts == "block" then > - return serpent.block(..., serpent_opts) .. output_eos["lua"] > + return serpent.block(value, serpent_opts) > + end > + return serpent.line(value, serpent_opts) > +end 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. > + > +output_handlers["lua"] = function(status, opts, ...) > + local collect = {} > + -- > + -- If no data present at least EOS should be put, > + -- otherwise wire readers won't be able to find > + -- where the end of string is. > + if not ... then > + return output_eos["lua"] > + end > + for i = 1, select('#', ...) do > + collect[i] = format_lua_value(select(i, ...), opts) > end > - return serpent.line(..., serpent_opts) .. output_eos["lua"] > + return table.concat(collect, ', ') .. output_eos["lua"] > end > > local function output_verify_opts(fmt, opts) > 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. > 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). > @@ -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`. > + > +-- Suppress console log messages > +log.level(4) We don't call box.cfg() and so have no logs. This call seems to be unneeded. > +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): | 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. > + > +local EOL = ";" > + > +test = tap.test("console-lua") test -> local test 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{<...>}). > +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. > + > +-- > +-- 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. [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. > + > +-- > +-- 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. > +-- 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) This way is more safe: even if a test harness will parse TAP13 output incorrectly it will catch the process exit code. ---- #!/usr/bin/env tarantool -- -- vim: ts=4 sw=4 et local tap = require('tap') local console = require('console') local socket = require('socket') local fio = require('fio') local EOL = ';' local CONSOLE_SOCKET = 'console-lua.sock' -- Set Lua output mode. local function set_lua_output(client, opts) local opts = opts or {} local mode = opts.block and 'lua,block' or 'lua' client:write(('\\set output %s\n'):format(mode)) assert(client:read(EOL), 'true' .. EOL, 'set lua output mode') end -- Start console and setup a client. local function start_console() -- Remove old Unix socket, ignore an error. fio.unlink(CONSOLE_SOCKET) -- Start console. local server = console.listen('unix/:./' .. CONSOLE_SOCKET) assert(server ~= nil, 'console.listen started') -- Connect, skip handshake. local client = socket.tcp_connect('unix/', CONSOLE_SOCKET) assert(client ~= nil, 'connect to console') local handshake = client:read({chunk = 128}) assert(string.match(handshake, '^Tarantool .*console') ~= nil, 'handshake') -- Set Lua output mode. set_lua_output(client, {block = false}) return server, client end -- Close a client and a console server itself. local function stop_console(server, client) -- Disconnect from console and stop it. client:close() server:close() -- Check that the console has been stopped. local new_client = socket.tcp_connect('unix/', CONSOLE_SOCKET) assert(new_client == nil, 'console.listen stopped') end -- Give `{x}` for a scalar `x`, just `x` otherwise. local function totable(x) return type(x) == 'table' and x or {x} end -- Execute a list of statements, show requests and responses. local function execute_statements(test, client, statements, name) test:test(name, function(test) test:plan(2 * #statements) for _, stmt in ipairs(statements) do local request = stmt .. '\n' local res = client:write(request) test:ok(res ~= nil, ('-> [[%s]]'):format(request:gsub('\n', '\\n'))) local response = client:read(EOL) test:ok(response ~= nil and response:endswith(EOL), ('<- [[%s]]'):format(response)) end end) end -- Execute a statement and verify its response. local function execute_and_verify(test, client, input, exp_output, name) test:test(name, function(test) test:plan(2) -- Write a statement. local res = client:write(input .. '\n') test:ok(res ~= nil, ('-> [[%s]]'):format(input)) -- Read and verify an answer. local exp = exp_output .. EOL local res = client:read(EOL) test:is(res, exp, ('<- [[%s]]'):format(exp:gsub('\n', '\\n'))) end) end local cases = { { 'multireturn', before = 'a = {1, 2, 3}', input = '1, 2, nil, a', exp_line = '1, 2, box.NULL, {1, 2, 3}', exp_block = '1, 2, box.NULL, {\n 1,\n 2,\n 3\n}', after = 'a = nil', } } local test = tap.test('console-lua') test:plan(#cases) local server, client = start_console() for _, case in ipairs(cases) do test:test(case[1], function(test) test:plan(4) -- Prepare the test case. execute_statements(test, client, totable(case.before), 'before script') -- Verify line and block mode. execute_and_verify(test, client, case.input, case.exp_line, 'line mode') set_lua_output(client, {block = true}) execute_and_verify(test, client, case.input, case.exp_block, 'block mode') set_lua_output(client, {block = false}) -- Cleanup the test case. execute_statements(test, client, totable(case.after), 'after script') end) end stop_console(server, client) os.exit(test:check() and 0 or 1)