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 27BE02B969 for ; Wed, 26 Sep 2018 20:45:22 -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 mPIofqjnBbm5 for ; Wed, 26 Sep 2018 20:45:22 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 60AF92A2CB for ; Wed, 26 Sep 2018 20:45:21 -0400 (EDT) Date: Thu, 27 Sep 2018 03:45:16 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] lua: show locals when a tap test fails Message-ID: <20180927004516.uivnneuomck3nls5@tkn_work_nb> References: <84509c8ab6e29159082ff27a5d72c9228adc1c21.1534595542.git.alexander.turenko@tarantool.org> <4ed59be1-1b73-423f-f016-d7ecb14ed907@tarantool.org> <20180923175917.fehrr36hxfthqgvw@tkn_work_nb> <2034b653-1054-470d-1296-d2d74f990a7b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <2034b653-1054-470d-1296-d2d74f990a7b@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, Kirill Yukhin On Mon, Sep 24, 2018 at 01:47:45PM +0300, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! > > > > > @@ -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. > > Understandable. Then LGTM, looks useful. > To be honest, now I don't so sure in the thought that we should enable it by default or discard completely. Sometimes it is useful to have clean picture which cases are passed and which are not w/o extra information (w/o locals and w/o traces). But sometimes it is useful to know exact reason of a certain fail and have as much info as possible (so locals are helpful). When work on a patch locally both situations occur. Don't sure about CI. Now I thinking about the following implementation: save locals for a first failing case, print it after the whole test in the uppermost test:check() and then print cases list (with ok/not ok, name and so on) again to bring it to the focus. It gives both benefits: a developer does not have to scroll for pass/fail list and have detailed info available after some scrolling. We could simplify that to 'print traces and locals during the test, then give a short list in the uppermost test:check()', but there is a pitfall. When something (say, a test case preparation) lead to premature exit (say, because of a raised error) we still want to see the whole picture (what is passed and what is failed) at the first glance and then, maybe, detailed information about the fail. With the proposed implementation we'll get the cases list, but not details: I think it is better then vice versa. So, to sum it up, I propose to keep a during-test output short, then, if the test reach the uppermost test:check(), give details and repeat the short cases list. Now I think how better to handle the raised error case. Maybe it worth to do xpcall around the whole test and then traverse over a stack to find a frame with the uppermost test and print locals for that frame. I'm thinking whether it is implementable w/o extra handle from a test writer, say, as a global option (_G.print_test_locals_before_exit) of tarantool that can be set by test-run. Ugh, tricky thing... WBR, Alexander Turenko.