Re: [Tarantool-patches] [PATCH v6 09/25] Fix luacheck warnings in test/box

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks for the patch!

Again, I see a mess in the branch. Please review yourself before sending
the patchset: mixed tabs and spaces, unsorted enries in .luacheckrc --
we are talking about it since I joined the review (i.e. v3 series). Now
it's a v6 and I definitely don't want another one, but I doubt we will
avoid it. Furthermore the changes you made in scope of this patch just
partially fix the problem. Yes, I see you committed/rebased some
follow-up patches on the branch few hours ago and it's another
consequence of omitting self-review. Hasty fixes are not necessary (I
have another patchesets to be reviewed and my own tasks), but pointing
to the same mistakes just makes me frustrating. It would be great if you
re-check yourself once more before running git send-email next time.
This will definitely make the review process much easier, efficient and
ergo faster.

Please send follow-up patches to this thread, since it's too hard to
comment the changes you made after the series was sent.

I reviewed this one and its follow-up you recently pushed. Please
consider the comments below.

On 29.05.20, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Part of #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                        | 13 +++++++++++++
>  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 ++--
>  24 files changed, 51 insertions(+), 46 deletions(-)
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 630834e16..f99cc17eb 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -41,3 +41,16 @@ files["src/box/lua/schema.lua"] = {globals = {"tonumber64"}, ignore = {"431", "4
>  files["test/app/lua/fiber.lua"] = {globals = {"box_fiber_run_test"}}
>  files["test/app-tap/lua/require_mod.lua"] = {globals = {"exports"}}
>  files["test/app-tap/string.test.lua"] = {globals = {"utf8"}}
> +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"}

One more time: please sort the entries. It *really* helps for reviewing
and further maintenance. I don't want to refer the last time I was
asking it, please just fix.

One more time: please adjust the indentation (tabs vs spaces) and to use
only one as we already discussed several times. I don't want to refer
the last time I was asking it, please just fix.

Furthermore, I can't figure out the rules you're using for indentation.
.luacheckrc is kinda Lua source file, so please adjust it regarding our
contrubition guide. The result should be similar to the example below:
| files["test/box/lua/bitset.lua"] = {
|     globals = {
|         "create_space", "fill", "delete", "clear", "drop_space",
|         "dump", "test_insert_delete"
|     }
| }

Feel free to put each global entry to a seraparate line if it seems
better for you.

> +}
> +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"}}

<snipped>

I see no rule for checking .result files, *but* I know the exact reason,
why I see such changes in the patch. The mask ought to be updated *prior
to or together with* the changes. Please remove this change, since
test/box/hash_miltipart.test.lua is a diff-based test, thereby has to be
skipped by luacheck as we discussed several times.

> diff --git a/test/box/hash_multipart.result b/test/box/hash_multipart.result
> index e94313b62..e6915652f 100644
> --- a/test/box/hash_multipart.result
> +++ b/test/box/hash_multipart.result
> @@ -62,7 +62,7 @@ test_run:cmd("setopt delimiter ';'")
>  function select_all()
>      local result = {}
>      local tuple, v
> -    for tuple, v in hash:pairs() do
> +    for _, v in hash:pairs() do
>          table.insert(result, v)
>      end
>      return result
> diff --git a/test/box/hash_multipart.test.lua b/test/box/hash_multipart.test.lua
> index c0a871bee..c572d13b6 100644
> --- a/test/box/hash_multipart.test.lua
> +++ b/test/box/hash_multipart.test.lua
> @@ -24,7 +24,7 @@ test_run:cmd("setopt delimiter ';'")
>  function select_all()
>      local result = {}
>      local tuple, v
> -    for tuple, v in hash:pairs() do
> +    for _, v in hash:pairs() do
>          table.insert(result, v)
>      end
>      return result

<snipped>

I failed to find any source that uses require_init.lua, require_mod.lua
(except require_init.lua) and test_init.lua. Looks these chunk can be
just dropped. But I'm totally against such actions in scope of this
patch, so please revert the changes related to these files, add them to
the exclude list like you did for test_init.lua in the follow-up patch
and create an issue for further actions.

> diff --git a/test/box/lua/require_init.lua b/test/box/lua/require_init.lua

<snipped>

> diff --git a/test/box/lua/test_init.lua b/test/box/lua/test_init.lua

<snipped>

> diff --git a/test/box/on_schema_init.lua b/test/box/on_schema_init.lua
> index 17cf89166..f74d6d7fe 100644
> --- a/test/box/on_schema_init.lua
> +++ b/test/box/on_schema_init.lua

Here is the diff from the follow-up patch:

| diff --git a/test/box/on_schema_init.lua b/test/box/on_schema_init.lua
| index f74d6d7fe..a32470b40 100644
| --- a/test/box/on_schema_init.lua
| +++ b/test/box/on_schema_init.lua
| @@ -1,12 +1,12 @@
|  #!/usr/bin/env tarantool
|  local os = require('os')
|  
| -function test_before_replace_trig(old, new)
| +local function test_before_replace_trig(old, new) -- luacheck: ignore

I see no reason to use "ignore" here and below instead of "no unused
args" that explicitly describes the suppressed warning.

|      -- return multiple values so that the stack fills earlier.
|      return new:update{{'+', 2, 1}}, new:update{{'+', 2, 1}}, new:update{{'+', 2, 1}}, new:update{{'+', 2, 1}}
|  end
|  
| -function space_on_replace_trig(old, new)
| +local function space_on_replace_trig(old, new) -- luacheck: ignore
|      if new and new[3] == 'test_on_schema_init' then
|          box.on_commit(function()
|              box.space.test_on_schema_init:before_replace(test_before_replace_trig)
| @@ -14,7 +14,7 @@ function space_on_replace_trig(old, new)
|      end
|  end
|  
| -function on_init_trig()
| +local function on_init_trig()
|      box.space._space:on_replace(space_on_replace_trig)
|  end
|  

<snipped>

Again, the changeset below is made for diff-based tests, please drop it.

> diff --git a/test/box/tree_pk.result b/test/box/tree_pk.result
> index df3c78bed..f2a4f5352 100644
> --- a/test/box/tree_pk.result
> +++ b/test/box/tree_pk.result
> @@ -153,8 +153,8 @@ test_run:cmd("setopt delimiter ';'")
>  ...
>  function crossjoin(space0, space1, limit)
>      local result = {}
> -    for state, v0 in space0:pairs() do
> -        for state, v1 in space1:pairs() do
> +    for _, v0 in space0:pairs() do
> +        for _, v1 in space1:pairs() do
>              if limit <= 0 then
>                  return result
>              end
> diff --git a/test/box/tree_pk.test.lua b/test/box/tree_pk.test.lua
> index 1190ab424..86041a642 100644
> --- a/test/box/tree_pk.test.lua
> +++ b/test/box/tree_pk.test.lua
> @@ -58,8 +58,8 @@ test_run = env.new()
>  test_run:cmd("setopt delimiter ';'")
>  function crossjoin(space0, space1, limit)
>      local result = {}
> -    for state, v0 in space0:pairs() do
> -        for state, v1 in space1:pairs() do
> +    for _, v0 in space0:pairs() do
> +        for _, v1 in space1:pairs() do
>              if limit <= 0 then
>                  return result
>              end
> -- 
> 2.23.0
> 

-- 
Best regards,
IM