[Tarantool-patches] [PATCH v7 0/2] Fix luacheck warnings in test/engine and test/engine_long

[Tarantool-patches] [PATCH v7 0/2] Fix luacheck warnings in test/engine and test/engine_long

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Changelog v7:

- rebased to a master branch (and includes fixes for test/box and test/box-tap to pass ci [1])
- fixed new warnings

Changelog v6:

- splitted patch in test/ for patches per sub-directory
- adjusted supressions in .luacheckrc
- fixed formatting issues in .luacheckrc

1. https://lists.tarantool.org/tarantool-patches/cover.1610709207.git.sergeyb@tarantool.org/T/#t

Gitlab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/242152198
GH branch: ligurio/gh-5458-luacheck-warnings-test-engine
GH issues:
  - https://github.com/tarantool/tarantool/issues/5458
  - https://github.com/tarantool/tarantool/issues/5650

Sergey Bronnikov (2):
  luacheck: fix warnings in test/engine
  luacheck: fix warnings in test/engine_long

 .luacheckrc                   | 10 ++++++++--
 test/engine/box.lua           |  8 +-------
 test/engine/conflict.lua      | 12 ++++++++----
 test/engine/conflict.test.lua |  4 ++--
 test/engine_long/suite.lua    |  5 +----
 5 files changed, 20 insertions(+), 19 deletions(-)

