[Tarantool-patches] [PATCH 0/2] Fix new luacheck warnings in test/box-tap and test/box

[Tarantool-patches] [PATCH 0/2] Fix new luacheck warnings in test/box-tap and test/box

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Gitlab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/242130838
Follows up:
 - https://github.com/tarantool/tarantool/issues/5457
 - https://github.com/tarantool/tarantool/issues/5455

Sergey Bronnikov (2):
  luacheck: fix warnings in test/box
  luacheck: fix warnings in test/box-tap

 test/box-tap/feedback_daemon.test.lua        |  6 +++---
 test/box/gh-5304-insert_during_recovery.lua  | 10 +++++-----
 test/box/gh-5304-replace_during_recovery.lua |  1 +
 3 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.25.1

[Tarantool-patches] [PATCH 1/2] luacheck: fix warnings in test/box

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Follows up #5455
---
 test/box/gh-5304-insert_during_recovery.lua  | 10 +++++-----
 test/box/gh-5304-replace_during_recovery.lua |  1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/test/box/gh-5304-insert_during_recovery.lua b/test/box/gh-5304-insert_during_recovery.lua
index ac6eef342..c8b6c5cfb 100644
--- a/test/box/gh-5304-insert_during_recovery.lua
+++ b/test/box/gh-5304-insert_during_recovery.lua
@@ -1,24 +1,24 @@
 #!/usr/bin/env tarantool
 
-function none(old_space, new_space)
+local function none(old_space, new_space) -- luacheck: ignore
 end
 
-function trigger_replace(old_space, new_space)
+local function trigger_replace(old_space, new_space) -- luacheck: ignore
     box.space.temp:replace({1})
     box.space.loc:replace({1})
 end
 
-function trigger_insert(old_space, new_space)
+local function trigger_insert(old_space, new_space) -- luacheck: ignore
     box.space.temp:insert({1})
     box.space.loc:insert({1})
 end
 
-function trigger_upsert(old_space, new_space)
+local function trigger_upsert(old_space, new_space) -- luacheck: ignore
     box.space.temp:upsert({1}, {{'=', 1, 4}})
     box.space.loc:upsert({1}, {{'=', 1, 4}})
 end
 
-trigger = nil
+local trigger = nil
 
 if arg[1] == 'none' then
     trigger = none
