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!

We are almost there.

See 3 comments below.

> diff --git a/test/sql-tap/badutf1.test.lua b/test/sql-tap/badutf1.test.lua
> index 654629bf7..f375fea0f 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
> +local sql_exec = nil

>> For me it's not make sense. Replaced push/pop suppression with variable definition.

1. Lets at least be consistent and either use `-- luacheck` comments or
`= nil` hack. Not both, especially not on 2 adjacent lines. It simply
raises unnecessary questions why couldn't it be done in one way.

>  test:do_test(
>      "badutf-1.10",
>      function()> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index 2a189582d..9ef5e7925 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -1537,32 +1561,26 @@ test:do_select_tests(
>  -- considered equal to other NULL values and distinct from all non-NULL
>  -- values.
>  --
> --- MUST_WORK_TEST
> -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.

To me it is not clear why null can be "". Empty string is not the
same as NULL.

The same for the other `db("nullvalue", "null")` removals where
`null` was used for anything. If there are such other places.

> -        {"5", "SELECT NULL UNION ALL SELECT 'ab'", {null, "ab"}},
> -        {"6", "SELECT NULL UNION     SELECT 'ab'", {null, "ab"}},
> +        {"5", "SELECT NULL UNION ALL SELECT 'ab'", {"", "ab"}},
> +        {"6", "SELECT NULL UNION     SELECT 'ab'", {"", "ab"}},
>          {"7", "SELECT NULL INTERSECT SELECT 'ab'", {}},
> -        {"8", "SELECT NULL EXCEPT    SELECT 'ab'", {null}},
> -        {"9", "SELECT NULL UNION ALL SELECT 0", {null, 0}},
> -        {"10", "SELECT NULL UNION     SELECT 0", {null, 0}},
> +        {"8", "SELECT NULL EXCEPT    SELECT 'ab'", {""}},
> +        {"9", "SELECT NULL UNION ALL SELECT 0", {"", 0}},
> +        {"10", "SELECT NULL UNION     SELECT 0", {"", 0}},
>          {"11", "SELECT NULL INTERSECT SELECT 0", {}},
> -        {"12", "SELECT NULL EXCEPT    SELECT 0", {null}},
> -        {"13", "SELECT c FROM q1 UNION ALL SELECT g FROM q3", {null, -42.47, "null", 2, 2}},
> -        {"14", "SELECT c FROM q1 UNION     SELECT g FROM q3", {null, -42.47, 2}},
> +        {"12", "SELECT NULL EXCEPT    SELECT 0", {""}},
> +        {"13", "SELECT c FROM q1 UNION ALL SELECT g FROM q3", {"", -42.47, "", 2, 2}},
> +        {"14", "SELECT c FROM q1 UNION     SELECT g FROM q3", {"", -42.47, 2}},
>          {"15", "SELECT c FROM q1 INTERSECT SELECT g FROM q3", {}},
> -        {"16", "SELECT c FROM q1 EXCEPT    SELECT g FROM q3", {null, -42.47}},
> +        {"16", "SELECT c FROM q1 EXCEPT    SELECT g FROM q3", {"", -42.47}},
>      })
> -
> -db("nullvalue", "")
> -end
> diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
> index ea25727a4..574faafd6 100755
> --- a/test/sql-tap/index1.test.lua
> +++ b/test/sql-tap/index1.test.lua
> @@ -585,7 +585,7 @@ test:do_test(
>          for i = 1, 50, 1 do
>              test:execsql(string.format("INSERT INTO t3 VALUES('x%sx',%s,0.%s)", i, i, i))
>          end
> -        local sql_search_count = 0
> +        -- luacheck: ignore X

3. Lets be consistent and not jump from one method to another.
We either use luacheck comments or `local X = nil` hacks. The same
for other similar inconsistencies in this and the rest of the patches.

>          return X(381, "X!cmd", [=[["concat",[["execsql","SELECT c FROM t3 WHERE b==10"]],["sql_search_count"]]]=])
>      end, {
>          -- <index-11.1>