From: Igor Munkin <imun@tarantool.org>
To: sergeyb@tarantool.org
Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org,
v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v4 6/10] Fix luacheck warnings in test/
Date: Mon, 27 Apr 2020 17:38:28 +0300 [thread overview]
Message-ID: <20200427143827.GN11314@tarantool.org> (raw)
In-Reply-To: <b7acaf4b82b590eae11bdfe89da644c4e1595a3d.1587476678.git.sergeyb@tarantool.org>
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:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Closes #4681
>
> Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Reviewed-by: Igor Munkin <imun@tarantool.org>
>
> Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Co-authored-by: Igor Munkin <imun@tarantool.org>
> ---
> .luacheckrc | 34 ++++++
> test/app-tap/cfg.test.lua | 2 +-
> test/app-tap/clock.test.lua | 4 +-
> test/app-tap/console.test.lua | 15 +--
> test/app-tap/console_lua.test.lua | 22 ++--
> test/app-tap/csv.test.lua | 56 ++++-----
> test/app-tap/debug.test.lua | 10 +-
> test/app-tap/errno.test.lua | 6 +-
> test/app-tap/fail_main.test.lua | 6 +-
> .../gh-4761-json-per-call-options.test.lua | 11 +-
> test/app-tap/http_client.test.lua | 54 ++++-----
> test/app-tap/iconv.test.lua | 6 +-
> test/app-tap/init_script.test.lua | 14 +--
> test/app-tap/inspector.test.lua | 6 +-
> test/app-tap/json.test.lua | 1 -
> test/app-tap/logger.test.lua | 23 ++--
> test/app-tap/minimal.test.lua | 4 +-
> test/app-tap/module_api.test.lua | 11 +-
> test/app-tap/msgpackffi.test.lua | 3 +-
> test/app-tap/pcall.test.lua | 6 +-
> test/app-tap/snapshot.test.lua | 17 ++-
> test/app-tap/string.test.lua | 54 ++++-----
> test/app-tap/tap.test.lua | 26 ++--
> test/app-tap/tarantoolctl.test.lua | 76 ++++++------
> test/app-tap/trigger.test.lua | 20 ++--
> test/app-tap/yaml.test.lua | 16 +--
> test/box-py/box.lua | 2 +-
> test/box-tap/auth.test.lua | 36 +++---
> test/box-tap/cfg.test.lua | 34 +++---
> test/box-tap/cfgup.test.lua | 2 +-
> test/box-tap/feedback_daemon.test.lua | 4 +-
> test/box-tap/gc.test.lua | 6 +-
> test/box-tap/key_def.test.lua | 58 ++++-----
> test/box-tap/merger.test.lua | 112 +++++++++---------
> test/box-tap/on_schema_init.test.lua | 4 +-
> test/box-tap/schema_mt.test.lua | 18 +--
> test/box-tap/session.storage.test.lua | 10 +-
> test/box-tap/session.test.lua | 59 ++++-----
> test/box-tap/trigger_atexit.test.lua | 10 +-
> test/box-tap/trigger_yield.test.lua | 10 +-
> test/box/box.lua | 2 +-
> test/box/hash_multipart.result | 2 +-
> test/box/hash_multipart.test.lua | 2 +-
> test/box/lua/bitset.lua | 7 +-
> test/box/lua/cfg_bad_vinyl_dir.lua | 2 +-
> test/box/lua/cfg_rtree.lua | 2 +-
> test/box/lua/cfg_test1.lua | 2 +-
> test/box/lua/cfg_test2.lua | 2 +-
> test/box/lua/cfg_test3.lua | 2 +-
> test/box/lua/cfg_test4.lua | 2 +-
> test/box/lua/cfg_test5.lua | 4 +-
> test/box/lua/cfg_test6.lua | 2 +-
> test/box/lua/fifo.lua | 2 +-
> test/box/lua/identifier.lua | 9 +-
> test/box/lua/index_random_test.lua | 2 +-
> test/box/lua/require_init.lua | 3 -
> test/box/lua/test_init.lua | 10 +-
> test/box/lua/utils.lua | 13 +-
> test/box/on_schema_init.lua | 2 +-
> test/box/proxy.lua | 2 +-
> test/box/tiny.lua | 2 +-
> test/box/tree_pk.result | 4 +-
> test/box/tree_pk.test.lua | 4 +-
> test/engine/tree_min_max_count.result | 2 +-
> test/engine/tree_min_max_count.test.lua | 2 +-
> test/engine_long/suite.lua | 4 +-
> test/long_run-py/lua/finalizers.lua | 8 +-
> test/long_run-py/suite.lua | 6 +-
> test/replication-py/master.lua | 2 +-
> test/replication-py/panic.lua | 2 +-
> test/replication-py/replica.lua | 4 -
> test/replication/lua/fast_replica.lua | 3 +-
> test/replication/lua/rlimit.lua | 2 +-
> test/replication/master.lua | 2 +-
> test/replication/replicaset_ro_mostly.result | 2 +-
> .../replication/replicaset_ro_mostly.test.lua | 2 +-
> test/sql-tap/alter.test.lua | 4 +-
> test/sql-tap/analyze3.test.lua | 6 +-
> test/sql-tap/analyze5.test.lua | 2 +-
> test/sql-tap/analyze9.test.lua | 30 ++---
> test/sql-tap/between.test.lua | 4 +-
> test/sql-tap/date.test.lua | 3 +-
> test/sql-tap/delete1.test.lua | 2 +-
> test/sql-tap/e_delete.test.lua | 2 +-
> test/sql-tap/e_expr.test.lua | 22 ++--
> test/sql-tap/func.test.lua | 2 +-
> test/sql-tap/func3.test.lua | 24 ++--
> test/sql-tap/gh-2723-concurrency.test.lua | 8 +-
> .../gh-3083-ephemeral-unref-tuples.test.lua | 2 +-
> .../gh-3307-xfer-optimization-issue.test.lua | 16 +--
> .../gh-3332-tuple-format-leak.test.lua | 2 +-
> .../gh-4077-iproto-execute-no-bind.test.lua | 7 +-
> .../gh2127-indentifier-max-length.test.lua | 10 +-
> test/sql-tap/identifier-characters.test.lua | 2 +-
> test/sql-tap/index1.test.lua | 3 +-
> test/sql-tap/index7.test.lua | 2 +-
> test/sql-tap/join3.test.lua | 2 +-
> test/sql-tap/lua-tables.test.lua | 2 +-
> test/sql-tap/lua/sqltester.lua | 28 ++---
> test/sql-tap/misc1.test.lua | 10 +-
> test/sql-tap/misc5.test.lua | 2 +-
> test/sql-tap/select1.test.lua | 10 +-
> test/sql-tap/select2.test.lua | 8 +-
> test/sql-tap/select4.test.lua | 1 -
> test/sql-tap/select5.test.lua | 1 -
> test/sql-tap/select9.test.lua | 12 +-
> test/sql-tap/selectA.test.lua | 8 +-
> test/sql-tap/selectB.test.lua | 14 +--
> test/sql-tap/selectG.test.lua | 1 -
> test/sql-tap/sort.test.lua | 2 +-
> test/sql-tap/sql-errors.test.lua | 2 +-
> test/sql-tap/table.test.lua | 3 +-
> test/sql-tap/tkt-38cb5df375.test.lua | 1 -
> test/sql-tap/tkt-91e2e8ba6f.test.lua | 3 -
> test/sql-tap/tkt-9a8b09f8e6.test.lua | 3 -
> test/sql-tap/tkt-bd484a090c.test.lua | 3 +-
> test/sql-tap/tkt-fa7bf5ec.test.lua | 6 +-
> test/sql-tap/tkt2192.test.lua | 3 +-
> test/sql-tap/tkt3493.test.lua | 3 -
> test/sql-tap/trigger2.test.lua | 4 +-
> test/sql-tap/triggerA.test.lua | 1 -
> test/sql-tap/where2.test.lua | 9 +-
> test/sql-tap/where3.test.lua | 2 +-
> test/sql-tap/where4.test.lua | 4 +-
> test/sql-tap/where5.test.lua | 2 +-
> test/sql-tap/where6.test.lua | 2 +-
> test/sql-tap/where7.test.lua | 16 +--
> test/sql-tap/whereA.test.lua | 2 +-
> test/sql-tap/whereB.test.lua | 2 +-
> test/sql-tap/whereC.test.lua | 5 +-
> test/sql-tap/whereD.test.lua | 4 +-
> test/sql-tap/whereF.test.lua | 4 +-
> test/sql-tap/whereG.test.lua | 4 +-
> test/sql-tap/whereI.test.lua | 4 +-
> test/sql-tap/whereK.test.lua | 4 +-
> test/sql-tap/with1.test.lua | 14 +--
> test/sql-tap/with2.test.lua | 18 +--
> test/sql/lua/sql_tokenizer.lua | 2 +-
> test/sql/savepoints.result | 6 +-
> test/sql/savepoints.test.lua | 6 +-
> test/sql/triggers.result | 2 +-
> test/sql/triggers.test.lua | 2 +-
> test/vinyl/large.lua | 3 +-
> test/vinyl/txn_proxy.lua | 6 +-
> test/vinyl/upgrade/fill.lua | 8 +-
> test/vinyl/vinyl.lua | 2 +-
> test/wal_off/rtree_benchmark.result | 2 +-
> test/wal_off/rtree_benchmark.test.lua | 2 +-
> test/xlog-py/box.lua | 2 +-
> test/xlog/panic.lua | 2 +-
> test/xlog/reader.result | 2 +-
> test/xlog/reader.test.lua | 2 +-
> test/xlog/snap_io_rate.test.lua | 2 +-
> test/xlog/transaction.result | 2 +-
> test/xlog/transaction.test.lua | 2 +-
> .../2.1.3/gh-4771-upgrade-sequence/fill.lua | 12 +-
> test/xlog/xlog.lua | 2 +-
> 157 files changed, 704 insertions(+), 740 deletions(-)
>
> 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.
> +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.
> +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.
> +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')
================================================================================
> 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.
> + 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.
> + 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.
<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]).
> {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]).
> {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.
> 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?
> 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]).
> 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?
> 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.
<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]).
+ 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]).
> 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.
>
> -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.
I'm out from the further review for this version and will wait the next
one to proceed.
<snipped>
> --
> 2.23.0
>
[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016066.html
--
Best regards,
IM
next prev parent reply other threads:[~2020-04-27 14:45 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-21 14:00 [Tarantool-patches] [PATCH v4 0/10] Add static analysis with luacheck sergeyb
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 1/10] Add initial luacheck config sergeyb
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 2/10] gitlab-ci: enable static analysis with luacheck sergeyb
2020-04-21 20:04 ` Alexander Tikhonov
2020-04-22 8:09 ` Sergey Bronnikov
2020-04-22 8:11 ` Kirill Yukhin
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 3/10] Fix luacheck warnings in extra/dist/tarantoolctl.in sergeyb
2020-04-23 11:40 ` Igor Munkin
2020-04-24 8:02 ` Sergey Bronnikov
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 4/10] Fix luacheck warnings in src/lua/ sergeyb
2020-04-23 14:13 ` Igor Munkin
2020-04-24 9:12 ` Sergey Bronnikov
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 5/10] Fix luacheck warnings in src/box/lua/ sergeyb
2020-04-23 22:54 ` Igor Munkin
2020-05-07 10:32 ` Sergey Bronnikov
2020-05-07 14:34 ` Sergey Bronnikov
2020-05-07 10:52 ` Sergey Bronnikov
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 6/10] Fix luacheck warnings in test/ sergeyb
2020-04-27 14:38 ` Igor Munkin [this message]
2020-05-06 16:16 ` Sergey Bronnikov
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 7/10] schema: fix index promotion to functional index sergeyb
2020-04-23 23:24 ` Igor Munkin
2020-04-23 23:29 ` Igor Munkin
2020-04-28 23:19 ` Vladislav Shpilevoy
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 8/10] schema: fix internal symbols dangling in _G sergeyb
2020-04-21 14:13 ` Oleg Babin
2020-04-21 14:45 ` Sergey Bronnikov
2020-04-21 19:52 ` Igor Munkin
2020-04-23 23:27 ` Igor Munkin
2020-04-28 23:19 ` Vladislav Shpilevoy
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 9/10] Disabled test/luajit-tap in luacheckrc sergeyb
2020-04-24 10:16 ` Igor Munkin
2020-04-29 14:25 ` Sergey Bronnikov
2020-04-21 14:00 ` [Tarantool-patches] [PATCH v4 10/10] luajit: Fix warnings spotted by luacheck sergeyb
2020-04-21 19:33 ` Igor Munkin
2020-04-22 10:14 ` Sergey Bronnikov
2020-04-23 6:24 ` Sergey Bronnikov
2020-04-23 10:03 ` Igor Munkin
2020-04-23 10:30 ` Sergey Bronnikov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200427143827.GN11314@tarantool.org \
--to=imun@tarantool.org \
--cc=o.piskunov@tarantool.org \
--cc=sergeyb@tarantool.org \
--cc=tarantool-patches@dev.tarantool.org \
--cc=v.shpilevoy@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v4 6/10] Fix luacheck warnings in test/' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox