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

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

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
>