From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 10306469710 for ; Wed, 6 May 2020 19:16:47 +0300 (MSK) Date: Wed, 6 May 2020 19:16:27 +0300 From: Sergey Bronnikov Message-ID: <20200506161627.GA87761@pony.bronevichok.ru> References: <20200427143827.GN11314@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200427143827.GN11314@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 6/10] Fix luacheck warnings in test/ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Igor, thanks a lot for review! See my comments inline. On 17:38 Mon 27 Apr , Igor Munkin wrote: > Sergey, > > Thanks for the patch! I didn't finish the review, but I'm not going to > continue with this version (the reason is at the bottom of my reply). > However I left several comments for the part I've checked, please > consider them. > > On 21.04.20, sergeyb@tarantool.org wrote: > > diff --git a/.luacheckrc b/.luacheckrc > > index 64692b27c..cec1f4ffe 100644 > > --- a/.luacheckrc > > +++ b/.luacheckrc > > @@ -37,3 +37,37 @@ files["src/box/lua/console.lua"] = {ignore = {"212"}} > > files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}} > > files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}} > > files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}} > > +files["test/app/lua/fiber.lua"] = {globals = {"box_fiber_run_test"}} > > test/app/{app,loaders}.lua are also Lua source files and need to be > tested via luacheck. Replaced file mask "test/app/*.lua" with "test/app/*.test.lua" to exclude test files only and allow luacheck to check non-test code. All warnings in a new code has beed fixed or supressed. > > +files["test/app-tap/console.test.lua"] = {globals = {"long_func"}} > > +files["test/app-tap/lua/require_mod.lua"] = {globals = {"exports"}} > > +files["test/app-tap/module_api.test.lua"] = {ignore = {"311"}} > > In previous review I provided a diff with fix for this class of > warnings. You've said nothing against my approaches, but fixed it on > your own way with no discussion. It's simply unproductive, because I > need to double-check all changes now. Sorry for triggering your double efforts. Actually it is not easy for me to merge your fixes provided inline in a mail. Fixes on top of branch would require much less efforts from my side - just squash them with appropriate commits. Anyway our main goal is fixing warnings regardless my own way or your own way. Isn't it? > > +files["test/app-tap/string.test.lua"] = {globals = {"utf8"}} > > +files["test/app-tap/tarantoolctl.test.lua"] = {ignore = {"113", "421"}} > > +files["test/box-tap/session.test.lua"] = { > > + globals = {"active_connections", "session", "space", "f1", "f2"}, > > + ignore = {"211"} > > +} > > +files["test/box/lua/push.lua"] = {globals = {"push_collection"}} > > +files["test/box/lua/index_random_test.lua"] = {globals = {"index_random_test"}} > > +files["test/box/lua/utils.lua"] = { > > + globals = {"space_field_types", "iterate", "arithmetic", "table_shuffle", > > + "table_generate", "tuple_to_string", "check_space", "space_bsize", > > + "create_iterator", "setmap", "sort"}} > > +files["test/box/lua/bitset.lua"] = { > > + globals = {"create_space", "fill", "delete", "clear", "drop_space", > > + "dump", "test_insert_delete"} > > +} > > +files["test/box/lua/fifo.lua"] = {globals = {"fifomax", "find_or_create_fifo", "fifo_push", "fifo_top"}} > > +files["test/box/lua/identifier.lua"] = {globals = {"run_test"}} > > +files["test/box/lua/require_mod.lua"] = {globals = {"exports"}} > > +files["test/luajit-tap/gh-4476-fix-string-find-recording.test.lua"] = {ignore = {"231"}} > > +files["test/luajit-tap/or-232-unsink-64-kptr.test.lua"] = {ignore = {"542"}} > > This files have to be added right after luajit submodule bump. Added supressions for tests in luajit-tap dir in a separate commit. > > +files["test/replication/lua/fast_replica.lua"] = { > > + globals = {"join", "start_all", "stop_all", "wait_all", > > + "drop_all", "drop_all", "vclock_diff", "unregister", > > + "delete", "start", "stop", "call_all", "drop", "wait"}, > > + ignore = {"212", "213"} > > +} > > +files["test/sql-tap/*.lua"] = {ignore = {"611", "612", "613", "614", "621", "631", "211", "113", "111"}} > > +files["test/sql-tap/lua/sqltester.lua"] = {globals = {"table_match_regex_p"}} > > +files["test/sql-tap/e_expr.test.lua"] = {ignore = {"512"}} > > > > > diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua > > index 4feadfa5e..e81b48a80 100755 > > --- a/test/app-tap/console.test.lua > > +++ b/test/app-tap/console.test.lua > > > > > @@ -59,7 +59,7 @@ test:is(client:read(";"), 'true;', "pushed message") > > client:write('\\set output lua\n') > > client:read(";") > > > > -long_func_f = nil > > +local long_func_f > > function long_func() > > Please consider the fix I proposed in previous review[1]. This warning > is a false positive and can be suppressed inline. Here is the diff: > > ================================================================================ > > diff --git a/.luacheckrc b/.luacheckrc > index cec1f4ffe..4faf770e8 100644 > --- a/.luacheckrc > +++ b/.luacheckrc > @@ -38,7 +38,6 @@ files["src/box/lua/load_cfg.lua"] = {ignore = {"542"}} > files["src/box/lua/net_box.lua"] = {ignore = {"431", "432", "231", "411", "212"}} > files["src/box/lua/schema.lua"] = {ignore = {"431", "432", "542", "212"}} > files["test/app/lua/fiber.lua"] = {globals = {"box_fiber_run_test"}} > -files["test/app-tap/console.test.lua"] = {globals = {"long_func"}} > files["test/app-tap/lua/require_mod.lua"] = {globals = {"exports"}} > files["test/app-tap/module_api.test.lua"] = {ignore = {"311"}} > files["test/app-tap/string.test.lua"] = {globals = {"utf8"}} > diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua > index e81b48a80..454d5a892 100755 > --- a/test/app-tap/console.test.lua > +++ b/test/app-tap/console.test.lua > @@ -60,6 +60,7 @@ client:write('\\set output lua\n') > client:read(";") > > local long_func_f > +-- luacheck: globals long_func (is called via client socket) > function long_func() > long_func_f = fiber.self() > box.session.push('push') Fix applied. > ================================================================================ > > > long_func_f = fiber.self() > > box.session.push('push') > > > > > diff --git a/test/app-tap/console_lua.test.lua b/test/app-tap/console_lua.test.lua > > index 3ed6aad97..263e86c54 100755 > > --- a/test/app-tap/console_lua.test.lua > > +++ b/test/app-tap/console_lua.test.lua > > > > > @@ -57,8 +57,8 @@ end > > > > -- > > -- Execute a list of statements, show requests and responses. > > -local function execute_statements(test, client, statements, name) > > - test:test(name, function(test) > > +local function execute_statements(test_obj, client, statements, name) > > Minor: I guess you can use for argument as well as for > parameter. Agree, replaced "test_obj" with "testcase". > > + test_obj:test(name, function(test) > > test:plan(2 * #statements) > > > > for _, stmt in ipairs(statements) do > > @@ -75,15 +75,15 @@ end > > > > -- > > -- Execute a statement and verify its response. > > -local function execute_and_verify(test, client, input, exp_output, name) > > - test:test(name, function(test) > > +local function execute_and_verify(test_obj, client, input, exp_output, name) > > Minor: I guess you can use for argument as well as for > parameter. Agree, replaced "test_obj" with "testcase". > > + test_obj: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) > > + res = client:read(EOL) > > test:is(res, exp, ('<- [[%s]]'):format(exp:gsub('\n', '\\n'))) > > end) > > end > > > > > diff --git a/test/app-tap/errno.test.lua b/test/app-tap/errno.test.lua > > index 5fd8eaca4..4a13edf2a 100755 > > --- a/test/app-tap/errno.test.lua > > +++ b/test/app-tap/errno.test.lua > > @@ -3,10 +3,10 @@ > > local tap = require('tap') > > local errno = require('errno') > > > > -local test = tap.test("errno") > > +local suite = tap.test("errno") > > > > -test:plan(1) > > -test:test("primary", function(test) > > +suite:plan(1) > > +suite:test("primary", function(test) > > test:plan(10) > > test:is(type(errno), "table", "type of table") > > test:ok(errno.EINVAL ~= nil, "errno.EINVAL is available") > > These changes introduce a mess in naming: in previous chunks you rename > argument to , but here you decided to change naming for > the root suite. I guess you need either to adjust it to the common > naming ( and ) or leave the shadowing, considering this > warning is raised in many other places and we have no policy for it for > now. Agree, there is a mess a bit here. Reverted "suite" to "test" and replaced "test" with "testcase" in the following tests: test/app-tap/console_lua.test.lua test/app-tap/errno.test.lua test/app-tap/gh-4761-json-per-call-options.test.lua test/app-tap/http_client.test.lua test/app-tap/string.test.lua test/app-tap/trigger.test.lua test/box-tap/key_def.test.lua test/box-tap/merger.test.lua > > > > diff --git a/test/app-tap/gh-4761-json-per-call-options.test.lua b/test/app-tap/gh-4761-json-per-call-options.test.lua > > index 1fb24744e..a8c02dd2f 100755 > > --- a/test/app-tap/gh-4761-json-per-call-options.test.lua > > +++ b/test/app-tap/gh-4761-json-per-call-options.test.lua > > @@ -13,23 +13,24 @@ local res = tap.test('gh-4761-json-per-call-options', function(test) > > test:plan(2) > > > > -- Preparation code: call :decode() with a custom option. > > - local ok, err = pcall(json.decode, '{"foo": {"bar": 1}}', > > + local ok, _ = pcall(json.decode, '{"foo": {"bar": 1}}', > > Underscore can be freely omitted here (as was proposed in the previous > review[1]). removed > > {decode_max_depth = 1}) > > assert(not ok, 'expect "too many nested data structures" error') > > > > -- Verify that the instance option remains unchanged. > > local exp_res = {foo = {bar = 1}} > > - local ok, res = pcall(json.decode, '{"foo": {"bar": 1}}') > > + local res > > + ok, res = pcall(json.decode, '{"foo": {"bar": 1}}') > > test:is_deeply({ok, res}, {true, exp_res}, > > 'json instance settings remain unchanged after :decode()') > > > > -- Same check for json.encode. > > local nan = 1/0 > > - local ok, err = pcall(json.encode, {a = nan}, > > + ok, _ = pcall(json.encode, {a = nan}, > > Underscore can be freely omitted here (as was proposed in the previous > review[1]). removed > > {encode_invalid_numbers = false}) > > assert(not ok, 'expected "number must not be NaN or Inf" error') > > - local exp_res = '{"a":inf}' > > - local ok, res = pcall(json.encode, {a = nan}) > > + exp_res = '{"a":inf}' > > + ok, res = pcall(json.encode, {a = nan}) > > test:is_deeply({ok, res}, {true, exp_res}, > > 'json instance settings remain unchanged after :encode()') > > end) > > diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua > > index b85b605cf..1c5bf853b 100755 > > --- a/test/app-tap/http_client.test.lua > > +++ b/test/app-tap/http_client.test.lua > > @@ -3,13 +3,13 @@ > > local tap = require('tap') > > local client = require('http.client') > > local json = require('json') > > -local test = tap.test("curl") > > +local suite = tap.test("curl") > > These changes introduce a mess in naming: in previous chunks you rename > argument to , but here you decided to change naming for > the root suite. I guess you need either to adjust it to the common > naming ( and ) or leave the shadowing, considering this > warning is raised in many other places and we have no policy for it for > now. see my answer above, fixed > > local fiber = require('fiber') > > local socketlib = require('socket') > > local os = require('os') > > > > local TARANTOOL_SRC_DIR = os.getenv("TARANTOOL_SRC_DIR") or "../.." > > -test:diag("TARANTOOL_SRC_DIR=%s", TARANTOOL_SRC_DIR) > > +suite:diag("TARANTOOL_SRC_DIR=%s", TARANTOOL_SRC_DIR) > > > > local function merge(...) > > local res = {} > > > > > @@ -270,7 +269,7 @@ local function test_errors(test) > > test:ok(not status and string.find(json.encode(err), > > "Unsupported protocol"), > > "POST: exception on bad protocol") > > - local r = http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfcba.ru") > > + http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfcba.ru") > > Why did you leave this line? What does it test? I thought this line tests http get for unavailable URL... Unexpectedly there is a story with this testcase. Sometime ago we had separate testcase with URL that contains unavailable host name. This testcase was a flaky and may hang forever on resolving unavailable host name. Testcase has been removed in commit [2], but http:get was kept for unknown reasons. Discussed with Alexender Turenko and decided to remove this line too. [2] https://github.com/tarantool/tarantool/commit/33254bd6632b616a6c9080e059d2d52e6b54e035 > > end > > > > -- gh-3679 Check that opts.headers values can be strings only. > > > > > diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua > > index 492d5ea0b..56db1d2c7 100755 > > --- a/test/app-tap/logger.test.lua > > +++ b/test/app-tap/logger.test.lua > > @@ -26,7 +26,7 @@ end > > log.info(message) > > local line = file:read() > > test:is(line:sub(-message:len()), message, "message") > > -s, err = pcall(json.decode, line) > > +local s, _ = pcall(json.decode, line) > > Underscore can be freely omitted here (as was proposed in the previous > review[1]). removed > > test:ok(not s, "plain") > > -- > > -- gh-700: Crash on calling log.info() with formatting characters > > > > > @@ -56,34 +56,34 @@ file:close() > > > > test:ok(log.pid() >= 0, "pid()") > > > > --- logger uses 'debug', try to set it to nil > > -debug = nil > > +-- luacheck: ignore (logger uses 'debug', try to set it to nil) > > +local debug = nil > > This change breaks the test: debug global variable need to be set to > nil, not the local one. Furthermore, the change differs to the one I've > seen in the previous branch. Has the branch been rebased? make 'debug' global again > > log.info("debug is nil") > > debug = require('debug') > > > > > > > diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua > > index a6658cc61..e0f2bf75f 100755 > > --- a/test/app-tap/module_api.test.lua > > +++ b/test/app-tap/module_api.test.lua > > There is a fix for the warning related to variable in previous > review[1]. Please consider it, since this warning will never be fixed. applied > > > > diff --git a/test/app-tap/snapshot.test.lua b/test/app-tap/snapshot.test.lua > > index 587f8279b..bd5270702 100755 > > --- a/test/app-tap/snapshot.test.lua > > +++ b/test/app-tap/snapshot.test.lua > > > > > @@ -126,11 +125,11 @@ local function gh1094() > > break > > end > > end > > - local sf, mf = pcall(box.snapshot) > > - for i, f in pairs(files) do > > + local sf, _ = pcall(box.snapshot) > > > Underscore can be freely omitted here (as was proposed in the previous > review[1]). removed > + for _, f in pairs(files) do > > f:close() > > end > > - local ss, ms = pcall(box.snapshot) > > + local ss, _ = pcall(box.snapshot) > > Underscore can be freely omitted here (as was proposed in the previous > review[1]). removed > > > test:ok(not sf and ss, msg) > > end > > gh1094() > > > > > diff --git a/test/app-tap/string.test.lua b/test/app-tap/string.test.lua > > index 02a1a84d7..bf1be5c9e 100755 > > --- a/test/app-tap/string.test.lua > > +++ b/test/app-tap/string.test.lua > > @@ -1,11 +1,11 @@ > > #!/usr/bin/env tarantool > > > > local tap = require('tap') > > -local test = tap.test("string extensions") > > +local suite = tap.test("string extensions") > > These changes introduce a mess in naming: in previous chunks you rename > argument to , but here you decided to change naming for > the root suite. I guess you need either to adjust it to the common > naming ( and ) or leave the shadowing, considering this > warning is raised in many other places and we have no policy for it for > now. see my answer above, fixed > > > > -test:plan(7) > > +suite:plan(7) > > > > -test:test("split", function(test) > > +suite:test("split", function(test) > > test:plan(10) > > > > -- testing basic split (works over gsplit) > > > > > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua > > index 4d7059559..6c3cfd450 100755 > > --- a/test/app-tap/tarantoolctl.test.lua > > +++ b/test/app-tap/tarantoolctl.test.lua > > @@ -4,7 +4,6 @@ local ffi = require('ffi') > > local fio = require('fio') > > local tap = require('tap') > > local uuid = require('uuid') > > -local yaml = require('yaml') > > local errno = require('errno') > > local fiber = require('fiber') > > local ok, test_run = pcall(require, 'test_run') > > Well, the fact you simply didn't read my reply[1] drives me crazy. I'm > totally OK whether you *fix* the problem other way. I can handle the > fact you introduce unused variables and then suppress them. But ignoring > bug fix for this test is unacceptable for me. OK, I mention it one more > time. > > **Here is the fix for the bug you successfully masked in the second > series in a row**: > > ================================================================================ > > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua > index 6c3cfd450..6f5cd0bdd 100755 > --- a/test/app-tap/tarantoolctl.test.lua > +++ b/test/app-tap/tarantoolctl.test.lua > @@ -32,7 +32,7 @@ local function recursive_rmdir(path) > end > end > if fio.rmdir(path) == false then > - print(string.format('!!! failed to rmdir path "%s"', file)) > + print(string.format('!!! failed to rmdir path "%s"', path)) > print(string.format('!!! [errno %s]: %s', errno(), errno.strerror())) > end > end > > ================================================================================ > > It's a bug since there is no variable in this scope and this line > is just a copy-paste from the loop body. Fixed. > I'm out from the further review for this version and will wait the next > one to proceed. Something goes wrong and some your patches was missed. Sorry for this. I have reviewed your suggested patches in [1] one more time to make sure all of them are applied. This patch was not applied as we decided previosly to supress warnings triggered by unused 'self' argument in luacherkrc: Fixed several warnings in test/app-tap/table.test.lua with the diff below: ================================================================================ diff --git a/test/app-tap/table.test.lua b/test/app-tap/table.test.lua index 60c095fdf..7279b07ed 100755 --- a/test/app-tap/table.test.lua +++ b/test/app-tap/table.test.lua @@ -145,7 +145,7 @@ end do -- check usage of __copy metamethod local copy_mt = nil; copy_mt = { - __copy = function(self) + __copy = function() local new_self = { a = 1} return setmetatable(new_self, copy_mt) end @@ -164,7 +164,7 @@ end do -- check usage of __copy metamethod + shallow local copy_mt = nil; copy_mt = { - __copy = function(self) + __copy = function() local new_self = { a = 1} return setmetatable(new_self, copy_mt) end @@ -191,7 +191,7 @@ end do -- check usage of not __copy metamethod on second level + shallow local copy_mt = nil; copy_mt = { - __copy = function(self) + __copy = function() local new_self = { a = 1 } return setmetatable(new_self, copy_mt) end > > > > -- > > 2.23.0 > > > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016066.html > > -- > Best regards, > IM -- sergeyb@