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

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

Hello!

On 27.02.2021 02:42, Vladislav Shpilevoy wrote:
> 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.
Thanks for solution, it's definitely better than suppression.
>>           "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.
>
made it local
>>       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.
Done
>
>>       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.

luacheck doesn't warn about it and tests passed successfully.

Although from code style point of view it's better to define it...


--- a/test/sql-tap/triggerC.test.lua
+++ b/test/sql-tap/triggerC.test.lua
@@ -779,6 +779,8 @@ for testno, v in ipairs(tests11) do
      --         -- X(891, "X!cmd", 
[=[["concat",["defaults"],["defaults"]]]=])
      --     })

+    -- Legacy from the original code. Must be replaced with valid value.
+    local defaults = nil
      test:do_test(
          "triggerC-11."..testno..".3",
          function()
@@ -790,6 +792,7 @@ for testno, v in ipairs(tests11) do
                  SELECT a,b FROM log;
              ]]
          end, {
+            defaults
          })

      --

>>           })
>>   
>>       --
>> 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()
>