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 E6F432A700 for ; Sun, 23 Sep 2018 13:59:21 -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 xCcp6Xw8uS_V for ; Sun, 23 Sep 2018 13:59:21 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 25F8929A31 for ; Sun, 23 Sep 2018 13:59:21 -0400 (EDT) Date: Sun, 23 Sep 2018 20:59:18 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] lua: show locals when a tap test fails Message-ID: <20180923175917.fehrr36hxfthqgvw@tkn_work_nb> References: <84509c8ab6e29159082ff27a5d72c9228adc1c21.1534595542.git.alexander.turenko@tarantool.org> <4ed59be1-1b73-423f-f016-d7ecb14ed907@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4ed59be1-1b73-423f-f016-d7ecb14ed907@tarantool.org> 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: Vladislav Shpilevoy Cc: Eugine Blikh , tarantool-patches@freelists.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)