From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0B0362A2E1 for ; Fri, 24 Aug 2018 08:25:11 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4hDPYT-IKweT for ; Fri, 24 Aug 2018 08:25:10 -0400 (EDT) Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BED7A2A08D for ; Fri, 24 Aug 2018 08:25:10 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] lua: show locals when a tap test fails References: <84509c8ab6e29159082ff27a5d72c9228adc1c21.1534595542.git.alexander.turenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4ed59be1-1b73-423f-f016-d7ecb14ed907@tarantool.org> Date: Fri, 24 Aug 2018 15:25:07 +0300 MIME-Version: 1.0 In-Reply-To: <84509c8ab6e29159082ff27a5d72c9228adc1c21.1534595542.git.alexander.turenko@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alexander Turenko , Eugine Blikh Cc: tarantool-patches@freelists.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):