From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 A3B22469710 for ; Tue, 19 May 2020 13:24:24 +0300 (MSK) Date: Tue, 19 May 2020 13:23:48 +0300 From: Sergey Bronnikov Message-ID: <20200519102348.GA43919@pony.bronevichok.ru> References: <20200324080124.GA67461@pony.bronevichok.ru> <20200515222155.dztctndmwtr67qaj@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200515222155.dztctndmwtr67qaj@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v1] 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: Oleg Piskunov , tarantool-patches@dev.tarantool.org, Cyrill Gorcunov Hello, Alexander thanks for review. See my comments inline. Patch is updated in a branch. On 01:21 Sat 16 May , Alexander Turenko wrote: > There is https://github.com/tarantool/test-run/issues/194 > > My initial thought was that we'll fix both problems at once. I think it > would be good to have both actions under one option: update existing > result files and write new result files, because this way it is simpler > to use. Agree, patch modified to cover case when .result file is absent at all. > However I don't insist: if you want to implement only updating existing > result files, I don't mind. > > On Tue, Mar 24, 2020 at 11:01:24AM +0300, Sergey Bronnikov wrote: > > In case of test failure test-run.py create a file .reject with actual > > test output and one need to move .reject file to .result manually when > > test has a valid behaviout. With option --update-ref-output test-run.py > > will do it automagically. > > > > Fixes: #4654 > > Nit: It does not reference tarantool's issue in GitHub web interface. I > use a full link when I need to link an issue from another repository. Updated commit message. > > > > GitHub branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4654-update-ref-output > > > > --- > > lib/options.py | 8 ++++++++ > > lib/test.py | 19 +++++++++++++++---- > > 2 files changed, 23 insertions(+), 4 deletions(-) > > > > diff --git a/lib/options.py b/lib/options.py > > index 8bacb4a..174a62f 100644 > > --- a/lib/options.py > > +++ b/lib/options.py > > @@ -201,6 +201,14 @@ class Options: > > help="""Run the server under 'luacov'. > > Default: false.""") > > > > + parser.add_argument( > > + "--update-ref-output", > > Maybe --update-result it would be more intuitive for developers, but I > don't insist. Replaced "--updtae-ref-output" to "--update-result". > > > + dest="update_reference_output", > > + action="store_true", > > + default=False, > > + help="""Update file with reference output (.reject) in case of fail > > Typo: .reject -> .result. Fixed. > > + and set status pass. Default: false.""") > > + > > We have status 'new' (which in fact means that a test is passed, but > shown as [ new ] in the output). I would introduce [ updated ] bagde for > this sake. Added new status. > NB: If you'll introduce 'updated' status, let's also count it in > statistics as 'updated' (AFAIR, it should work properly just based on > test.run() return value, but, please, check). Tested and looks like new status accounted in statistics. > > # XXX: We can use parser.parse_intermixed_args() on > > # Python 3.7 to understand commands like > > # ./test-run.py foo --exclude bar baz > > diff --git a/lib/test.py b/lib/test.py > > index 3e93af3..396bb89 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 > > from lib.colorer import color_stdout > > from lib.utils import non_empty_valgrind_logs > > from lib.utils import print_tail_n > > @@ -242,23 +243,33 @@ class Test(object): > > color_stdout("[ new ]\n", schema='test_new') > > else: > > has_result = os.path.exists(self.tmp_result) > > + update_reference = lib.Options().args.update_reference_output > > has_result, update_reference -- two terms are used to reference one > thing. I guess you dislike 'result' term, but it should be either kept > or changed consistently. Both variables has gone after rewriting condition. > > if has_result: > > - shutil.copy(self.tmp_result, self.reject) > > - short_status = 'fail' > > + if update_reference: > > + reject_dest = self.result > > + else: > > + reject_dest = self.reject > > + shutil.copy(self.tmp_result, reject_dest) > > + if update_reference: > > + short_status = 'pass' > > + else: > > + short_status = 'fail' > > We have one if-branch, which sets 'skip' status, another for 'pass', one > for 'new' and this one, which previously set 'fail'. I propose to keep > this code block organized in such way and add one more branch, which > will set 'updated' status. > > It also looks more clear, because here we have two `if update_reference` > for 9 lines of code: it is better to hoist this branching up to parent's > if-elif-else chain. Done. > Like so (not tested): > > | shutil.copy(self.tmp_result, self.result) > | short_status = 'new' > | color_stdout("[ new ]\n", schema='test_new') > | + 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 = 'updated' > | + color_stdout("[ updated ]\n", schema='test_new') > | else: > | has_result = os.path.exists(self.tmp_result) > | if has_result: > > (You may add 'test_updated' to a schema in colorer.py if you want.) I'm no a fan of coloring output in a terminal. Using existed color schemes is ok for me. > > color_stdout("[ fail ]\n", schema='test_fail') > > It will show '[ fail ]' even when a result fill will be updated? Line has gone after rewriting code. > > > > where = "" > > if not self.is_crash_reported and not has_result: > > color_stdout('\nCannot open %s\n' % self.tmp_result, > > schema='error') > > - elif not self.is_crash_reported and not self.is_executed_ok: > > + elif not self.is_crash_reported and not self.is_executed_ok and \ > > + not update_reference: > > When exit code is non-zero we should report a test failure anyway. Well, removed "not update_reference" from condition. > > self.print_diagnostics(self.reject, > > "Test failed! Output from reject file " > > "{0}:\n".format(self.reject)) > > 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 update_reference: > > To be honest I dislike mangling of the code block that process a test > failure with those 'if update_reference' conditions. Let's process test > fail situation and update result situation separately (as proposed > above). We don't need to process option "update_result" here, so separate branch is useless. -- sergeyb@