From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (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 952A046970E for ; Fri, 17 Jan 2020 21:59:07 +0300 (MSK) Date: Fri, 17 Jan 2020 21:59:11 +0300 From: Alexander Turenko Message-ID: <20200117185911.rqgmcunv4th5nrdc@tkn_work_nb> References: <20200115085605.32307-1-gorcunov@gmail.com> <20200115085605.32307-4-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200115085605.32307-4-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH 3/3] test/app-tap: add console_lua test List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml Sorry for nitpicking, but I still have several comments. All comments are not blocking, so if there will be some urgency, the patchset is okay to push. But it would be better to address them if time permits. WBR, Alexander Turenko. > test/app-tap: add console_lua test Nit: we often use 'test:' prefix and it is useful for searching / excluding of such commits. I would use it here too. > > Test multireturn and empty output in lua mode. Nit: I would say "lack of a parameter of '\set output' command" or at least "empty output format", because 'empty output' is hard to understand w/o looking into previous commits. > > Co-developed-by: Alexander Turenko > Signed-off-by: Cyrill Gorcunov > --- > test/app-tap/console_lua.test.lua | 154 ++++++++++++++++++++++++++++++ > 1 file changed, 154 insertions(+) > create mode 100755 test/app-tap/console_lua.test.lua > > diff --git a/test/app-tap/console_lua.test.lua b/test/app-tap/console_lua.test.lua > new file mode 100755 > index 000000000..76b67f6f4 > --- /dev/null > +++ b/test/app-tap/console_lua.test.lua > @@ -0,0 +1,154 @@ > +#!/usr/bin/env tarantool > +-- > +-- vim: ts=4 sw=4 et > + > +local console = require('console') > +local socket = require('socket') > +local tap = require('tap') > +local fio = require('fio') > + > +local EOL = ';' > +local CONSOLE_SOCKET = 'console-lua.sock' > + > +-- > +-- Set Lua output mode. > +local function set_lua_output(client, opts) > + if opts ~= nil then > + 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 If opts are not set, then, it worth to set some default rather then leave an existing mode: it would be strange if set_lua_output() function will not set some lua output mode with default opts. I propose to set lua line mode by default. Like so: | local opts = opts or {} | local mode = opts.block and 'lua,block' or 'lua' > +end > + > +-- > +-- Start console and setup a client. > +local function start_console() > + -- Make sure not stale sockets are > + -- left from previous runs. > + fio.unlink(CONSOLE_SOCKET) > + > + local server = console.listen('unix/:./' .. CONSOLE_SOCKET) > + assert(server ~= nil, 'console.listen started') > + > + 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') > + > + -- Switch to Lua output mode. > + set_lua_output(client, {block=false}) Nit: We surround '=' with spaces in a table. See https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/ , around 'use space in map definitions around equality sign and commas'. > + return server, client > +end > + > +-- > +-- Disconnect from console and stop it. > +local function stop_console(server, client) > + client:close() > + server:close() > + > + 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) Ref 1 (see below): | if #statements == 0 then | test:skip(name) | return | end Just in case: I don't propose to add this hunk. > + 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) > + > + local res = client:write(input .. '\n') > + test:ok(res ~= nil, ('-> [[%s]]'):format(input)) > + > + local exp = exp_output .. EOL > + local res = client:read(EOL) > + test:is(res, exp, ('<- [[%s]]'):format(exp:gsub('\n', '\\n'))) > + end) > +end > + > +-- > +-- Test cases table: > +-- @name: Name of the test, mandatory > +-- @prepare: Preparation script, optional > +-- @opts: Console options, optional > +-- @input: Input statement to execute, mandatory > +-- @expected: Expected results to compare with, mandatory > +-- @cleanup: Cleanup script to run after the test, optional That's neat :) > +local cases = { > + { > + name = 'multireturn line mode', > + prepare = 'a = {1, 2, 3}', > + opts = {block = false}, > + input = '1, 2, nil, a', > + expected = '1, 2, box.NULL, {1, 2, 3}', > + cleanup = 'a = nil', > + }, { > + name = 'multireturn block mode', > + prepare = 'a = {1, 2, 3}', > + opts = {block = true}, > + input = '1, 2, nil, a', > + expected = '1, 2, box.NULL, {\n 1,\n 2,\n 3\n}', > + cleanup = 'a = nil', > + }, { > + name = 'empty output', > + input = '\\set output', > + expected = '"Specify output format: lua or yaml."', > + } > +} > + > +local test = tap.test('console-lua') > +test:plan(#cases) > + > +local server, client = start_console() > + > +for _, case in ipairs(cases) do > + test:test(case.name, function(test) > + local plan = 1 > + > + for k, _ in pairs(case) do > + if k == 'prepare' or k == 'cleanup' then > + plan = plan + 1 > + end > + end > + > + test:plan(plan) We can simplify this: just plan 3 tests. See: diff --git a/test/app-tap/console_lua.test.lua b/test/app-tap/console_lua.test.lua index 76b67f6f4..cf9e091f7 100755 --- a/test/app-tap/console_lua.test.lua +++ b/test/app-tap/console_lua.test.lua @@ -126,26 +126,14 @@ local server, client = start_console() for _, case in ipairs(cases) do test:test(case.name, function(test) - local plan = 1 + test:plan(3) - for k, _ in pairs(case) do - if k == 'prepare' or k == 'cleanup' then - plan = plan + 1 - end - end - - test:plan(plan) - - if case.prepare ~= nil then - execute_statements(test, client, totable(case.prepare), case.name .. ': prepare script') - end + execute_statements(test, client, totable(case.prepare), case.name .. ': prepare script') set_lua_output(client, case.opts) execute_and_verify(test, client, case.input, case.expected, case.name) - if case.cleanup ~= nil then - execute_statements(test, client, totable(case.cleanup), case.name .. ': cleanup script') - end + execute_statements(test, client, totable(case.cleanup), case.name .. ': cleanup script') end) end It is completely okay to emit '1..0' plan: this is mentioned in the TAP spec (see [1]), used in other implementations (see [2]), works good with our 'tap' module and test-run (at least for nested tests). We however can add explicit skip of prepare/cleanup in execute_statements(), see ref 1 above. But I don't think it is necessary. [1]: https://testanything.org/tap-version-13-specification.html#skipping-tests [2]: https://node-tap.org/tap-protocol/#plan > + > + if case.prepare ~= nil then > + execute_statements(test, client, totable(case.prepare), case.name .. ': prepare script') > + end Too long line. We usually fit lines within 80 symbols. case.name is not necessary here, because it is printed within case.name section in TAP13 output. Compare: Current: TAP version 13 1..3 # multireturn line mode 1..3 # multireturn line mode: prepare script 1..2 ok - -> [[a = {1, 2, 3}\n]] ok - <- [[;]] # multireturn line mode: prepare script: end ok - multireturn line mode: prepare script # multireturn line mode 1..2 ok - -> [[1, 2, nil, a]] ok - <- [[1, 2, box.NULL, {1, 2, 3};]] # multireturn line mode: end ok - multireturn line mode # multireturn line mode: cleanup script 1..2 ok - -> [[a = nil\n]] ok - <- [[;]] # multireturn line mode: cleanup script: end ok - multireturn line mode: cleanup script # multireturn line mode: end ok - multireturn line mode <...> W/o case.name: TAP version 13 1..3 # multireturn line mode 1..3 # prepare script 1..2 ok - -> [[a = {1, 2, 3}\n]] ok - <- [[;]] # prepare script: end ok - prepare script # script 1..2 ok - -> [[1, 2, nil, a]] ok - <- [[1, 2, box.NULL, {1, 2, 3};]] # script: end ok - script # cleanup script 1..2 ok - -> [[a = nil\n]] ok - <- [[;]] # cleanup script: end ok - cleanup script # multireturn line mode: end ok - multireturn line mode <...> I got this output using the following code: | execute_statements(test, client, totable(case.prepare), 'prepare script') | | set_lua_output(client, case.opts) | execute_and_verify(test, client, case.input, case.expected, 'script') | | execute_statements(test, client, totable(case.cleanup), 'cleanup script') I think that the latter output is cleaner, but I don't mind to include a case name again if you'll find this useful. Just is case: you can get this output using one of the following commands: $ ./src/tarantool test/app-tap/console_lua.test.lua $ (cd test && ./test-run.py console_lua --verbose) > + > + set_lua_output(client, case.opts) > + execute_and_verify(test, client, case.input, case.expected, case.name) Same as above: case.name is not necessary. > + > + if case.cleanup ~= nil then > + execute_statements(test, client, totable(case.cleanup), case.name .. ': cleanup script') > + end Same as above: too long line. Same as above: case.name is not necessary. > + end) > +end > + > +stop_console(server, client) > + > +os.exit(test:check() and 0 or 1) > -- > 2.20.1 >