[Tarantool-patches] [PATCH v8 03/14] test: fix luacheck warnings W113 in test/sql-tap
Sergey Bronnikov
sergeyb at tarantool.org
Thu Feb 25 13:39:17 MSK 2021
Hello,
thanks for review again
On 24.02.2021 00:25, Vladislav Shpilevoy wrote:
> 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.
Setted undefined variables/functions to nil everywhere instead of inline
luacheck supressions.
There some places in files lua_sql.test.lua, subquery.test.lua and
minmax2.test.lua where I still use luacheck suppressions
because setting variables/functions to nil doesn't work and broke tests.
>
>> 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.
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
> 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.
Done
>> - {"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.
Done (except files lua_sql.test.lua, subquery.test.lua and
minmax2.test.lua, see above)
>
>> return X(381, "X!cmd", [=[["concat",[["execsql","SELECT c FROM t3 WHERE b==10"]],["sql_search_count"]]]=])
>> end, {
>> -- <index-11.1>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20210225/2563c0b2/attachment.htm>
More information about the Tarantool-patches
mailing list