From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 8FFF07030F; Thu, 25 Feb 2021 13:39:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8FFF07030F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614249560; bh=7PMlbWVx3oMtYZYtDOdFUpFRYJfAgqZngak1KqBOyBQ=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=TBBKZjhB1lXkWJKe5fDSw/RP5X7ltd9VcEQhh/ahKWJhI34EpWrBhzj4hErrZN8oU S7+ut3kkzAjoCtp4Zb+KmJIbS0FGNr5sTSTXRN91QGuM/tU84K86mq1lbA1wK5B1eH RJA584l7QLRS6VJC1lF5gP8yDBB4p2YrDdI+ThYI= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2B0E97030F for ; Thu, 25 Feb 2021 13:39:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2B0E97030F Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lFE3B-0001Va-Tk; Thu, 25 Feb 2021 13:39:18 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org References: <3a5360daf42cd920e3ce536020a6ff2fbccc095e.1611232655.git.sergeyb@tarantool.org> <140f1c8e-dc25-3295-9bd8-e7835e2e89c8@tarantool.org> Message-ID: Date: Thu, 25 Feb 2021 13:39:17 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/alternative; boundary="------------3924B6F2EADB05936226577B" Content-Language: en-US X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD975C3EC174F5669229511437AA01F46811CFCF616A939B362182A05F5380850402549869BB1FAD42BA5D88DFE527C12A370042E14BA9644C413633396175B4C87 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B9FBA884A7C9B8BAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063752AC809489EC5B9C8638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95CDDE882590F889B1CBD88B9E868B25B32C9412E23D67280A0A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3CF36E64A7E3F8E58117882F4460429728AD0CFFFB425014E868A13BD56FB6657A7F4EDE966BC389F9E8FC8737B5C22497A7B0CDC955A1711089D37D7C0E48F6CCF19DD082D7633A0E7DDDDC251EA7DABAAAE862A0553A39223F8577A6DFFEA7C74F8CD32ECFF047543847C11F186F3C5E7DDDDC251EA7DABCC89B49CDF41148FA8EF81845B15A4842623479134186CDE6BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5428C4894E60CA8F8AAD101D9E21D33DB6D43F2F251FEE143D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7557E988E9157162368E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3403182E70AE6895E01A16B22D73C76FB2C476395F636923C2A87E079A8E67EDD6585B2A38EBA0633A1D7E09C32AA3244C188929932ECD630E585005F505A795A1259227199D06760AFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojvz1c9SWJtj8gtks2nblxOg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A06382260797DB3F07F67D7FA7CE3BF239A5C8EDD788429FD8613638ED9BB8B05EE7B3AFB559BB5D741EB96D19CD4E7312BAA970A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v8 03/14] test: fix luacheck warnings W113 in test/sql-tap X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------3924B6F2EADB05936226577B Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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, { >> -- --------------3924B6F2EADB05936226577B Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit

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>


      

    

  


--------------3924B6F2EADB05936226577B--