[Tarantool-patches] [PATCH 0/2] box/console: Fixes for 2.3 series

[Tarantool-patches] [PATCH 0/2] box/console: Fixes for 2.3 series

[Cyrill Gorcunov]

The issues are

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

Branch

gorcunov/console-fixes

In this version I accumulated two issue since they depend on
each other by testing means.

Sasha, I left @opts variable untouched simply because we might
extend it in future (while for now it is only a single string).

Cyrill Gorcunov (2):
  box/console: handle multireturn in lua mode
  box/console: handle empty output format

 src/box/lua/console.lua           | 41 +++++++++++++------
 test/app-tap/console-lua.skipcond |  7 ++++
 test/app-tap/console-lua.test.lua | 68 +++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 13 deletions(-)
 create mode 100644 test/app-tap/console-lua.skipcond
 create mode 100755 test/app-tap/console-lua.test.lua

-- 
2.20.1

[Tarantool-patches] [PATCH 1/2] 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 ++++++++++++-------
 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
+
+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:
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
@@ -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')
+
+-- Suppress console log messages
+log.level(4)
+local CONSOLE_SOCKET = fio.pathjoin(fio.cwd(), 'tarantool-test-console-lua.sock')
+os.remove(CONSOLE_SOCKET)
+
+local EOL = ";"
+
+test = tap.test("console-lua")
+
+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}
+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")
+
+--
+-- 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")
+
+--
+-- 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
+-- Check that admin console has been stopped
+test:isnil(socket.tcp_connect("unix/", CONSOLE_SOCKET), "console.listen stopped")
+
+test:check()
+os.exit(0)
-- 
2.20.1

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

[Alexander Turenko]

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

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

[Cyrill Gorcunov]

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.

[Tarantool-patches] [PATCH 2/2] 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 +++
 test/app-tap/console-lua.test.lua | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

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
diff --git a/test/app-tap/console-lua.test.lua b/test/app-tap/console-lua.test.lua
index d3271a369..837e8ea18 100755
--- a/test/app-tap/console-lua.test.lua
+++ b/test/app-tap/console-lua.test.lua
@@ -20,7 +20,7 @@ local EOL = ";"
 
 test = tap.test("console-lua")
 
-test:plan(7)
+test:plan(8)
 
 --
 -- Start console and connect to it
@@ -45,6 +45,12 @@ 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")
 
+--
+-- Try to setup empty output mode
+client:write('\\set output\n')
+test:is(client:read(EOL), '\"Specify output format: lua or yaml.\"' .. EOL,
+        "set empty output mode")
+
 --
 -- Disconect from console
 client:shutdown()
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 0/2] box/console: Fixes for 2.3 series

[Cyrill Gorcunov]

On Wed, Dec 25, 2019 at 07:08:51PM +0300, Cyrill Gorcunov wrote:
> The issues are
> 
> https://github.com/tarantool/tarantool/issues/4604
> https://github.com/tarantool/tarantool/issues/4638
> 
> Branch
> 
> gorcunov/console-fixes

Ping?