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

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi!

Thanks for the comments. Answered inline.

I squashed all changes into the one commit and rebased on top of fresh
1.10.

branch: https://github.com/tarantool/tarantool/tree/Totktonada/gh-3627-tap-show-local-variables
issue: https://github.com/tarantool/tarantool/issues/3627

WBR, Alexander Turenko.

> > +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.
> 

Fixed by your patch.

> > +      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.
> 

Fixed by your patch.

> > +          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?
> 

Yep. We can write it as '[nil]' or like so, but don't sure it worth to
do.

> >           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'?
> 

Fixed.

> > @@ -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.
> 

The primary reason of the option is to omit the second run. It is more
useful when the fail is flaky or when it occurs in CI. If you going to
do second run you can print anything you want manually.

So I think it worth to enable it by default or discard the patch
entirely. Or maybe control it from an environment variable.

I personally use the patch around month locally. It sometimes gives very
large output (graphql schemas), but saves time to write something like
`print(require('yaml').encode(res))` each time a test fails.

If you dislike such extra output for failing tests, we should skip the
patch entirely. It is useful or not useful, not something in the middle.

> > --- 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.
> 

The reason here is just same as for `test.trace = false`: to compare
output of the module 'tap' easily in the test.

My diff:

diff --git a/src/lua/tap.lua b/src/lua/tap.lua
index 625326065..765c62c77 100644
--- a/src/lua/tap.lua
+++ b/src/lua/tap.lua
@@ -252,8 +252,8 @@ local function test(parent, name, fun, ...)
         total   = 0;
         failed  = 0;
         planned = 0;
-        trace   = parent == nil and true or parent.trace;
-        locals  = parent == nil and true or parent.locals;
+        trace   = parent == nil or parent.trace;
+        locals  = parent == nil or parent.locals;
     }, test_mt)
     if fun ~= nil then
         test:diag('%s', test.name)