Re: [Tarantool-patches] [PATCH v7] test: fix luacheck warnings in test/long_run-py

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 3 comments below.

On 13.01.2021 15:35, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Closes #5460
> 
> 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>
> ---
> 
> Changelog v7:
> 
> - updated an exclusion mask in .luacheckrc
> 
> Changelog v6:
> 
> - splitted patch in test/ for patches per sub-directory
> - adjusted supressions in .luacheckrc
> - fixed formatting issues in .luacheckrc
> 
> Gitlab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/241108315
> Issue: https://github.com/tarantool/tarantool/issues/5460
> Branch: ligurio/gh-5460-luacheck-warnings-test-long_run-py
> 
>  .luacheckrc                         |  2 +-
>  test/long_run-py/lua/finalizers.lua |  8 +++-----
>  test/long_run-py/suite.lua          | 16 +++++++++-------
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 4b829f3dc..68736d8db 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -37,7 +37,7 @@ exclude_files = {
>      "test/box-tap/**/*.lua",
>      "test/engine/**/*.lua",
>      "test/engine_long/**/*.lua",
> -    "test/long_run-py/**/*.lua",
> +    "test/long_run-py/lua/finalizers.lua",

1. Why did you leave it in 'exclude' and yet you fix it?

I tried to remove it, and found that it says your change
in the next hunk is not correct. When I fixed it, I saw it
says the 'result' variable is not used but only mutated.

Then I tried to 'read' result somehow after the loop, but
it says the code is unreachable. Then I made the loop look
like it can end, and now it seems to be working.

====================
diff --git a/.luacheckrc b/.luacheckrc
index 68736d8db..3f4913df3 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -37,7 +37,6 @@ exclude_files = {
     "test/box-tap/**/*.lua",
     "test/engine/**/*.lua",
     "test/engine_long/**/*.lua",
-    "test/long_run-py/lua/finalizers.lua",
     "test/luajit-tap/**/*.lua",
     "test/replication/**/*.lua",
     "test/replication-py/**/*.lua",
diff --git a/test/long_run-py/lua/finalizers.lua b/test/long_run-py/lua/finalizers.lua
index cb6400363..88bd7734c 100644
--- a/test/long_run-py/lua/finalizers.lua
+++ b/test/long_run-py/lua/finalizers.lua
@@ -7,10 +7,14 @@ local function test_finalizers()
     local result = {}
     local i = 1
     local ffi = require('ffi')
-    while true do
-        local result[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
+    while i ~= 0 do
+        result[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
         i = i + 1
     end
+    -- Fake-read 'result' to calm down 'luacheck' complaining that the variable
+    -- is never used.
+    assert(#result ~= 0)
+    return "done"
 end;
 
 test_finalizers()
====================

It looks a bit ugly maybe, because i ~= 0 is always true, and because
there is unreachable code after the loop.

I suggest to either find a better way to fix the warning; or don't change
finalizers.lua at all and keep the whole file ignored; or apply my diff,
or revert your 'local' usage in the next diff hunk, and silence this single
warning about 'result' being not used.

>      "test/luajit-tap/**/*.lua",
>      "test/replication/**/*.lua",
>      "test/replication-py/**/*.lua",
> diff --git a/test/long_run-py/lua/finalizers.lua b/test/long_run-py/lua/finalizers.lua
> index 69146a323..cb6400363 100644
> --- a/test/long_run-py/lua/finalizers.lua
> +++ b/test/long_run-py/lua/finalizers.lua
> @@ -1,19 +1,17 @@
>  #!/usr/bin/env tarantool
>  
> -function on_gc(t)
> +local function on_gc()
>  end;
>  
> -function test_finalizers()
> +local function test_finalizers()
>      local result = {}
>      local i = 1
>      local ffi = require('ffi')
>      while true do
> -        result[i] = ffi.gc(ffi.cast('void *', 0), on_gc)
> +        local result[i] = ffi.gc(ffi.cast('void *', 0), on_gc)

2. This change is not correct. Even luacheck tells it, if you don't
ignore this file. You assign a value to a table member, not
declare a variable.

>          i = i + 1
>      end
> -    return "done"
>  end;
>  
>  test_finalizers()
>  test_finalizers()
> -
> diff --git a/test/long_run-py/suite.lua b/test/long_run-py/suite.lua
> index 0b33dec7d..7a09dd2b8 100644
> --- a/test/long_run-py/suite.lua
> +++ b/test/long_run-py/suite.lua
> @@ -109,3 +106,8 @@ function delete_insert(engine_name)
>      box.space.tester:drop()
>      return {counter, string_value_2}
>  end
> +
> +return {
> +    delete_replace_update = delete_replace_update;
> +    delete_insert = delete_insert;

3. Please, use ',' instead of ';'.

> +}
>