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

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

Hi again! Thanks for the fixes!

>>> -if 0>0 then
>>> -db("nullvalue", "null")
>>>  test:do_select_tests(
>>>      "e_select-7.9",
>>>      {
>>> -        {"1", "SELECT NULL UNION ALL SELECT NULL", {null, null}},
>>> -        {"2", "SELECT NULL UNION     SELECT NULL", {null}},
>>> -        {"3", "SELECT NULL INTERSECT SELECT NULL", {null}},
>>> +        {"1", "SELECT NULL UNION ALL SELECT NULL", {"", ""}},
>>> +        {"2", "SELECT NULL UNION     SELECT NULL", {""}},
>>> +        {"3", "SELECT NULL INTERSECT SELECT NULL", {""}},
>>>          {"4", "SELECT NULL EXCEPT    SELECT NULL", {}},
>> 2. But why? What was the effect of `db("nullvalue", "null")` in the
>> original tests? If you don't know then maybe it is worth leaving it
>> as is + adding `local null = box.NULL` or `local null = nil` so at
>> least we could preserve the test body unchanged for future
>> investigations.
> 
> Well, reverted hunks back and disabled testcase in the test.
> 
> I found a source code of original test [1] and it is not clear
> 
> why "nullvalue" redefined there.
> 
> 1. https://www.sqlite.org/src/file?ci=trunk&name=test%2Fe_select.test&ln=1504

It is not 'redefinition' it seems.
https://www.oreilly.com/library/view/using-sqlite/9781449394592/re64.html

It is a marker for output. From what I understood, sqlite prints NULLs as
empty string, and here in the test they wanted to look for NULLs in the
result. But you can't look for N empty strings. So NULL's string representation
was replaced with "null" temporary. In our case it won't happen because we
know number of returned columns and their exact values. Somebody will find
the same in the future and will either fix or delete the test.

See 4 more comments below and diff in the end of the email.

> diff --git a/test/sql-tap/lua_sql.test.lua b/test/sql-tap/lua_sql.test.lua
> index b22f195ca..4704970c8 100755
> --- a/test/sql-tap/lua_sql.test.lua
> +++ b/test/sql-tap/lua_sql.test.lua
> @@ -75,9 +75,10 @@ box.schema.func.create('CHECK_FROM_SQL_TO_LUA', {language = 'Lua',
>                         exports = {'LUA', 'SQL'}})
>  
>  -- check for different types
> -for i = 1, #from_sql_to_lua, 1 do
> +for i = 1, #from_sql_to_lua, 1 do -- luacheck: ignore from_sql_to_lua
>      test:do_execsql_test(
>          "lua_sql-2.2."..i,
> +        -- luacheck: ignore from_sql_to_lua

1. You can just declare local from_sql_to_lua and also save it to _G. No
need to ignore it.

>          "select check_from_sql_to_lua("..i..","..from_sql_to_lua[i][1]..")",
>          {1})
>  end
> @@ -102,9 +103,10 @@ box.schema.func.create('CHECK_FROM_LUA_TO_SQL', {language = 'Lua',
>                         exports = {'LUA', 'SQL'}})
>  
>  -- check for different types
> -for i = 1, #from_lua_to_sql, 1 do
> +for i = 1, #from_lua_to_sql, 1 do -- luacheck: ignore from_lua_to_sql
>      test:do_execsql_test(
>          "lua_sql-2.3."..i,
> +        -- luacheck: ignore from_lua_to_sql
>          "select "..tostring(from_lua_to_sql[i][1]).." = check_from_lua_to_sql("..i..")",
>          {true})
>  end
> diff --git a/test/sql-tap/minmax2.test.lua b/test/sql-tap/minmax2.test.lua
> index f3d608aab..b6c02a4cc 100755
> --- a/test/sql-tap/minmax2.test.lua
> +++ b/test/sql-tap/minmax2.test.lua
> @@ -72,6 +72,7 @@ test:do_test(
>  test:do_test(
>      "minmax2-1.2",
>      function()
> +        -- luacheck: ignore sql_search_count
>          return box.stat.sql().sql_search_count - sql_search_count

2. There is no reason for sql_search_count to be global.

>      end, 19)
>  
> @@ -89,6 +90,7 @@ test:do_test(
>  test:do_test(
>      "minmax2-1.4",
>      function()
> +        -- luacheck: ignore sql_search_count
>          return box.stat.sql().sql_search_count - sql_search_count
>      end, 19)
>  
> diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
> index f57a22624..bb03f25ab 100755
> --- a/test/sql-tap/subquery.test.lua
> +++ b/test/sql-tap/subquery.test.lua
> @@ -684,6 +684,8 @@ test:do_test(
>          -- This is the key test.  The subquery should have only run once.  If
>          -- The double-quoted identifier "two" were causing the subquery to be
>          -- processed as a correlated subquery, then it would have run 4 times.
> +
> +        -- luacheck: ignore callcnt
>          return callcnt

3. This can be fixed, lets not ignore it. One way would be
to store callcnt in a table which is saved in a local variable
and into _G.

	local callcnt = {value = 0}
	_G.callcnt = callcnt

Then you can use callcnt.value from anywhere to reference the
same counter.

Another way is to have it local and update it via a function
stored in _G.

Also can simply write _G.callcnt in 3 more places instead of
ignoring it.

>      end, 1)
>  
> @@ -706,7 +708,9 @@ test:do_test(
>  test:do_test(
>      "subquery-6.2",
>      function()
> +        -- luacheck: ignore callcnt
>          return callcnt
> +
>      end, 4)
>  
>  test:do_test(
> @@ -725,6 +729,7 @@ test:do_test(
>  test:do_test(
>      "subquery-6.4",
>      function()
> +        -- luacheck: ignore callcnt
>          return callcnt
>      end, 1)
>  
> diff --git a/test/sql-tap/triggerC.test.lua b/test/sql-tap/triggerC.test.lua
> index e95641938..7d496bc4d 100755
> --- a/test/sql-tap/triggerC.test.lua
> +++ b/test/sql-tap/triggerC.test.lua
> @@ -791,7 +791,6 @@ for testno, v in ipairs(tests11) do
>                  SELECT a,b FROM log;
>              ]]
>          end, {
> -            defaults

4. The variable is supposed to be filled with something, from
what I see in the commented code.

You can declare it with nil somewhere above to keep the not
working tests intact.

>          })
>  
>      --
> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
> index 2f1af29b0..40fc71998 100755
> --- a/test/sql-tap/view.test.lua
> +++ b/test/sql-tap/view.test.lua
> @@ -1119,6 +1093,9 @@ if (0 > 0)
>              -- </view-22.1>
>          })
>  
> +    -- Legacy from the original code. Must be replaced with analogue
> +    -- functions from box.
> +    local X = nil
>      test:do_test(
>          "view-22.2",
>          function()

I tried to fix some of my comments:

====================
diff --git a/test/sql-tap/lua_sql.test.lua b/test/sql-tap/lua_sql.test.lua
index 4704970c8..d073ffe4f 100755
--- a/test/sql-tap/lua_sql.test.lua
+++ b/test/sql-tap/lua_sql.test.lua
@@ -51,7 +51,7 @@ for _, val in ipairs({
         {result})
 end
 
-_G.from_sql_to_lua = {
+local from_sql_to_lua = {
     [1] = {1, 1},
     [2] = {"1", 1},
     [3] = {"1.5", 1.5},
@@ -60,6 +60,7 @@ _G.from_sql_to_lua = {
     [6] = {"x'0500'", "\u{0005}\u{0000}"},
     [7] = {"123123123123123", 123123123123123LL},
 }
+_G.from_sql_to_lua = from_sql_to_lua
 
 box.schema.func.create('CHECK_FROM_SQL_TO_LUA', {language = 'Lua',
                        is_deterministic = true,
@@ -75,10 +76,9 @@ box.schema.func.create('CHECK_FROM_SQL_TO_LUA', {language = 'Lua',
                        exports = {'LUA', 'SQL'}})
 
 -- check for different types
-for i = 1, #from_sql_to_lua, 1 do -- luacheck: ignore from_sql_to_lua
+for i = 1, #from_sql_to_lua, 1 do
     test:do_execsql_test(
         "lua_sql-2.2."..i,
-        -- luacheck: ignore from_sql_to_lua
         "select check_from_sql_to_lua("..i..","..from_sql_to_lua[i][1]..")",
         {1})
 end
diff --git a/test/sql-tap/minmax2.test.lua b/test/sql-tap/minmax2.test.lua
index b6c02a4cc..6a7859d42 100755
--- a/test/sql-tap/minmax2.test.lua
+++ b/test/sql-tap/minmax2.test.lua
@@ -56,12 +56,12 @@ test:do_execsql_test(
         -- </minmax2-1.0>
     })
 
-_G.sql_search_count = 0
+local sql_search_count = 0
 
 test:do_test(
     "minmax2-1.1",
     function()
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT min(x) FROM t1"
     end, {
         -- <minmax2-1.1>
@@ -72,14 +72,13 @@ test:do_test(
 test:do_test(
     "minmax2-1.2",
     function()
-        -- luacheck: ignore sql_search_count
         return box.stat.sql().sql_search_count - sql_search_count
     end, 19)
 
 test:do_test(
     "minmax2-1.3",
     function()
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT max(x) FROM t1"
     end, {
         -- <minmax2-1.3>
@@ -90,7 +89,6 @@ test:do_test(
 test:do_test(
     "minmax2-1.4",
     function()
-        -- luacheck: ignore sql_search_count
         return box.stat.sql().sql_search_count - sql_search_count
     end, 19)
 
@@ -98,7 +96,7 @@ test:do_test(
     "minmax2-1.5",
     function()
         test:execsql "CREATE INDEX t1i1 ON t1(x DESC)"
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT min(x) FROM t1"
     end, {
         -- <minmax2-1.5>
@@ -109,13 +107,13 @@ test:do_test(
 test:do_test(
     "minmax2-1.6",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 1)
 
 test:do_test(
     "minmax2-1.7",
     function()
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT max(x) FROM t1"
     end, {
         -- <minmax2-1.7>
@@ -126,13 +124,13 @@ test:do_test(
 test:do_test(
     "minmax2-1.8",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 0)
 
 test:do_test(
     "minmax2-1.9",
     function()
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT max(y) FROM t1"
     end, {
         -- <minmax2-1.9>
@@ -143,7 +141,7 @@ test:do_test(
 test:do_test(
     "minmax2-1.10",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 19)
 
 test:do_test(
@@ -153,7 +151,7 @@ test:do_test(
             CREATE TABLE t2(a INTEGER PRIMARY KEY, b INT );
             INSERT INTO t2 SELECT x, y FROM t1;
         ]]
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT min(a) FROM t2"
     end, {
         -- <minmax2-2.0>
@@ -164,13 +162,13 @@ test:do_test(
 test:do_test(
     "minmax2-2.1",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 0)
 
 test:do_test(
     "minmax2-2.2",
     function()
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT max(a) FROM t2"
     end, {
         -- <minmax2-2.2>
@@ -181,7 +179,7 @@ test:do_test(
 test:do_test(
     "minmax2-2.3",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 0)
 
 test:do_test(
@@ -190,7 +188,7 @@ test:do_test(
         test:execsql "INSERT INTO t2 VALUES((SELECT max(a) FROM t2)+1,999)"
 
 
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql "SELECT max(a) FROM t2"
     end, {
         -- <minmax2-3.0>
@@ -201,7 +199,7 @@ test:do_test(
 test:do_test(
     "minmax2-3.1",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 0)
 
 test:do_test(
@@ -210,7 +208,7 @@ test:do_test(
         test:execsql "INSERT INTO t2 VALUES((SELECT max(a) FROM t2)+1,999)"
 
 
-        _G.sql_search_count = box.stat.sql().sql_search_count
+        sql_search_count = box.stat.sql().sql_search_count
         return test:execsql " SELECT b FROM t2 WHERE a=(SELECT max(a) FROM t2) "
 
 
@@ -224,7 +222,7 @@ test:do_test(
 test:do_test(
     "minmax2-3.3",
     function()
-        return box.stat.sql().sql_search_count - _G.sql_search_count
+        return box.stat.sql().sql_search_count - sql_search_count
     end, 1)
 
 test:do_execsql_test(
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index bb03f25ab..df3075ee3 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -685,8 +685,7 @@ test:do_test(
         -- The double-quoted identifier "two" were causing the subquery to be
         -- processed as a correlated subquery, then it would have run 4 times.
 
-        -- luacheck: ignore callcnt
-        return callcnt
+        return _G.callcnt
     end, 1)
 
 -- Ticket #1380.  Make sure correlated subqueries on an IN clause work
@@ -708,8 +707,7 @@ test:do_test(
 test:do_test(
     "subquery-6.2",
     function()
-        -- luacheck: ignore callcnt
-        return callcnt
+        return _G.callcnt
 
     end, 4)
 
@@ -729,8 +727,7 @@ test:do_test(
 test:do_test(
     "subquery-6.4",
     function()
-        -- luacheck: ignore callcnt
-        return callcnt
+        return _G.callcnt
     end, 1)
 
 box.func.CALLCNT:drop()