[tarantool-patches] Re: [PATCH] lua: show locals when a tap test fails
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Aug 24 15:25:07 MSK 2018
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):
More information about the Tarantool-patches
mailing list