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! Did you see this response and some of the others?

On 24.01.2021 18:33, Vladislav Shpilevoy via Tarantool-patches wrote:
> 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",