[tarantool-patches] Re: [PATCH] lua: show locals when a tap test fails

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch! See 7 comments below and the patch
on the branch.

On 18/08/2018 15:36, Alexander Turenko wrote:
> Fixes #3627.
> ---
> 
> branch: Totktonada/gh-3627-tap-show-local-variables
> travis-ci: https://travis-ci.org/tarantool/tarantool/builds/417616160
> issue: https://github.com/tarantool/tarantool/issues/3627
> 
>   src/lua/tap.lua           | 45 ++++++++++++++++++++++++++++++++++-----
>   test/app-tap/tap.result   | 26 ++++++++++++++++++++--
>   test/app-tap/tap.test.lua | 21 +++++++++++++++---
>   3 files changed, 82 insertions(+), 10 deletions(-)
> 
> diff --git a/src/lua/tap.lua b/src/lua/tap.lua
> index edc9f2211..9dd531422 100644
> --- a/src/lua/tap.lua
> +++ b/src/lua/tap.lua
> @@ -36,12 +36,43 @@ local function traceback(level)
>       return trace
>   end
>   
> +local function locals(test, level)
> +    level = level or 3
> +    local variables = {}
> +    local idx = 1
> +    while true do
> +      local name, value = debug.getlocal(level, idx)

1. Broken indentation: use 4 spaces, not 2.

> +      if name ~= nil then
> +          -- compare a table with a tuple raises an error, so we check types
> +          -- first

2. Please, start sentence with a capital letter and finish with
the dot.

3. Out of 66 symbols.

> +          local eq = type(value) == type(test) and value == test
> +          -- temporary values start with '('
> +          if not name:startswith('(') and not eq then
> +              variables[name] = value

4. So the variables, having nil value, are not showed?

> +          end
> +      else
> +          break
> +      end
> +      idx = 1 + idx
> +    end
> +    return variables
> +end
> @@ -221,6 +255,7 @@ local function test(parent, name, fun, ...)
>           failed  = 0;
>           planned = 0;
>           trace   = parent == nil and true or parent.trace;
> +        locals  = parent == nil and true or parent.locals;

5. If parent == nil, then this turns into 'true and true or ..',
so what is a point of 'and true'?

>       }, test_mt)
>       if fun ~= nil then
>           test:diag('%s', test.name)
> @@ -245,7 +280,7 @@ local function check(test)
>       if test.planned ~= test.total then
>           if test.parent ~= nil then
>               ok(test.parent, false, "bad plan", { planned = test.planned;
> -                run = test.total})
> +                run = test.total}, {locals = false})

6. Are we sure, that locals should be printed by default? Maybe
it would be better to print them on demand only via a global
option and remove per-test-case option? As I imagine a typical
test fail, when a test did not pass, you turn locals printing
on and run the test again.

>           else
>               diag(test, string.format("bad plan: planned %d run %d",
>                   test.planned, test.total))
> diff --git a/test/app-tap/tap.test.lua b/test/app-tap/tap.test.lua
> index 0e1de7f1c..29dc0b100 100755
> --- a/test/app-tap/tap.test.lua
> +++ b/test/app-tap/tap.test.lua
> @@ -12,15 +12,16 @@ local tap = require "tap"
>   -- Create a root test
>   --
>   test = tap.test("root test")
> --- Disable stack traces for this test because Tarantool test system also
> --- checks test output.
> +-- Disable stack traces and locals for this test because Tarantool test system
> +-- also checks test output.
>   test.trace = false
> +test.locals = false

7. In the comment 6 I meant exactly this: it is not
useful to reset locals, when I do not need them.

>   
>   --
>   -- ok, fail and skip predicates
>   --
>   
> -test:plan(32) -- plan to run 3 test
> +test:plan(33) -- plan to run 3 test
>   test:ok(true, 'true') -- basic function
>   local extra = { state = 'some userful information to debug on failure',
>           details = 'a table argument formatted using yaml.encode()' }

My diff (so far it is requested by SOP to paste it here):