diff --git a/test/box/gh-5304-replace_during_recovery.lua b/test/box/gh-5304-replace_during_recovery.lua
index d6a7099ac..8b9a657af 100644
--- a/test/box/gh-5304-replace_during_recovery.lua
+++ b/test/box/gh-5304-replace_during_recovery.lua
@@ -2,6 +2,7 @@
 
 if arg[1] == 'replace' then
     box.ctl.on_schema_init(function()
+        -- luacheck: ignore
         box.space._index:on_replace(function(old_space, new_space)
             if new_space[1] == 512 then
                 box.space.test:on_replace(function(old_tup, new_tup)
-- 
2.25.1

[Tarantool-patches] [PATCH 2/2] luacheck: fix warnings in test/box-tap

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Follows up #5457
---
 test/box-tap/feedback_daemon.test.lua | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/box-tap/feedback_daemon.test.lua b/test/box-tap/feedback_daemon.test.lua
index 123033cda..8cfbf31d7 100755
--- a/test/box-tap/feedback_daemon.test.lua
+++ b/test/box-tap/feedback_daemon.test.lua
@@ -124,8 +124,8 @@ local fh = fio.open("feedback.json")
 test:ok(fh, "file is created")
 local file_data = fh:read()
 -- Ignore the report time. The data should be equal other than that.
-feedback_save = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
-file_data = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
+feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
+file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
 test:is(file_data, feedback_save, "data is equal")
 fh:close()
 fio.unlink("feedback.json")
@@ -247,7 +247,7 @@ box.space.features_memtx_empty:drop()
 box.space.features_memtx:drop()
 box.space.features_sync:drop()
 
-function check_stats(stat)
+local function check_stats(stat)
     local sub = test:test('feedback operation stats')
     sub:plan(18)
     local box_stat = box.stat()
-- 
2.25.1

Re: [Tarantool-patches] [PATCH 1/2] luacheck: fix warnings in test/box

[Serge Petrenko via Tarantool-patches]

15.01.2021 14:22, sergeyb@tarantool.org пишет:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Follows up #5455
> ---

Thanks for the  patch!
LGTM.
One question below.


>   test/box/gh-5304-insert_during_recovery.lua  | 10 +++++-----
>   test/box/gh-5304-replace_during_recovery.lua |  1 +
>   2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/test/box/gh-5304-insert_during_recovery.lua b/test/box/gh-5304-insert_during_recovery.lua
> index ac6eef342..c8b6c5cfb 100644
> --- a/test/box/gh-5304-insert_during_recovery.lua
> +++ b/test/box/gh-5304-insert_during_recovery.lua
> @@ -1,24 +1,24 @@
>   #!/usr/bin/env tarantool
>   
> -function none(old_space, new_space)
> +local function none(old_space, new_space) -- luacheck: ignore

What's "luacheck: ignore" comment for? Haven't you fixed the warning 
already by introducing `local` ?

>   end
>   
> -function trigger_replace(old_space, new_space)
> +local function trigger_replace(old_space, new_space) -- luacheck: ignore
>       box.space.temp:replace({1})
>       box.space.loc:replace({1})
>   end
>   
> -function trigger_insert(old_space, new_space)
> +local function trigger_insert(old_space, new_space) -- luacheck: ignore
>       box.space.temp:insert({1})
>       box.space.loc:insert({1})
>   end
>   
> -function trigger_upsert(old_space, new_space)
> +local function trigger_upsert(old_space, new_space) -- luacheck: ignore
>       box.space.temp:upsert({1}, {{'=', 1, 4}})
>       box.space.loc:upsert({1}, {{'=', 1, 4}})
>   end
>   
> -trigger = nil
> +local trigger = nil
>   
>   if arg[1] == 'none' then
>       trigger = none
> diff --git a/test/box/gh-5304-replace_during_recovery.lua b/test/box/gh-5304-replace_during_recovery.lua
> index d6a7099ac..8b9a657af 100644
> --- a/test/box/gh-5304-replace_during_recovery.lua
> +++ b/test/box/gh-5304-replace_during_recovery.lua
> @@ -2,6 +2,7 @@
>   
>   if arg[1] == 'replace' then
>       box.ctl.on_schema_init(function()
> +        -- luacheck: ignore
>           box.space._index:on_replace(function(old_space, new_space)
>               if new_space[1] == 512 then
>                   box.space.test:on_replace(function(old_tup, new_tup)

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 2/2] luacheck: fix warnings in test/box-tap

[Serge Petrenko via Tarantool-patches]

15.01.2021 14:22, sergeyb@tarantool.org пишет:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Follows up #5457
> ---
>   test/box-tap/feedback_daemon.test.lua | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/test/box-tap/feedback_daemon.test.lua b/test/box-tap/feedback_daemon.test.lua
> index 123033cda..8cfbf31d7 100755
> --- a/test/box-tap/feedback_daemon.test.lua
> +++ b/test/box-tap/feedback_daemon.test.lua
> @@ -124,8 +124,8 @@ local fh = fio.open("feedback.json")
>   test:ok(fh, "file is created")
>   local file_data = fh:read()
>   -- Ignore the report time. The data should be equal other than that.
> -feedback_save = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
> -file_data = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
> +feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
> +file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
Thanks for finding this typo!
Please also replace `time:0` with `"time":0`

Other than that LGTM.

>   test:is(file_data, feedback_save, "data is equal")
>   fh:close()
>   fio.unlink("feedback.json")
> @@ -247,7 +247,7 @@ box.space.features_memtx_empty:drop()
>   box.space.features_memtx:drop()
>   box.space.features_sync:drop()
>   
> -function check_stats(stat)
> +local function check_stats(stat)
>       local sub = test:test('feedback operation stats')
>       sub:plan(18)
>       local box_stat = box.stat()

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 1/2] luacheck: fix warnings in test/box

[Sergey Bronnikov via Tarantool-patches]

Thanks for review!

On 15.01.2021 15:39, Serge Petrenko wrote:
>
>
> 15.01.2021 14:22, sergeyb@tarantool.org пишет:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Follows up #5455
>> ---
>
> Thanks for the  patch!
> LGTM.
> One question below.
>
>
>> test/box/gh-5304-insert_during_recovery.lua  | 10 +++++-----
>>   test/box/gh-5304-replace_during_recovery.lua |  1 +
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/test/box/gh-5304-insert_during_recovery.lua 
>> b/test/box/gh-5304-insert_during_recovery.lua
>> index ac6eef342..c8b6c5cfb 100644
>> --- a/test/box/gh-5304-insert_during_recovery.lua
>> +++ b/test/box/gh-5304-insert_during_recovery.lua
>> @@ -1,24 +1,24 @@
>>   #!/usr/bin/env tarantool
>>   -function none(old_space, new_space)
>> +local function none(old_space, new_space) -- luacheck: ignore
>
> What's "luacheck: ignore" comment for? Haven't you fixed the warning 
> already by introducing `local` ?
>
There are two warnings here. First one about global scope, that was 
fixed with "local" and second one

about unused parameters, that was suppressed by "luacheck: ignore".

>>   end
>>   -function trigger_replace(old_space, new_space)
>> +local function trigger_replace(old_space, new_space) -- luacheck: 
>> ignore
>>       box.space.temp:replace({1})
>>       box.space.loc:replace({1})
>>   end
>>   -function trigger_insert(old_space, new_space)
>> +local function trigger_insert(old_space, new_space) -- luacheck: ignore
>>       box.space.temp:insert({1})
>>       box.space.loc:insert({1})
>>   end
>>   -function trigger_upsert(old_space, new_space)
>> +local function trigger_upsert(old_space, new_space) -- luacheck: ignore
>>       box.space.temp:upsert({1}, {{'=', 1, 4}})
>>       box.space.loc:upsert({1}, {{'=', 1, 4}})
>>   end
>>   -trigger = nil
>> +local trigger = nil
>>     if arg[1] == 'none' then
>>       trigger = none
>> diff --git a/test/box/gh-5304-replace_during_recovery.lua 
>> b/test/box/gh-5304-replace_during_recovery.lua
>> index d6a7099ac..8b9a657af 100644
>> --- a/test/box/gh-5304-replace_during_recovery.lua
>> +++ b/test/box/gh-5304-replace_during_recovery.lua
>> @@ -2,6 +2,7 @@
>>     if arg[1] == 'replace' then
>>       box.ctl.on_schema_init(function()
>> +        -- luacheck: ignore
>>           box.space._index:on_replace(function(old_space, new_space)
>>               if new_space[1] == 512 then
>>                   box.space.test:on_replace(function(old_tup, new_tup)
>

Re: [Tarantool-patches] [PATCH 2/2] luacheck: fix warnings in test/box-tap

[Sergey Bronnikov via Tarantool-patches]

Thanks for review!

On 15.01.2021 15:40, Serge Petrenko wrote:
>
>
> 15.01.2021 14:22, sergeyb@tarantool.org пишет:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Follows up #5457
>> ---
>>   test/box-tap/feedback_daemon.test.lua | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/test/box-tap/feedback_daemon.test.lua 
>> b/test/box-tap/feedback_daemon.test.lua
>> index 123033cda..8cfbf31d7 100755
>> --- a/test/box-tap/feedback_daemon.test.lua
>> +++ b/test/box-tap/feedback_daemon.test.lua
>> @@ -124,8 +124,8 @@ local fh = fio.open("feedback.json")
>>   test:ok(fh, "file is created")
>>   local file_data = fh:read()
>>   -- Ignore the report time. The data should be equal other than that.
>> -feedback_save = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
>> -file_data = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
>> +feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
>> +file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
> Thanks for finding this typo!
> Please also replace `time:0` with `"time":0`

Sure.

@@ -124,8 +124,8 @@ local fh = fio.open("feedback.json")
  test:ok(fh, "file is created")
  local file_data = fh:read()
  -- Ignore the report time. The data should be equal other than that.
-feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
-file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
+feedback_save = string.gsub(feedback_save, '"time":(%d+)', '"time":0')
+file_data = string.gsub(file_data, '"time":(%d+)', '"time":0')
  test:is(file_data, feedback_save, "data is equal")
  fh:close()
  fio.unlink("feedback.json")

>
> Other than that LGTM.
>
>>   test:is(file_data, feedback_save, "data is equal")
>>   fh:close()
>>   fio.unlink("feedback.json")
>> @@ -247,7 +247,7 @@ box.space.features_memtx_empty:drop()
>>   box.space.features_memtx:drop()
>>   box.space.features_sync:drop()
>>   -function check_stats(stat)
>> +local function check_stats(stat)
>>       local sub = test:test('feedback operation stats')
>>       sub:plan(18)
>>       local box_stat = box.stat()
>

Re: [Tarantool-patches] [PATCH 1/2] luacheck: fix warnings in test/box

[Serge Petrenko via Tarantool-patches]

15.01.2021 15:54, Sergey Bronnikov пишет:
> Thanks for review!
>
> On 15.01.2021 15:39, Serge Petrenko wrote:
>>
>>
>> 15.01.2021 14:22, sergeyb@tarantool.org пишет:
>>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>>
>>> Follows up #5455
>>> ---
>>
>> Thanks for the  patch!
>> LGTM.
>> One question below.
>>
>>
>>> test/box/gh-5304-insert_during_recovery.lua | 10 +++++-----
>>>   test/box/gh-5304-replace_during_recovery.lua |  1 +
>>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/test/box/gh-5304-insert_during_recovery.lua 
>>> b/test/box/gh-5304-insert_during_recovery.lua
>>> index ac6eef342..c8b6c5cfb 100644
>>> --- a/test/box/gh-5304-insert_during_recovery.lua
>>> +++ b/test/box/gh-5304-insert_during_recovery.lua
>>> @@ -1,24 +1,24 @@
>>>   #!/usr/bin/env tarantool
>>>   -function none(old_space, new_space)
>>> +local function none(old_space, new_space) -- luacheck: ignore
>>
>> What's "luacheck: ignore" comment for? Haven't you fixed the warning 
>> already by introducing `local` ?
>>
> There are two warnings here. First one about global scope, that was 
> fixed with "local" and second one
>
> about unused parameters, that was suppressed by "luacheck: ignore".

Ok, I see, thanks!

>
>>>   end
>>>   -function trigger_replace(old_space, new_space)
>>> +local function trigger_replace(old_space, new_space) -- luacheck: 
>>> ignore
>>>       box.space.temp:replace({1})
>>>       box.space.loc:replace({1})
>>>   end
>>>   -function trigger_insert(old_space, new_space)
>>> +local function trigger_insert(old_space, new_space) -- luacheck: 
>>> ignore
>>>       box.space.temp:insert({1})
>>>       box.space.loc:insert({1})
>>>   end
>>>   -function trigger_upsert(old_space, new_space)
>>> +local function trigger_upsert(old_space, new_space) -- luacheck: 
>>> ignore
>>>       box.space.temp:upsert({1}, {{'=', 1, 4}})
>>>       box.space.loc:upsert({1}, {{'=', 1, 4}})
>>>   end
>>>   -trigger = nil
>>> +local trigger = nil
>>>     if arg[1] == 'none' then
>>>       trigger = none
>>> diff --git a/test/box/gh-5304-replace_during_recovery.lua 
>>> b/test/box/gh-5304-replace_during_recovery.lua
>>> index d6a7099ac..8b9a657af 100644
>>> --- a/test/box/gh-5304-replace_during_recovery.lua
>>> +++ b/test/box/gh-5304-replace_during_recovery.lua
>>> @@ -2,6 +2,7 @@
>>>     if arg[1] == 'replace' then
>>>       box.ctl.on_schema_init(function()
>>> +        -- luacheck: ignore
>>>           box.space._index:on_replace(function(old_space, new_space)
>>>               if new_space[1] == 512 then
>>> box.space.test:on_replace(function(old_tup, new_tup)
>>

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 2/2] luacheck: fix warnings in test/box-tap

[Serge Petrenko via Tarantool-patches]

15.01.2021 15:56, Sergey Bronnikov пишет:
> Thanks for review!
>
> On 15.01.2021 15:40, Serge Petrenko wrote:
>>
>>
>> 15.01.2021 14:22, sergeyb@tarantool.org пишет:
>>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>>
>>> Follows up #5457
>>> ---
>>>   test/box-tap/feedback_daemon.test.lua | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/test/box-tap/feedback_daemon.test.lua 
>>> b/test/box-tap/feedback_daemon.test.lua
>>> index 123033cda..8cfbf31d7 100755
>>> --- a/test/box-tap/feedback_daemon.test.lua
>>> +++ b/test/box-tap/feedback_daemon.test.lua
>>> @@ -124,8 +124,8 @@ local fh = fio.open("feedback.json")
>>>   test:ok(fh, "file is created")
>>>   local file_data = fh:read()
>>>   -- Ignore the report time. The data should be equal other than that.
>>> -feedback_save = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
>>> -file_data = string.gsub(feedback_save, 'time:(%d+)', 'time:0')
>>> +feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
>>> +file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
>> Thanks for finding this typo!
>> Please also replace `time:0` with `"time":0`
>
> Sure.
>
> @@ -124,8 +124,8 @@ local fh = fio.open("feedback.json")
>  test:ok(fh, "file is created")
>  local file_data = fh:read()
>  -- Ignore the report time. The data should be equal other than that.
> -feedback_save = string.gsub(feedback_save, '"time":(%d+)', 'time:0')
> -file_data = string.gsub(file_data, '"time":(%d+)', 'time:0')
> +feedback_save = string.gsub(feedback_save, '"time":(%d+)', '"time":0')
> +file_data = string.gsub(file_data, '"time":(%d+)', '"time":0')
>  test:is(file_data, feedback_save, "data is equal")
>  fh:close()
>  fio.unlink("feedback.json")

Thanks, LGTM.

>
>>
>> Other than that LGTM.
>>
>>>   test:is(file_data, feedback_save, "data is equal")
>>>   fh:close()
>>>   fio.unlink("feedback.json")
>>> @@ -247,7 +247,7 @@ box.space.features_memtx_empty:drop()
>>>   box.space.features_memtx:drop()
>>>   box.space.features_sync:drop()
>>>   -function check_stats(stat)
>>> +local function check_stats(stat)
>>>       local sub = test:test('feedback operation stats')
>>>       sub:plan(18)
>>>       local box_stat = box.stat()
>>

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH 0/2] Fix new luacheck warnings in test/box-tap and test/box

[Kirill Yukhin via Tarantool-patches]

Hello,

On 15 янв 14:22, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Gitlab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/242130838
> Follows up:
>  - https://github.com/tarantool/tarantool/issues/5457
>  - https://github.com/tarantool/tarantool/issues/5455
> 
> Sergey Bronnikov (2):
>   luacheck: fix warnings in test/box
>   luacheck: fix warnings in test/box-tap


I've checked your patchset into 2.7 and master.
Also cherry-picked second patch to 2.6

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH 1/2] luacheck: fix warnings in test/box

[Igor Munkin via Tarantool-patches]

Sergey,

Even I'm a bit late (Kirill is the fastest hand in the wild west^W^W
Tarantool), I have a single nit regarding Serge question.

On 15.01.21, Sergey Bronnikov via Tarantool-patches wrote:
> Thanks for review!
> 
> On 15.01.2021 15:39, Serge Petrenko wrote:
> >

<snipped>

> >> diff --git a/test/box/gh-5304-insert_during_recovery.lua 
> >> b/test/box/gh-5304-insert_during_recovery.lua
> >> index ac6eef342..c8b6c5cfb 100644
> >> --- a/test/box/gh-5304-insert_during_recovery.lua
> >> +++ b/test/box/gh-5304-insert_during_recovery.lua
> >> @@ -1,24 +1,24 @@
> >>   #!/usr/bin/env tarantool
> >>   -function none(old_space, new_space)
> >> +local function none(old_space, new_space) -- luacheck: ignore
> >
> > What's "luacheck: ignore" comment for? Haven't you fixed the warning 
> > already by introducing `local` ?
> >
> There are two warnings here. First one about global scope, that was 
> fixed with "local" and second one
> 
> about unused parameters, that was suppressed by "luacheck: ignore".

AFAICS there is a specific ignore reason for inline suppressions such as
"no unused args" that is used in other Lua sources repo-wide (just grep for
it). You can also see other available inline reasons here[1].

> 
> >>   end

<snipped>

> >

[1]: https://luacheck.readthedocs.io/en/stable/inline.html

-- 
Best regards,
IM