Hello,
thanks for review again
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 = nilFor 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
DoneTo 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 (except files lua_sql.test.lua, subquery.test.lua and minmax2.test.lua, see above)- {"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 X3. 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>