[Tarantool-patches] [PATCH v6 09/25] Fix luacheck warnings in test/box
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jul 16 16:23:45 MSK 2020
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 at tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb at tarantool.org>
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Reviewed-by: Igor Munkin <imun at tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Co-authored-by: Igor Munkin <imun at 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
More information about the Tarantool-patches
mailing list