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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for review!

On 13.01.2021 20:04, Vladislav Shpilevoy wrote:
> 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?

Change in a file was done to fix luacheck warning, but I didn't know how 
to fix

other warnings so decided to exclude it at all.

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

Agree, it's looks a bit ugly, although I think it is better than keep 
suppression in .luacheckrc for the whole file.

So I applied your change and squashed to commit. Thanks.

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

It is still not unclear for me why luacheck complains here.

result table declared before a loop and scope for it is a whole function 
body, what's wrong?

Similar lua chunk successfully executed by puc lua:

local result = {}
local i = 0
while i == 0 do
     result[i] = 1
end

>
>>           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 ';'.
>
Fixed in a branch.

--- a/test/long_run-py/suite.lua
+++ b/test/long_run-py/suite.lua
@@ -108,6 +108,6 @@ local function delete_insert(engine_name)
  end

  return {
-    delete_replace_update = delete_replace_update;
-    delete_insert = delete_insert;
+    delete_replace_update = delete_replace_update,
+    delete_insert = delete_insert,
  }

>> +}
>>