[Tarantool-patches] [PATCH v4 6/10] Fix luacheck warnings in test/
Sergey Bronnikov
sergeyb at tarantool.org
Wed May 6 19:16:27 MSK 2020
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 at tarantool.org wrote:
> > diff --git a/.luacheckrc b/.luacheckrc
<snipped>
> > 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"}}
>
> <snipped>
>
> > 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
>
> <snipped>
>
> > @@ -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')
>
> <snipped>
>
> > 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
>
> <snipped>
>
> > @@ -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 <testcase> 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 <testcase> 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
>
> <snipped>
>
> > 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
> <test> argument to <testcase>, but here you decided to change naming for
> the root suite. I guess you need either to adjust it to the common
> naming (<test> and <testcase>) 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
> <snipped>
>
> > 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
> <test> argument to <testcase>, but here you decided to change naming for
> the root suite. I guess you need either to adjust it to the common
> naming (<test> and <testcase>) 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 = {}
>
> <snipped>
>
> > @@ -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.
>
> <snipped>
>
> > 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
>
> <snipped>
>
> > @@ -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')
> >
>
> <snipped>
>
> > 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 <obj> variable in previous
> review[1]. Please consider it, since this warning will never be fixed.
applied
> <snipped>
>
> > 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
>
> <snipped>
>
> > @@ -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()
>
> <snipped>
>
> > 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
> <test> argument to <testcase>, but here you decided to change naming for
> the root suite. I guess you need either to adjust it to the common
> naming (<test> and <testcase>) 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)
>
> <snipped>
>
> > 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 <file> 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
> <snipped>
>
> > --
> > 2.23.0
> >
>
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016066.html
>
> --
> Best regards,
> IM
--
sergeyb@
More information about the Tarantool-patches
mailing list