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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Igor,

On 19:06 Mon 01 Jun , Igor Munkin wrote:
> 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.

I have splitted lines per variable and sorted alphabetically.
Update is in a branch and a new patch series.

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

Fixed too.

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

As said above I have put each entry to a separate line.

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

reviewed overall patch series and removed all changes for diff-based tests

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

Well, marked them to be dropped:

     "test/app/*.test.lua",
-    "test/box/**/*.lua",
+    "test/box/*.test.lua",
+    -- Unused source file, to be dropped (gh-5169).
+    "test/box/lua/require_init.lua",
+    -- Unused source file, to be dropped (gh-5169).
+    "test/box/lua/require_mod.lua",
+    -- Unused source file, to be dropped (gh-5169).
+    "test/box/lua/test_init.lua",
     "test/box-py/**/*.lua",

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

I have set exact ignore reason in a luacheck comment

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

Done.

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