Re: [Tarantool-patches] [PATCH v8 04/14] test: fix luacheck warnings W211 in test/sql-tap

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi,

thanks for review!

On 24.01.2021 20:34, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> On 21.01.2021 13:49, sergeyb@tarantool.org wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> W211 (Unused local variable)
>>
>> Part of #5464
>>
>> test: fix luacheck warnings W111 in test/sql-tap
>> ---
>> diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
>> index 81b08e223..f29b26175 100755
>> --- a/test/sql-tap/e_expr.test.lua
>> +++ b/test/sql-tap/e_expr.test.lua
>> @@ -1080,7 +1080,6 @@ if (0>0) then
>>       local function parameter_test(tn, sql, params, result)
>>           local stmt = sql_prepare_v2("db", sql, -1)
>>           for _ in X(0, "X!foreach", [=[["number name",["params"]]]=]) do
>> -            local nm = sql_bind_parameter_name(stmt, number)
> 1. You should not delete the function call. It seems it must have
> had an effect on the statement object. Without this call in future it
> will be harder to understand what is missing if you delete it.
reverted hunk
>
>>               X(480, "X!cmd", [=[["do_test",[["tn"],".name.",["number"]],[["list","set","",["nm"]]],["name"]]]=])
>>               sql_bind_int(stmt, number, ((-1) * number))
>>           end
>> diff --git a/test/sql-tap/misc1.test.lua b/test/sql-tap/misc1.test.lua
>> index 7d928bea0..b1c4f026b 100755
>> --- a/test/sql-tap/misc1.test.lua
>> +++ b/test/sql-tap/misc1.test.lua
>> @@ -726,19 +726,8 @@ test:do_execsql_test(
>>   -- MUST_WORK_TEST collate
>>   if 0>0 then
>>       db("collate", "numeric", "numeric_collate")
>> -    local function numeric_collate(lhs, rhs)
>> -        if (lhs == rhs)
>> -        then
>> -            return 0
>> -        end
>> -        return X(0, "X!expr", [=[["?:",[">",["lhs"],["rhs"]],3,["-",1]]]=])
>> -    end
> 2. The more I look at these db + X pairs, the more it seems like an
> artifact after TCL -> Lua conversion. I think originally it was
> supposed that the function after 'db()' should be called, and 'X'
> should execute something. Worth investigating how these tests looked
> originally. Otherwise we may delete too much and not able to resurrect
> the tests in future.

Yes, you are right regarding artifacts.

Although I would not like an idea to fix sql tests in scope of task with 
luacheck

and just suppress places where we cannot fix right now.

>
> If these code blocks are important for understanding the test (which I
> don't understand now, but anyway), maybe it would better to comment
> them out instead of deleing.
>
> Or delete the entire test cases if we are not going to try to resurrect
> them, which is also fine.