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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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)