[Tarantool-patches] [PATCH v2 0/3] box/console: Fix multireturn and empty output

[Tarantool-patches] [PATCH v2 0/3] box/console: Fix multireturn and empty output

[Cyrill Gorcunov]

In v2 Sasha provided more suitable tests which I've updated a bit.

The issues are

https://github.com/tarantool/tarantool/issues/4604
https://github.com/tarantool/tarantool/issues/4638

Branch

gorcunov/console-fixes-2

Cyrill Gorcunov (3):
  box/console: handle multireturn in lua mode
  box/console: handle empty output format
  test/app-tap: add console_lua test

 src/box/lua/console.lua           |  41 +++++---
 test/app-tap/console_lua.test.lua | 154 ++++++++++++++++++++++++++++++
 2 files changed, 182 insertions(+), 13 deletions(-)
 create mode 100755 test/app-tap/console_lua.test.lua

-- 
2.20.1

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

[Cyrill Gorcunov]

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 <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/lua/console.lua | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

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
+
+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)
-- 
2.20.1

[Tarantool-patches] [PATCH 2/3] box/console: handle empty output format

[Cyrill Gorcunov]

In case if output format is not specified we should exit
with more readable error message.

Fixes #4638

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/lua/console.lua | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index a4a36c625..17e2c91b2 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -126,6 +126,9 @@ end
 
 local function parse_output(value)
     local fmt, opts
+    if not value then
+        return 'Specify output format: lua or yaml.'
+    end
     if value:match("([^,]+),([^,]+)") ~= nil then
         fmt, opts = value:match("([^,]+),([^,]+)")
     else
-- 
2.20.1

[Tarantool-patches] [PATCH 3/3] test/app-tap: add console_lua test

[Cyrill Gorcunov]

Test multireturn and empty output in lua mode.

Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 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
+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})
+    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)
+    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
+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)
+
+        if case.prepare ~= nil then
+            execute_statements(test, client, totable(case.prepare), case.name .. ': prepare script')
+        end
+
+        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
+    end)
+end
+
+stop_console(server, client)
+
+os.exit(test:check() and 0 or 1)
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 3/3] test/app-tap: add console_lua test

[Alexander Turenko]

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 <alexander.turenko@tarantool.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  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
> 

Re: [Tarantool-patches] [PATCH 3/3] test/app-tap: add console_lua test

[Cyrill Gorcunov]

On Fri, Jan 17, 2020 at 09:59:11PM +0300, Alexander Turenko wrote:
> Sorry for nitpicking, but I still have several comments.
> 

Thanks a huge, Sasha! I'll address them.