-- 
2.25.1

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

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Closes #5458

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                   |  2 +-
 test/engine/box.lua           |  8 +-------
 test/engine/conflict.lua      | 12 ++++++++----
 test/engine/conflict.test.lua |  4 ++--
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/.luacheckrc b/.luacheckrc
index b7f9abb45..d58fd57b0 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -39,7 +39,7 @@ exclude_files = {
     "test/box/lua/require_mod.lua",
     -- Unused source file, to be dropped (gh-5169).
     "test/box/lua/test_init.lua",
-    "test/engine/**/*.lua",
+    "test/engine/*.test.lua",
     "test/engine_long/**/*.lua",
     "test/long_run-py/**/*.lua",
     "test/luajit-tap/**/*.lua",
diff --git a/test/engine/box.lua b/test/engine/box.lua
index e2f04cba2..8558c9ac0 100644
--- a/test/engine/box.lua
+++ b/test/engine/box.lua
@@ -1,11 +1,5 @@
 #!/usr/bin/env tarantool
-os = require('os')
-
-local vinyl = {
-    threads = 3,
-    range_size=1024*64,
-    page_size=1024,
-}
+local os = require('os')
 
 box.cfg{
     listen              = os.getenv("LISTEN"),
diff --git a/test/engine/conflict.lua b/test/engine/conflict.lua
index b757b81d2..1f9fe5a45 100644
--- a/test/engine/conflict.lua
+++ b/test/engine/conflict.lua
@@ -1,11 +1,11 @@
 
-function test_conflict()
+local function test_conflict()
     local test_run = require('test_run')
     local inspector = test_run.new()
     local engine = inspector:get_cfg('engine')
 
     local s = box.schema.space.create('tester', {engine=engine});
-    local i = s:create_index('test_index', {type = 'tree', parts = {1, 'string'}});
+    s:create_index('test_index', {type = 'tree', parts = {1, 'string'}});
 
     local commits = 0
     local function conflict()
@@ -16,10 +16,14 @@ function test_conflict()
     end;
 
     local fiber = require('fiber');
-    local f0 = fiber.create(conflict);
-    local f1 = fiber.create(conflict); -- conflict
+    fiber.create(conflict);
+    fiber.create(conflict); -- conflict
     fiber.sleep(0);
 
     s:drop();
     return commits
 end
+
+return {
+    test_conflict = test_conflict;
+}
diff --git a/test/engine/conflict.test.lua b/test/engine/conflict.test.lua
index d83c03a7a..7744b41b4 100644
--- a/test/engine/conflict.test.lua
+++ b/test/engine/conflict.test.lua
@@ -1,4 +1,4 @@
 
-dofile('conflict.lua')
+conflict = require('conflict')
 
-test_conflict()
+conflict.test_conflict()
-- 
2.25.1

[Tarantool-patches] [PATCH v7 2/2] luacheck: fix warnings in test/engine_long

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Closes #5650

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                | 8 +++++++-
 test/engine_long/suite.lua | 5 +----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/.luacheckrc b/.luacheckrc
index d58fd57b0..b581136b2 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -40,7 +40,7 @@ exclude_files = {
     -- Unused source file, to be dropped (gh-5169).
     "test/box/lua/test_init.lua",
     "test/engine/*.test.lua",
-    "test/engine_long/**/*.lua",
+    "test/engine_long/*.test.lua",
     "test/long_run-py/**/*.lua",
     "test/luajit-tap/**/*.lua",
     "test/replication/**/*.lua",
@@ -176,3 +176,9 @@ files["test/box-tap/extended_error.test.lua"] = {
         "forbidden_function",
     },
 }
+files["test/engine_long/suite.lua"] = {
+    globals = {
+        "delete_replace_update",
+        "delete_insert",
+    }
+}
diff --git a/test/engine_long/suite.lua b/test/engine_long/suite.lua
index 9ac2bff9f..586160a1a 100644
--- a/test/engine_long/suite.lua
+++ b/test/engine_long/suite.lua
@@ -2,7 +2,7 @@ local function string_function()
     local random_number
     local random_string
     random_string = ""
-    for x = 1,20,1 do
+    for _ = 1,20,1 do
         random_number = math.random(65, 90)
         random_string = random_string .. string.char(random_number)
     end
@@ -10,7 +10,6 @@ local function string_function()
 end
 
 function delete_replace_update(engine_name, iterations)
-    local string_value
     if (box.space._space.index.name:select{'tester'}[1] ~= nil) then
         box.space.tester:drop()
     end
@@ -40,7 +39,6 @@ function delete_replace_update(engine_name, iterations)
         random_number = math.random(1,6)
 
         string_value_3 = string_function()
---      print('<'..counter..'> [' ..  random_number .. '] value_2: ' .. string_value_2 .. ' value_3: ' .. string_value_3)
         if random_number == 1 then
             box.space.tester:delete{string_value_2}
         end
@@ -69,7 +67,6 @@ function delete_replace_update(engine_name, iterations)
 end
 
 function delete_insert(engine_name, iterations)
-    local string_value
     if (box.space._space.index.name:select{'tester'}[1] ~= nil) then
         box.space.tester:drop()
     end
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v7 2/2] luacheck: fix warnings in test/engine_long

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

> diff --git a/.luacheckrc b/.luacheckrc
> index d58fd57b0..b581136b2 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -176,3 +176,9 @@ files["test/box-tap/extended_error.test.lua"] = {
>          "forbidden_function",
>      },
>  }
> +files["test/engine_long/suite.lua"] = {
> +    globals = {
> +        "delete_replace_update",
> +        "delete_insert",

There is no reason for these functions to be global. The only reason
is being able to call a function via IPROTO_CALL, which is not the
case here.

I proper fix should make them local and returned as a result of
require():

====================
diff --git a/.luacheckrc b/.luacheckrc
index 02349331e..8c67f9d47 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -131,9 +131,3 @@ files["test/box-tap/extended_error.test.lua"] = {
         "forbidden_function",
     },
 }
-files["test/engine_long/suite.lua"] = {
-    globals = {
-        "delete_replace_update",
-        "delete_insert",
-    }
-}
diff --git a/test/engine_long/box.lua b/test/engine_long/box.lua
index 28a1560d5..94ae81301 100644
--- a/test/engine_long/box.lua
+++ b/test/engine_long/box.lua
@@ -1,7 +1,5 @@
 #!/usr/bin/env tarantool
 
-require('suite')
-
 os.execute("rm -rf vinyl_test")
 os.execute("mkdir -p vinyl_test")
 
diff --git a/test/engine_long/delete_insert.result b/test/engine_long/delete_insert.result
index b1d504271..06085ca78 100644
--- a/test/engine_long/delete_insert.result
+++ b/test/engine_long/delete_insert.result
@@ -1,6 +1,9 @@
 test_run = require('test_run')
 ---
 ...
+delete_insert = require('suite').delete_insert
+---
+...
 inspector = test_run.new()
 ---
 ...
diff --git a/test/engine_long/delete_insert.test.lua b/test/engine_long/delete_insert.test.lua
index 275aaa23e..b2c4ec308 100644
--- a/test/engine_long/delete_insert.test.lua
+++ b/test/engine_long/delete_insert.test.lua
@@ -1,4 +1,5 @@
 test_run = require('test_run')
+delete_insert = require('suite').delete_insert
 inspector = test_run.new()
 engine = inspector:get_cfg('engine')
 iterations = 100000
diff --git a/test/engine_long/delete_replace_update.result b/test/engine_long/delete_replace_update.result
index 66cb9c82c..b12e00688 100644
--- a/test/engine_long/delete_replace_update.result
+++ b/test/engine_long/delete_replace_update.result
@@ -1,3 +1,6 @@
+delete_replace_update = require('suite').delete_replace_update
+---
+...
 engine_name = 'memtx'
 ---
 ...
diff --git a/test/engine_long/delete_replace_update.test.lua b/test/engine_long/delete_replace_update.test.lua
index 466b8f007..e6906a94b 100644
--- a/test/engine_long/delete_replace_update.test.lua
+++ b/test/engine_long/delete_replace_update.test.lua
@@ -1,3 +1,4 @@
+delete_replace_update = require('suite').delete_replace_update
 engine_name = 'memtx'
 iterations = 100000
 
diff --git a/test/engine_long/suite.lua b/test/engine_long/suite.lua
index 586160a1a..995257382 100644
--- a/test/engine_long/suite.lua
+++ b/test/engine_long/suite.lua
@@ -9,7 +9,7 @@ local function string_function()
     return random_string
 end
 
-function delete_replace_update(engine_name, iterations)
+local function delete_replace_update(engine_name, iterations)
     if (box.space._space.index.name:select{'tester'}[1] ~= nil) then
         box.space.tester:drop()
     end
@@ -66,7 +66,7 @@ function delete_replace_update(engine_name, iterations)
     return {counter, random_number, string_value_2, string_value_3}
 end
 
-function delete_insert(engine_name, iterations)
+local function delete_insert(engine_name, iterations)
     if (box.space._space.index.name:select{'tester'}[1] ~= nil) then
         box.space.tester:drop()
     end
@@ -104,4 +104,7 @@ function delete_insert(engine_name, iterations)
     return {counter, string_value_2}
 end
 
-_G.protected_globals = {'delete_replace_update', 'delete_insert'}
+return {
+    delete_replace_update = delete_replace_update,
+    delete_insert = delete_insert
+}

Re: [Tarantool-patches] [PATCH v7 2/2] luacheck: fix warnings in test/engine_long

[Sergey Bronnikov via Tarantool-patches]

Thanks for review and proposed diff!

It was applied without changes from my side and force pushed.

CI: https://gitlab.com/tarantool/tarantool/-/pipelines/243025033


On 17.01.2021 19:22, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
>> diff --git a/.luacheckrc b/.luacheckrc
>> index d58fd57b0..b581136b2 100644
>> --- a/.luacheckrc
>> +++ b/.luacheckrc
>> @@ -176,3 +176,9 @@ files["test/box-tap/extended_error.test.lua"] = {
>>           "forbidden_function",
>>       },
>>   }
>> +files["test/engine_long/suite.lua"] = {
>> +    globals = {
>> +        "delete_replace_update",
>> +        "delete_insert",
> There is no reason for these functions to be global. The only reason
> is being able to call a function via IPROTO_CALL, which is not the
> case here.
>
> I proper fix should make them local and returned as a result of
> require():
>
> ====================
> diff --git a/.luacheckrc b/.luacheckrc
> index 02349331e..8c67f9d47 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -131,9 +131,3 @@ files["test/box-tap/extended_error.test.lua"] = {
>           "forbidden_function",
>       },
>   }
> -files["test/engine_long/suite.lua"] = {
> -    globals = {
> -        "delete_replace_update",
> -        "delete_insert",
> -    }
> -}
> diff --git a/test/engine_long/box.lua b/test/engine_long/box.lua
> index 28a1560d5..94ae81301 100644
> --- a/test/engine_long/box.lua
> +++ b/test/engine_long/box.lua
> @@ -1,7 +1,5 @@
>   #!/usr/bin/env tarantool
>   
> -require('suite')
> -
>   os.execute("rm -rf vinyl_test")
>   os.execute("mkdir -p vinyl_test")
>   
> diff --git a/test/engine_long/delete_insert.result b/test/engine_long/delete_insert.result
> index b1d504271..06085ca78 100644
> --- a/test/engine_long/delete_insert.result
> +++ b/test/engine_long/delete_insert.result
> @@ -1,6 +1,9 @@
>   test_run = require('test_run')
>   ---
>   ...
> +delete_insert = require('suite').delete_insert
> +---
> +...
>   inspector = test_run.new()
>   ---
>   ...
> diff --git a/test/engine_long/delete_insert.test.lua b/test/engine_long/delete_insert.test.lua
> index 275aaa23e..b2c4ec308 100644
> --- a/test/engine_long/delete_insert.test.lua
> +++ b/test/engine_long/delete_insert.test.lua
> @@ -1,4 +1,5 @@
>   test_run = require('test_run')
> +delete_insert = require('suite').delete_insert
>   inspector = test_run.new()
>   engine = inspector:get_cfg('engine')
>   iterations = 100000
> diff --git a/test/engine_long/delete_replace_update.result b/test/engine_long/delete_replace_update.result
> index 66cb9c82c..b12e00688 100644
> --- a/test/engine_long/delete_replace_update.result
> +++ b/test/engine_long/delete_replace_update.result
> @@ -1,3 +1,6 @@
> +delete_replace_update = require('suite').delete_replace_update
> +---
> +...
>   engine_name = 'memtx'
>   ---
>   ...
> diff --git a/test/engine_long/delete_replace_update.test.lua b/test/engine_long/delete_replace_update.test.lua
> index 466b8f007..e6906a94b 100644
> --- a/test/engine_long/delete_replace_update.test.lua
> +++ b/test/engine_long/delete_replace_update.test.lua
> @@ -1,3 +1,4 @@
> +delete_replace_update = require('suite').delete_replace_update
>   engine_name = 'memtx'
>   iterations = 100000
>   
> diff --git a/test/engine_long/suite.lua b/test/engine_long/suite.lua
> index 586160a1a..995257382 100644
> --- a/test/engine_long/suite.lua
> +++ b/test/engine_long/suite.lua
> @@ -9,7 +9,7 @@ local function string_function()
>       return random_string
>   end
>   
> -function delete_replace_update(engine_name, iterations)
> +local function delete_replace_update(engine_name, iterations)
>       if (box.space._space.index.name:select{'tester'}[1] ~= nil) then
>           box.space.tester:drop()
>       end
> @@ -66,7 +66,7 @@ function delete_replace_update(engine_name, iterations)
>       return {counter, random_number, string_value_2, string_value_3}
>   end
>   
> -function delete_insert(engine_name, iterations)
> +local function delete_insert(engine_name, iterations)
>       if (box.space._space.index.name:select{'tester'}[1] ~= nil) then
>           box.space.tester:drop()
>       end
> @@ -104,4 +104,7 @@ function delete_insert(engine_name, iterations)
>       return {counter, string_value_2}
>   end
>   
> -_G.protected_globals = {'delete_replace_update', 'delete_insert'}
> +return {
> +    delete_replace_update = delete_replace_update,
> +    delete_insert = delete_insert
> +}

Re: [Tarantool-patches] [PATCH v7 2/2] luacheck: fix warnings in test/engine_long

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the fixes!

The patchset LGTM.

Re: [Tarantool-patches] [PATCH v7 0/2] Fix luacheck warnings in test/engine and test/engine_long

[Kirill Yukhin via Tarantool-patches]

Hello,

On 15 янв 15:00, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Changelog v7:
> 
> - rebased to a master branch (and includes fixes for test/box and test/box-tap to pass ci [1])
> - fixed new warnings
> 
> Changelog v6:
> 
> - splitted patch in test/ for patches per sub-directory
> - adjusted supressions in .luacheckrc
> - fixed formatting issues in .luacheckrc
> 
> 1. https://lists.tarantool.org/tarantool-patches/cover.1610709207.git.sergeyb@tarantool.org/T/#t
> 
> Gitlab CI: https://gitlab.com/tarantool/tarantool/-/pipelines/242152198
> GH branch: ligurio/gh-5458-luacheck-warnings-test-engine
> GH issues:
>   - https://github.com/tarantool/tarantool/issues/5458
>   - https://github.com/tarantool/tarantool/issues/5650

I've checked your patchset into 2.6, 2.7 and master.

--
Regards, Kirill Yukhin