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

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

[Cyrill Gorcunov]

In v3 nits for tests are fixed.

The issues are

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

Branch

gorcunov/console-fixes-3

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 | 141 ++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+), 13 deletions(-)
 create mode 100755 test/app-tap/console_lua.test.lua

-- 
2.20.1

[Tarantool-patches] [PATCH v3 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 v3 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 v3 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 | 141 ++++++++++++++++++++++++++++++
 1 file changed, 141 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..5848bdf69
--- /dev/null
+++ b/test/app-tap/console_lua.test.lua
@@ -0,0 +1,141 @@
+#!/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)
+    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()
+    -- 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)
+        test:plan(3)
+
+        execute_statements(test, client, totable(case.prepare), 'prepare')
+
+        set_lua_output(client, case.opts)
+        execute_and_verify(test, client, case.input, case.expected, 'run')
+
+        execute_statements(test, client, totable(case.cleanup), 'cleanup')
+    end)
+end
+
+stop_console(server, client)
+
+os.exit(test:check() and 0 or 1)
-- 
2.20.1

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

[Alexander Turenko]

Thanks!

There are several missed comments.

Cited from [1]:

> > 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.

Added:

diff --git a/test/app-tap/console_lua.test.lua b/test/app-tap/console_lua.test.lua
index 5848bdf69..3ed6aad97 100755
--- a/test/app-tap/console_lua.test.lua
+++ b/test/app-tap/console_lua.test.lua
@@ -111,6 +111,16 @@ local cases = {
         input       = '1, 2, nil, a',
         expected    = '1, 2, box.NULL, {\n  1,\n  2,\n  3\n}',
         cleanup     = 'a = nil',
+    }, {
+        name        = 'trailing nils, line mode',
+        opts        = {block = false},
+        input       = '1, nil, nil, nil',
+        expected    = '1, box.NULL, box.NULL, box.NULL',
+    }, {
+        name        = 'trailing nils, block mode',
+        opts        = {block = true},
+        input       = '1, nil, nil, nil',
+        expected    = '1, box.NULL, box.NULL, box.NULL',
     }, {
         name        = 'empty output',
         input       = '\\set output',

Cited from [2]:

> > 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.

Changed to:

 | test: add app-tap/console_lua test
 |
 | Test multireturn in lua output mode and lack of a parameter in '\set
 | output <...>' command.
 |
 | Co-developed-by: Alexander Turenko <alexander.turenko@tarantool.org>
 | Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
 | Reviewed-by: Alexander Turenko <alexander.turenko@tarantool.org>

I don't think that a second review is necessary for those patches, they are
simple.

Pushed to master, 2.3, 2.2. CCed Kirill.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013595.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-January/013672.html

WBR, Alexander Turenko.

On Mon, Jan 20, 2020 at 08:59:47PM +0300, Cyrill Gorcunov wrote:
> In v3 nits for tests are fixed.
> 
> The issues are
> 
> https://github.com/tarantool/tarantool/issues/4604
> https://github.com/tarantool/tarantool/issues/4638
> 
> Branch
> 
> gorcunov/console-fixes-3
> 
> 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 | 141 ++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+), 13 deletions(-)
>  create mode 100755 test/app-tap/console_lua.test.lua
> 
> -- 
> 2.20.1
> 

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

[Cyrill Gorcunov]

On Tue, Jan 21, 2020 at 06:16:42PM +0300, Alexander Turenko wrote:
> Thanks!
> 
> There are several missed comments.
> 
> Cited from [1]:
> 

Thanks a huge, Sasha!!!