Re: [Tarantool-patches] [PATCH v4 6/10] Fix luacheck warnings in test/

[Sergey Bronnikov <sergeyb@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

<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@