Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Timur,

Thanks for the patch! Though the change is quite trivial, the patch is
not OK right from the top. Please adjust the commit subject according to
our contribution guidelines[1].

Regarding the changes itself: why did you send it within absolutely
unrelated changeset? This is not required for the following patches.
This is not related to SQL at all. This patch should be on the another
branch, and the next patches should be rebased on that branch (if you
need it so much).

You can argue that this is a trivial oneliner and I am making lot of ado
about nothing, *but* this is your point of view. And now let me share
mine one with you:
* This patch should be backported to all longterm branches, since I
  consider this as a bug. Since this patch is on the bottom of the
  series, I need to make more actions for this. For what? I see no
  reason for it.
* You've chosen Sasha as a *first* reviewer for the first patch of the
  series. However, I'm sure every member of our "L" team can carefully
  review such a simple fix. Hence, you decided to stall the whole series
  until Sasha approves this tiny patch?
* On the other hand if this patch has enough LGTMs, it can be applied
  "out of order". And when it is applied, you will need to rebase your
  working branch with SQL-related changes on master. As a result this
  *optimization* has no sense at all and only makes maintainer's life
  much worse.

Finally, if there was not a nit regarding this patch, I would suggest
you a deal: leave the patch as is and consider everything written above
for the further patches. However, there are several nits to be resolved,
so please move this patch out from this series in a separate one.

On 11.06.21, Timur Safin via Tarantool-patches wrote:
> It always was a problem that reported source line was not
> pointing to the actual callee line number, but rather to

Don't get why you are talking about callee here: regardless the line to
be reported (the current one or the one where the function is defined),
it is a *caller* for the <traceback> function.

So, the actual problem you're writing is that the line with function
signature is used instead of the line where the function execution is
stopped at the moment of the <traceback> call.

As for me, these are the different reasons.

> the start of file, i.e. we have seen:
> ```
> [001] sql-tap/tkt-9a8b09f8e6.test.lua                 memtx
> [001] not ok 22 - 4.3 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0>
> [001]
> [001] not ok 23 - 4.4 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0>
> ```
> (see the :0 part)

Minor: Strictly saying :123 part is also broken. The only difference
between them is that :0 line is used for so called "main" function.

> Instead of correct line numbers:
> ```
> [001] sql-tap/tkt-9a8b09f8e6.test.lua                 memtx
> [001] not ok 22 - 4.3 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:242>
> [001]
> [001] not ok 23 - 4.4 #
> [001] Traceback:
> [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142>
> [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:252>
> ```
> 
> The problem was due to `.linedefined` used, instead of source line in `.currentline`.

Typo: The line exceeds 72 chars.

> 
> Closes #6134
> ---
>  src/lua/tap.lua | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lua/tap.lua b/src/lua/tap.lua
> index 346724d84..77fd8d096 100644
> --- a/src/lua/tap.lua
> +++ b/src/lua/tap.lua
> @@ -23,7 +23,7 @@ local function traceback(level)
>          local frame = {
>              source = info.source;
>              src = info.short_src;
> -            line = info.linedefined or 0;
> +            line = info.currentline or info.linedefined or 0;

Why did you leave such a complex code here? I believe you can use just
info.currentline here.

>              what = info.what;
>              name = info.name;
>              namewhat = info.namewhat;
> -- 
> 2.29.2
> 

[1]: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message

-- 
Best regards,
IM