From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 C37CE469710 for ; Wed, 3 Jun 2020 20:27:28 +0300 (MSK) Date: Wed, 3 Jun 2020 20:27:11 +0300 From: Alexander Turenko Message-ID: <20200603172711.2dmy5bfekkzhi4as@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: sergeyb@tarantool.org Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org 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. 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. > @@ -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') (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 > + 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.