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! Thanks for the patch!

See 3 comments below.

On 21.01.2021 13:49, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> W113 (Accessing an undefined global variable)
> 
> Part of #5464
> ---
>  .luacheckrc                    |  37 +++++++++-
>  test/sql-tap/badutf1.test.lua  |  10 ++-
>  test/sql-tap/index1.test.lua   |   2 +-
>  test/sql-tap/lua/sqltester.lua |   2 +-
>  test/sql-tap/lua_sql.test.lua  |   6 +-
>  test/sql-tap/minmax2.test.lua  |   2 +
>  test/sql-tap/select1.test.lua  |   4 ++
>  test/sql-tap/subquery.test.lua |   7 +-
>  test/sql-tap/table.test.lua    |   6 +-
>  test/sql-tap/tkt1443.test.lua  |   2 +-
>  test/sql-tap/trigger9.test.lua |   1 +
>  test/sql-tap/triggerC.test.lua |   1 -
>  test/sql-tap/view.test.lua     | 126 +++++++++++++++------------------
>  13 files changed, 122 insertions(+), 84 deletions(-)
> 
> diff --git a/.luacheckrc b/.luacheckrc
> index 81aa61a2b..eb709d6a3 100644
> --- a/.luacheckrc
> +++ b/.luacheckrc
> @@ -124,3 +122,38 @@ files["test/box-tap/extended_error.test.lua"] = {
>          "forbidden_function",
>      },
>  }
> +files["test/sql-tap/analyze5.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}
> +files["test/sql-tap/cast.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}
> +files["test/sql-tap/func.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}
> +files["test/sql-tap/e_expr.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}
> +files["test/sql-tap/e_select1.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}
> +files["test/sql-tap/misc1.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}
> +files["test/sql-tap/sort.test.lua"] = {
> +    ignore = {
> +        "113",
> +    },
> +}

1. The commit title says "fix", but it does not look like a fix.
I suggest you to try harder to remove the warnings, not hide them.
For example, analyze5 can be fixed like this:

====================
diff --git a/.luacheckrc b/.luacheckrc
index eb709d6a3..977eb2945 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -122,11 +122,6 @@ files["test/box-tap/extended_error.test.lua"] = {
         "forbidden_function",
     },
 }
-files["test/sql-tap/analyze5.test.lua"] = {
-    ignore = {
-        "113",
-    },
-}
 files["test/sql-tap/cast.test.lua"] = {
     ignore = {
         "113",
diff --git a/test/sql-tap/analyze5.test.lua b/test/sql-tap/analyze5.test.lua
index 1d6665ad0..a9eb4dd2f 100755
--- a/test/sql-tap/analyze5.test.lua
+++ b/test/sql-tap/analyze5.test.lua
@@ -26,6 +26,8 @@ local function eqp(sql)
     return test:execsql("EXPLAIN QUERY PLAN"..sql)
 end
 
+local X = nil
+
 local function alpha(blob)
     local ret = ""
     for _, c in ipairs(X(37, "X!cmd", [=[["split",["blob"],""]]=])) do
@@ -155,6 +157,9 @@ test:do_test(
 
 -- Verify that range queries generate the correct row count estimates
 --
+local t1x = nil
+local t1y = nil
+local t1z = nil
 for i, v in pairs({
 {'z>=0 AND z<=0',      t1z,  400},
 {'z>=1 AND z<=1',      t1z,  300},
====================

It does not make much sense, and the whole test does not make it, because
it is disabled. But it is better than just move the ignores from one
place to another. At least now they are local to the disabled places,
not to the whole file.

Cast.test.lua:

====================
diff --git a/.luacheckrc b/.luacheckrc
index eb709d6a3..15944e378 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -127,11 +127,6 @@ files["test/sql-tap/analyze5.test.lua"] = {
         "113",
     },
 }
-files["test/sql-tap/cast.test.lua"] = {
-    ignore = {
-        "113",
-    },
-}
 files["test/sql-tap/func.test.lua"] = {
     ignore = {
         "113",
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index 4d7c92cfc..bcee65d9b 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -786,13 +786,19 @@ test:do_execsql_test(
 -- Test to see if it is possible to trick sql into reading past
 -- the end of a blob when converting it to a number.
 if 0 > 0 then
+-- Legacy from the original code. Must be replaced with analogue
+-- functions from box.
+local sql_prepare = nil
+local sql_bind_blob = nil
+local sql_step = nil
+local sql_column_int = nil
+local STMT
 test:do_test(
     "cast-3.32.1",
     function()
-        local blob, DB, STMT
+        local blob
         blob = 1234567890
-        DB = sql_connection_pointer("db")
-        STMT = sql_prepare(DB, "SELECT CAST(? AS NUMBER)", -1, "TAIL")
+        STMT = sql_prepare("SELECT CAST(? AS NUMBER)", -1, "TAIL")
         sql_bind_blob("-static", STMT, 1, blob, 5)
         return sql_step(STMT)
     end, {
====================

Func.test.lua:

====================
diff --git a/.luacheckrc b/.luacheckrc
index eb709d6a3..7608240c5 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -132,11 +132,6 @@ files["test/sql-tap/cast.test.lua"] = {
         "113",
     },
 }
-files["test/sql-tap/func.test.lua"] = {
-    ignore = {
-        "113",
-    },
-}
 files["test/sql-tap/e_expr.test.lua"] = {
     ignore = {
         "113",
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index d14eef421..a83b15fbc 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -1048,7 +1048,8 @@ test:do_execsql_test(
 --
 -- MUST_WORK_TEST testfunc not implemented
 if 0 > 0 then
-local DB = sql_connection_pointer("db")
+local DB = nil
+local X = nil
 X(525, "X!cmd", [=[["sql_register_test_function",["::DB"],"testfunc"]]=])
 test:do_catchsql_test(
     "func-10.1",
@@ -1142,6 +1143,18 @@ end
 --
 -- MUST_WORK_TEST test_destructor_count not implemented
 if 0 > 0 then
+local X = nil
+-- Legacy from the original code. Must be replaced with analogue
+-- functions from box.
+local sql_prepare = nil
+local sql_bind_text = nil
+local sql_bind_blob = nil
+local sql_column_text = nil
+local sql_step = nil
+local sql_finalize = nil
+local db = nil
+local STMT
+
 if X(595, "X!cmd", "[\"expr\",\"[db eval {PRAGMA encoding}]==\\\"UTF-8\\\"\"]")
  then
     test:do_execsql_test(
@@ -1300,10 +1313,9 @@ test:do_execsql_test(
 test:do_test(
     "func-13.7",
     function()
-        local DB, sql, STMT, res
-        DB = sql_connection_pointer("db")
+        local sql, STMT, res
         sql = "SELECT test_auxdata( ? , a ) FROM t4;"
-        STMT = sql_prepare(DB, sql, -1, "TAIL")
+        STMT = sql_prepare(sql, -1, "TAIL")
         sql_bind_text(STMT, 1, "hello\0", -1)
         res = {  }
         while X(690, "X!cmd", [=[["expr"," \"sql_ROW\"==[sql_step $STMT] "]]=])
@@ -1455,7 +1467,7 @@ test:do_test(
         test:execsql([[
             CREATE TABLE tbl2(id integer primary key, a INT, b INT);
         ]])
-        local STMT = sql_prepare(DB, "INSERT INTO tbl2 VALUES(1, ?, ?)", -1, "TAIL")
+        local STMT = sql_prepare("INSERT INTO tbl2 VALUES(1, ?, ?)", -1, "TAIL")
         sql_bind_blob(STMT, 1, "abc", 3)
         sql_step(STMT)
         sql_finalize(STMT)
@@ -2439,6 +2451,7 @@ test:do_execsql_test(
 --
 -- MUST_WORK_TEST
 if 0>0 then
+local X = nil
 test:do_execsql_test(
     "func-25.1",
     [[
@@ -2470,7 +2483,7 @@ test:do_test(
     "func-26.2",
     function()
         local a = ""
-        for _ in X(0, "X!for", [=[["set i 1","$i<=$::sql_MAX_FUNCTION_ARG","incr i"]]=]) do
+        for i in X(0, "X!for", [=[["set i 1","$i<=$::sql_MAX_FUNCTION_ARG","incr i"]]=]) do
             table.insert(a,i)
         end
         return test:execsql(string.format([[
@@ -2486,7 +2499,7 @@ test:do_test(
     "func-26.3",
     function()
         local a = ""
-        for _ in X(0, "X!for", [=[["set i 1","$i<=$::sql_MAX_FUNCTION_ARG+1","incr i"]]=]) do
+        for i in X(0, "X!for", [=[["set i 1","$i<=$::sql_MAX_FUNCTION_ARG+1","incr i"]]=]) do
             table.insert(a,i)
         end
         return test:catchsql(string.format([[
@@ -2502,7 +2515,7 @@ test:do_test(
     "func-26.4",
     function()
         local a = ""
-        for _ in X(0, "X!for", [=[["set i 1","$i<=$::sql_MAX_FUNCTION_ARG-1","incr i"]]=]) do
+        for i in X(0, "X!for", [=[["set i 1","$i<=$::sql_MAX_FUNCTION_ARG-1","incr i"]]=]) do
             table.insert(a,i)
         end
         return test:catchsql(string.format([[
====================

I am sure the other files also can be properly fixed, with stubs
and comments for legacy old functions, and with normal fixes for
the things like in the last lines of the diff above, where it was
a real bug - loop variable was not defined.

> diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua
> index 654629bf7..b7fa8ce0b 100755
> --- a/test/sql-tap/badutf1.test.lua
> +++ b/test/sql-tap/badutf1.test.lua
> @@ -93,8 +93,12 @@ test:do_test(
>      })
>  
>  -- commented as it uses utf16
> -if 0>0 then
> -sql("db2", "")
> +-- testcases are broken
> +-- https://github.com/tarantool/tarantool/issues/5743
> +local is_gh_5743_closed = false
> +if is_gh_5743_closed then
> +sql("db2", "") -- luacheck: ignore sql
> +-- luacheck: push ignore sql_exec

2. Or you can use 'local sql_exec = nil', and won't need to
write 'pop'.

The same to the other places, where you try to ignore the same
unknown variable again and again. However I don't insist on
hacks vs luacheck comments.

>  test:do_test(
>      "badutf-1.10",
>      function()
> @@ -206,7 +210,7 @@ test:do_test(
>          -- </badutf-1.20>
>      })
>  end
> -
> +-- luacheck: pop
>  > diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
> index 2f1af29b0..67f461132 100755
> --- a/test/sql-tap/view.test.lua
> +++ b/test/sql-tap/view.test.lua
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env tarantool
>  local test = require("sqltester")
> -test:plan(78)
> +test:plan(83)
>  
>  --!./tcltestrunner.lua
>  -- 2002 February 26
> @@ -501,21 +501,18 @@ test:do_execsql_test(
>      })
>  
>  -- MUST_WORK_TEST

3. Why did you keep MUST_WORK_TEST, if it passes now?
The same in other places below.

> -if (0 > 0)
> - then
> -    test:do_test(
> -        "view-7.2",
> -        function()
> -            return test:execsql [[
> -                SELECT * FROM test;
> -            ]]
> -        end, {
> -            -- <view-7.2>
> -            1, 2, 3
> -            -- </view-7.2>
> -        })
> +test:do_test(
> +    "view-7.2",
> +    function()
> +        return test:execsql [[
> +            SELECT * FROM test;
> +        ]]
> +    end, {
> +        -- <view-7.2>
> +        1, 2, 3
> +        -- </view-7.2>
> +    })
>  
> -end
>  test:do_execsql_test(
>      "view-7.3",
>      [[
> @@ -531,21 +528,18 @@ test:do_execsql_test(
>      })
>  
>  -- MUST_WORK_TEST
> -if (0 > 0)
> - then
> -    test:do_test(
> -        "view-7.4",
> -        function()
> -            return test:execsql [[
> -                SELECT * FROM test;
> -            ]]
> -        end, {
> -            -- <view-7.4>
> -            1, 2, 3
> -            -- </view-7.4>
> -        })
> +test:do_test(
> +    "view-7.4",
> +    function()
> +        return test:execsql [[
> +            SELECT * FROM test;
> +        ]]
> +    end, {
> +        -- <view-7.4>
> +        1, 2, 3
> +        -- </view-7.4>
> +    })
>  
> -end
>  test:do_execsql_test(
>      "view-7.5",
>      [[
> @@ -561,21 +555,18 @@ test:do_execsql_test(
>      })
>  
>  -- MUST_WORK_TEST
> -if (0 > 0)
> - then
> -    test:do_test(
> -        "view-7.6",
> -        function()
> -            return test:execsql [[
> -                SELECT * FROM test;
> -            ]]
> -        end, {
> -            -- <view-7.6>
> -            1, 2, 3
> -            -- </view-7.6>
> -        })
> +test:do_test(
> +    "view-7.6",
> +    function()
> +        return test:execsql [[
> +            SELECT * FROM test;
> +        ]]
> +    end, {
> +        -- <view-7.6>
> +        1, 2, 3
> +        -- </view-7.6>
> +    })
>  
> -end
>  test:do_execsql_test(
>      "view-8.1",
>      [[
> @@ -588,33 +579,29 @@ test:do_execsql_test(
>      })
>  
>  -- MUST_WORK_TEST
> -if (0 > 0)
> - then
> -    test:do_test(
> -        "view-8.2",
> -        function()
> -            return test:execsql [[
> -                SELECT * FROM v6 ORDER BY xyz;
> -            ]]
> -        end, {
> -            -- <view-8.2>
> -            7, 2, 13, 5, 19, 8, 27, 12
> -            -- </view-8.2>
> -        })
> -
> -    -- MUST_WORK_TEST problem with column names
> -    test:do_execsql_test(
> -        "view-8.3",
> -        [[
> -            CREATE VIEW v7(a) AS SELECT pqr+xyz FROM v6;
> -            SELECT * FROM v7 ORDER BY a;
> -        ]], {
> -            -- <view-8.3>
> -            9, 18, 27, 39
> -            -- </view-8.3>
> -        })
> +test:do_test(
> +    "view-8.2",
> +    function()
> +        return test:execsql [[
> +            SELECT * FROM v6 ORDER BY xyz;
> +        ]]
> +    end, {
> +        -- <view-8.2>
> +        7, 2, 13, 5, 19, 8, 27, 12
> +        -- </view-8.2>
> +    })
>  
> -end
> +-- MUST_WORK_TEST problem with column names
> +test:do_execsql_test(
> +    "view-8.3",
> +    [[
> +        CREATE VIEW v7(a) AS SELECT pqr+xyz FROM v6;
> +        SELECT * FROM v7 ORDER BY a;
> +    ]], {
> +        -- <view-8.3>
> +        9, 18, 27, 39
> +        -- </view-8.3>
> +    })
>  
>  test:do_execsql_test(
>      "view-8.4",