From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com [209.85.167.67]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 8B97046970E for ; Tue, 14 Jan 2020 23:27:21 +0300 (MSK) Received: by mail-lf1-f67.google.com with SMTP id y19so10872870lfl.9 for ; Tue, 14 Jan 2020 12:27:21 -0800 (PST) Date: Tue, 14 Jan 2020 23:27:18 +0300 From: Cyrill Gorcunov Message-ID: <20200114202718.GJ31598@uranus> References: <20191225160853.1407-1-gorcunov@gmail.com> <20191225160853.1407-2-gorcunov@gmail.com> <20200114005412.7zdb74fd5kn35l4v@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200114005412.7zdb74fd5kn35l4v@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH 1/2] box/console: handle multireturn in lua mode List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tml 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.