Re: [Tarantool-patches] [PATCH] luacheck: change global vars to local in sql-tap

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks for the effort!

Just `luacheck .` shows the following warnings, for example:

test/sql-tap/minmax2.test.lua:64:9: setting non-standard global variable sql_search_count
test/sql-tap/subquery.test.lua:696:9: setting non-standard global variable callcnt

It seems, `make luacheck` target does not work.

 | $ make VERBOSE=1 luacheck
 | <...>
 | /usr/bin/luacheck --codes --config                \
 |     /home/alex/p/tarantool-meta/r/t-3/.luacheckrc \
 |     /home/alex/p/tarantool-meta/r/t-3
 | <...>

If I run luacheck this way, it does not give any warnings. This does not
look as the problem from your patch, but it is something we should fix
prior it.

> +files["test/sql-tap/**/*.lua"] = { 
> +    only = { 
> +        -- Setting an undefined global variable.
> +        "111"
> +    }   
> +}

Can we mention further issues here (gh-5181 and gh-5464)? It would make
clear that this clause is necessary only as the temporary solution until
we'll actually fix more / all warnings. I'll be glad if you'll add a
list of warnings and its amount into the gh-5464 issue: it'll help with
futher estimation.

Can we use a blacklist instead of the whitelist? This way we'll also
catch all standard warnings except the listed ones. It seems, there are
not so many warnings to blacklist:

 | $ luacheck --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \
 |     grep -o '(W[0-9]\+)' | sort -u | cat -n
 |      1	(W111)
 |      2	(W113)
 |      3	(W211)
 |      4	(W212)
 |      5	(W213)
 |      6	(W231)
 |      7	(W311)
 |      8	(W511)
 |      9	(W512)
 |     10	(W542)
 |     11	(W611)
 |     12	(W612)
 |     13	(W613)
 |     14	(W614)
 |     15	(W621)
 |     16	(W631)

I would ask you to comment each of them (like you do it for the
whitelist) and leave an issue numbers, where it is expected to be fixed.

Some warnings that were not appear before, appear now:

 | # Remove the suppression
 | $ git diff | cat
 | diff --git a/.luacheckrc b/.luacheckrc
 | index 6fbd1a5b3..e9a906f08 100644
 | --- a/.luacheckrc
 | +++ b/.luacheckrc
 | @@ -33,13 +33,6 @@ exclude_files = {
 |      "src/box/lua/serpent.lua",
 |  }
 | 
 | -files["test/sql-tap/**/*.lua"] = {
 | -    only = {
 | -        -- Setting an undefined global variable.
 | -        "111"
 | -    }
 | -}
 | -
 |  files["src/lua/help.lua"] = {
 |      -- Globals defined for interactive mode.
 |      globals = {"help", "tutorial"},
 | 
 | # Revert the changes in the test/ directory.
 | $ git show HEAD test >p && patch -R -p1 <p && rm p
 | 
 | # Collect warnings.
 | $ luacheck --no-color --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \
 |     grep -o '(W[0-9]\+).*$' | sort | uniq -c | sort -rnk1,1 >before.txt
 | 
 | # Apply back the changes in the test/ directory.
 | $ git checkout -- test
 | 
 | # Collect the warnings again.
 | $ luacheck --no-color --codes test/sql-tap/*.lua test/sql-tap/*/*.lua | \
 |     grep -o '(W[0-9]\+).*$' | sort | uniq -c | sort -rnk1,1 >after.txt
 | 
 | # Look at the changed counters.
 | $ diff -u before.txt after.txt | grep '^+'
 | +++ after.txt	2020-11-06 01:40:41.940542423 +0300
 | +     58 (W211) unused variable 'testprefix'
 | +     33 (W631) line is too long (132 > 120)
 | +     20 (W631) line is too long (126 > 120)
 | +      9 (W113) accessing undefined variable 'sql_search_count'
 | +      9 (W111) setting non-standard global variable 'sql_search_count'
 | +      8 (W113) accessing undefined variable 'sql'
 | +      7 (W211) unused variable 'i'
 | +      6 (W111) setting non-standard global variable 'counter'
 | +      4 (W211) unused variable 'json'
 | +      4 (W113) accessing undefined variable 'i'
 | +      3 (W211) unused variable 'yaml'
 | +      3 (W211) unused variable 's'
 | +      2 (W211) unused variable 'rc'
 | +      2 (W211) unused variable 'msg'
 | +      2 (W113) accessing undefined variable 'STMT'
 | +      2 (W113) accessing undefined variable 'res'
 | +      2 (W111) setting non-standard global variable 'callcnt'
 | +      1 (W231) variable 'V' is never accessed
 | +      1 (W231) variable 'c' is never accessed
 | +      1 (W231) variable 'b' is never accessed
 | +      1 (W231) variable 'a' is never accessed
 | +      1 (W211) unused variable 'view_v1'
 | +      1 (W211) unused variable 'sql_search_count'
 | +      1 (W211) unused variable 'prev'
 | +      1 (W211) unused variable 'NULL'
 | +      1 (W211) unused variable 'nm'
 | +      1 (W211) unused variable 'limit'
 | +      1 (W211) unused variable 'i0'
 | +      1 (W211) unused variable 'fio'
 | +      1 (W211) unused variable 'DB'
 | +      1 (W113) accessing undefined variable 'y'
 | +      1 (W113) accessing undefined variable 'stmt'
 | +      1 (W113) accessing undefined variable 'r'
 | +      1 (W113) accessing undefined variable 'DB'
 | 
 | # Look more thoroughly.
 | $ diff -u before.txt after.txt | grep '^[-+]'

Some of those new warnings looks suspectful, e.g. 'testprefix' should be
removed in several places instead of making it local. We should verify
all those cases. I would formulate the criteria as follows: a fix of a
warning should not produce any new warnings (even such as 'too long
line').

test/sql-tap/e_expr.test.lua:

> -            local A = val[2]
> -            local B = val[3]
> -            local C = val[4]
> -            local testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)
> +            local A, B, C, testname
> +            A = val[2]
> +            B = val[3]
> +            C = val[4]
> +            testname = string.format("e_expr-1.%s.%s.%s", opname[op1], opname[op2], tn)

It seems, you changed those lines unintentionally.

test/sql-tap/gh2250-trigger-chain-limit.test.lua:

> -    function check(sql)
> -        msg = ''
> +local function check(sql)
> +        local msg = ''

The indentation of the 'local function' clause is broken.

test/sql-tap/index7.test.lua:

>  -- Capture the output of a pragma in a TEMP table.
>  --
> -local function capture_pragma(db, tabname, sql)
> +local function capture_pragma(db, tabname, sql) -- luacheck: ignore once
>      once = 1
>  end

The function is referred only from the commented code. There is no any
reference to 'once' too. In fact, it is a kind of inaccurate converting
from the original sqlite test ([1]). I would just remove it. If we'll
implement partial indices and will enable related SQL tests, we'll
anyway need to look into the original code.

[1]: https://sqlite.org/src/file?name=test/index7.test

test/sql-tap/trigger9.test.lua:

>  #!/usr/bin/env tarantool
> +-- luacheck: globals test
>  test = require("sqltester")
>  test:plan(28)

I all other places you add the luacheck directive on the same line as a
corresponding Lua statement. Let's do this consistently.

I'm okay with doing it in two lines if it becomes too long when written
in one line (not this case).