From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 977B4469710 for ; Thu, 4 Jun 2020 16:08:03 +0300 (MSK) Date: Thu, 4 Jun 2020 16:07:03 +0300 From: Sergey Bronnikov Message-ID: <20200604130703.GC19065@pony.bronevichok.ru> References: <20200603172711.2dmy5bfekkzhi4as@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200603172711.2dmy5bfekkzhi4as@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org Alexander, On 20:27 Wed 03 Jun , Alexander Turenko wrote: > I'm mostly okay with the patch, but decided to share several thoughts > around it. Whether you'll follow them or not, I'll consider it > ready-to-go. Just ping me, when you'll update everything that you want > to update. Patch has been updated according your comments. Also I have tested possible cases: - diff-based test, option --update-result is _not_ specified, result file is absent ====================================================================================== WORKR TEST PARAMS RESULT --------------------------------------------------------------------------------- [001] box/tuple.test.lua [001] TAP13 parse failed (Missing plan in the TAP source). [001] [001] No result file (box/tuple.result) found. [001] Run the test with --update-result option to write the new result file. [001] [ fail ] --------------------------------------------------------------------------------- - diff-based test, option --update-result is _not_ specified, result file is present ====================================================================================== WORKR TEST PARAMS RESULT --------------------------------------------------------------------------------- [001] box/tuple.test.lua [ pass ] --------------------------------------------------------------------------------- - diff-based test, option --update-result is specified, result file is absent ====================================================================================== WORKR TEST PARAMS RESULT --------------------------------------------------------------------------------- [001] box/tuple.test.lua [001] TAP13 parse failed (Missing plan in the TAP source). [001] [001] No result file (box/tuple.result) found. [001] [ updated ] --------------------------------------------------------------------------------- - diff-based test, option --update-result is specified, result file is present ====================================================================================== WORKR TEST PARAMS RESULT --------------------------------------------------------------------------------- [001] box/tuple.test.lua [ pass ] --------------------------------------------------------------------------------- - TAP-based test, option --update-result is _not_ specified ====================================================================================== WORKR TEST PARAMS RESULT --------------------------------------------------------------------------------- [001] box-tap/session.test.lua [ pass ] --------------------------------------------------------------------------------- - TAP-based test, option --update-result is specified ====================================================================================== WORKR TEST PARAMS RESULT --------------------------------------------------------------------------------- [001] box-tap/session.test.lua [ pass ] --------------------------------------------------------------------------------- > WBR, Alexander Turenko. > > > diff --git a/lib/test.py b/lib/test.py > > index 8bc1feb..733b90c 100644 > > --- a/lib/test.py > > +++ b/lib/test.py > > @@ -15,6 +15,7 @@ except ImportError: > > from StringIO import StringIO > > > > import lib > > +from lib.options import Options > > Unused. > > flake8 complains about this. removed > > @@ -235,9 +236,20 @@ class Test(object): > > elif (self.is_executed_ok and not > > self.is_equal_result and not > > os.path.isfile(self.result)) and not is_tap: > > + if lib.Options().args.update_result: > > + shutil.copy(self.tmp_result, self.result) > > + short_status = 'new' > > + color_stdout("[ new ]\n", schema='test_new') > > + else: > > + short_status = 'fail' > > + color_stdout("[ fail ]\n", schema='test_fail') > > (In brief: there are two way to do so; please choose one, which looks > better for you.) > > In [1] I proposed to keep this if-elif-else branching to be splitted by > status and to fall into 'else' at fail. The reason is that the 'else' > branch already do everything that we should do at fail: print output > diff, tarantool log, valgring log where appropriate. > > However in this certain case we have nothing to report: > self.is_crash_reported is set to True in check_tap_output() and 'TAP13 > parse failed' message is already shown. > > So I would just note that falling into 'else' branch would look more 'in > the spirit' of this code block, but I would let you decide how the code > would look better. > > The change upward your patch would look so: > > - elif (self.is_executed_ok and not > - self.is_equal_result and not > - os.path.isfile(self.result)) and not is_tap: > - if lib.Options().args.update_result: > - shutil.copy(self.tmp_result, self.result) > - short_status = 'new' > - color_stdout("[ new ]\n", schema='test_new') > - else: > - short_status = 'fail' > - color_stdout("[ fail ]\n", schema='test_fail') > + elif (self.is_executed_ok and > + not self.is_equal_result and > + not os.path.isfile(self.result) and > + not is_tap and > + lib.Options().args.update_result): > + shutil.copy(self.tmp_result, self.result) > + short_status = 'new' > + color_stdout("[ new ]\n", schema='test_new') > Applied your diff above. > (Also moved 'not' from line ends, because it was confusing.) > > Actual behaviour of both variants is the same. > > BTW, the 'TAP13 parse failed' message may now appear in the case, when > non-tap13 test fails and --update-result option is not passed. I would > add a few words here, like: 'No result file found. TAP13 parse failed'. > Maybe even 'Run the test with --update-result option to write the new > result file' on a separate line. It would give a developer better > understanding what actually occurs. > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-May/016775.html Agree. Updated error messages in check_tap_output(): except ValueError as e: - color_stdout('\nTAP13 parse failed: %s\n' % str(e), + color_stdout('\nTAP13 parse failed (%s).\n' % str(e), schema='error') + color_stdout('\nNo result file (%s) found.\n' % self.result, schema='error') + if not lib.Options().args.update_result: + color_stdout('Run the test with --update-result option to write the new result file.\n', + schema='error') self.is_crash_reported = True > > + elif (self.is_executed_ok and > > + not self.is_equal_result and > > + os.path.isfile(self.result) and > > + lib.Options().args.update_result): > > shutil.copy(self.tmp_result, self.result) > > - short_status = 'new' > > - color_stdout("[ new ]\n", schema='test_new') > > + short_status = 'updated' > > + color_stdout("[ updated ]\n", schema='test_new') > > It looks okay. > > > else: > > has_result = os.path.exists(self.tmp_result) > > if has_result: > > @@ -256,7 +268,8 @@ class Test(object): > > server.print_log(15) > > where = ": test execution aborted, reason " \ > > "'{0}'".format(diagnostics) > > - elif not self.is_crash_reported and not self.is_equal_result: > > + elif (not self.is_crash_reported and not self.is_equal_result and > > + not lib.Options().args.update_result): > > self.print_unidiff() > > server.print_log(15) > > where = ": wrong test output" > > As far as I see, we should not fall here, when --update-result is passed > (I failed to find any such situation). But even if it would be possible, > it would be not correct to just hide diff and set 'fail' status under > --update-result. > > I think it is safe enough to assume that update_result is False when > we're going to report failed status due to different results. I would > just remove this check and left the code as is. removed condition with lib.Options().args.update_result -- sergeyb@