Re: [Tarantool-patches] [PATCH v8 04/14] test: fix luacheck warnings W211 in test/sql-tap

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

Hello,

On 24.02.2021 00:26, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 5 comments below.
>
>> diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
>> index 37a7b7177..046ed8711 100755
>> --- a/test/sql-tap/e_expr.test.lua
>> +++ b/test/sql-tap/e_expr.test.lua
>> @@ -1087,7 +1087,7 @@ if (0>0) then
>>           local number = nil
>>           local stmt = sql_prepare_v2(sql, -1)
>>           for _ in X(0, "X!foreach", [=[["number name",["params"]]]=]) do
>> -            local nm = sql_bind_parameter_name(stmt, number)
>> +            local nm = sql_bind_parameter_name(stmt, number) -- luacheck: no unused
> 1. Why do you need 'nm' if it is unused? You don't need to drop the
> entire line. Just delete 'nm' variable and leave the function call
> without saving its result to anything. The same below for 'rc' and
> 'sql_finalize()'.

reverted changes back

--- a/test/sql-tap/e_expr.test.lua
+++ b/test/sql-tap/e_expr.test.lua
@@ -1087,7 +1087,7 @@ if (0>0) then
          local number = nil
          local stmt = sql_prepare_v2(sql, -1)
          for _ in X(0, "X!foreach", [=[["number name",["params"]]]=]) do
-            local nm = sql_bind_parameter_name(stmt, number) -- 
luacheck: no unused
+            sql_bind_parameter_name(stmt, number)
              X(480, "X!cmd", 
[=[["do_test",[["tn"],".name.",["number"]],[["list","set","",["nm"]]],["name"]]]=])
              sql_bind_int(stmt, number, ((-1) * number))
          end
@@ -1103,7 +1103,7 @@ if (0>0) then
          -- Legacy from the original code. Must be replaced with analogue
          -- functions from box.
          local sql_finalize = nil
-        local rc = sql_finalize(stmt) -- luacheck: no unused
+        sql_finalize(stmt)
          X(491, "X!cmd", 
[=[["do_test",[["tn"],".rc"],[["list","set","",["rc"]]],"sql_OK"]]=])
          X(492, "X!cmd", 
[=[["do_test",[["tn"],".res"],[["list","set","",["res"]]],["result"]]]=])
      end

>
>>               X(480, "X!cmd", [=[["do_test",[["tn"],".name.",["number"]],[["list","set","",["nm"]]],["name"]]]=])
>>               sql_bind_int(stmt, number, ((-1) * number))
>>           end
>> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
>> index 9cd517673..5dab23007 100755
>> --- a/test/sql-tap/func.test.lua
>> +++ b/test/sql-tap/func.test.lua
>> @@ -1382,7 +1380,6 @@ test:do_execsql_test(
>>       })
>>   
>>   db("cache", "flush")
>> -V = "three"
> 2. 'V' is supposed to be a variable. It is used in the queries
> in the surrounding code. Please, don't delete parts of the tests.
> It will not make it simpler to restore the context later. The tests
> in this patchset either must be deleted, or resurrected, or hacked
> to keep their context and not fail luacheck. What to choose - depends
> on individual tests. Here clearly we need to keep 'V'. Try to make
> another pass of self-review to locate more of such test-"breaking"
> changes.

reverted and suppressed

--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1382,6 +1382,7 @@ test:do_execsql_test(
      })

  db("cache", "flush")
+local V = "three" -- luacheck: no unused
  test:do_execsql_test(
      "13.8.6",
      [[

>> diff --git a/test/sql-tap/selectA.test.lua b/test/sql-tap/selectA.test.lua
>> index a608ab093..db4d03985 100755
>> --- a/test/sql-tap/selectA.test.lua
>> +++ b/test/sql-tap/selectA.test.lua
>> @@ -2359,9 +2357,6 @@ test:do_execsql_test(
>>   if (0 > 0)
>>    then
>>   end
>> -local function f(args)
>> -    return 1
>> -end
> 3. It seems the function is supposed to be somehow used a few
> lines below. Please, keep it.
>
reverted and suppressed

--- a/test/sql-tap/selectA.test.lua
+++ b/test/sql-tap/selectA.test.lua
@@ -2357,6 +2357,9 @@ test:do_execsql_test(
  if (0 > 0)
   then
  end
+local function f(args) -- luacheck: no unused
+    return 1
+end

  -- TODO stored procedures are not supported by now
  --db("func", "f", "f")

>>   -- TODO stored procedures are not supported by now
>>   --db("func", "f", "f")> diff --git a/test/sql-tap/sort.test.lua b/test/sql-tap/sort.test.lua
>> index 1cf0d14a8..0de598969 100755
>> --- a/test/sql-tap/sort.test.lua
>> +++ b/test/sql-tap/sort.test.lua
>> @@ -802,7 +801,6 @@ box.internal.sql_create_function("cksum", cksum)
>>               "sort-14."..tn,
>>               function()
>>                   sql_test_control("sql_TESTCTRL_SORTER_MMAP", "db", mmap_limit)
>> -                local prev = ""
> 4. Prev is supposed to be used in the request on the next line. It
> has `$prev` expression inside.

reverted and suppressed

--- a/test/sql-tap/sort.test.lua
+++ b/test/sql-tap/sort.test.lua
@@ -801,6 +801,7 @@ box.internal.sql_create_function("cksum", cksum)
              "sort-14."..tn,
              function()
                  sql_test_control("sql_TESTCTRL_SORTER_MMAP", "db", 
mmap_limit)
+                local prev = "" -- luacheck: no unused
                  X(536, "X!cmd", [=[["db","eval"," SELECT * FROM t11 
ORDER BY b ","\n         if {$b != [cksum $a]} {error \"checksum 
failed\"}\n         if {[string compare $b $prev] < 0} {error \"sort 
failed\"}\n         set prev $b\n       "]]=])
                  return X(541, "X!cmd", [=[["set","",""]]=])
              end, {

>
>>                   X(536, "X!cmd", [=[["db","eval"," SELECT * FROM t11 ORDER BY b ","\n         if {$b != [cksum $a]} {error \"checksum failed\"}\n         if {[string compare $b $prev] < 0} {error \"sort failed\"}\n         set prev $b\n       "]]=])
>>                   return X(541, "X!cmd", [=[["set","",""]]=])
>>               end, {
>> diff --git a/test/sql-tap/trigger1.test.lua b/test/sql-tap/trigger1.test.lua
>> index 8224ef1ec..0efdd21bc 100755
>> --- a/test/sql-tap/trigger1.test.lua
>> +++ b/test/sql-tap/trigger1.test.lua
>> @@ -482,8 +482,6 @@ test:execsql [[
>>   -- trigger works.  Then drop the trigger.  Make sure the table is
>>   -- still there.
>>   --
>> -local view_v1 = "view v1"
> 5. The variable is "used" in some of the commented out tests below.

reverted and suppressed

--- a/test/sql-tap/trigger1.test.lua
+++ b/test/sql-tap/trigger1.test.lua
@@ -483,6 +483,7 @@ test:execsql [[
  -- still there.
  --

+local view_v1 = "view v1" -- luacheck: no unused
  -- do_test trigger1-6.1 {
  --   execsql {SELECT type, name FROM sql_master}
  -- } [concat $view_v1 {table t2}]

>
>> -
>>   
>>   -- do_test trigger1-6.1 {
>>   --   execsql {SELECT type, name FROM sql_master}