* [Tarantool-patches] [PATCH v2] Add option to update file with reference output @ 2020-05-19 10:25 sergeyb 2020-05-20 8:03 ` Alexander V. Tikhonov 2020-06-03 17:27 ` Alexander Turenko 0 siblings, 2 replies; 7+ messages in thread From: sergeyb @ 2020-05-19 10:25 UTC (permalink / raw) To: tarantool-patches, gorcunov, alexander.turenko; +Cc: o.piskunov From: Sergey Bronnikov <sergeyb@tarantool.org> New option covers two cases: - 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 behaviour. With option --update-result test-run.py will do it automatically. - in case of abcense reference result file test-run.py forced to create such file in a source directory and set test status "new". This patch changes behaviour. With option --update-result test status is "new" and result file is created, without option test status is "fail" and result file is not created. Fixes https://github.com/tarantool/tarantool/issues/4654 Fixes https://github.com/tarantool/tarantool/issues/4258 Closes #194 --- GH branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4654-update-ref-output lib/options.py | 8 ++++++++ lib/test.py | 21 +++++++++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/options.py b/lib/options.py index 8bacb4a..47bbc0f 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-result", + dest="update_result", + action="store_true", + default=False, + help="""Update or create file with reference output (.result). + Default: false.""") + # 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 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 from lib.colorer import color_stdout from lib.utils import non_empty_valgrind_logs from lib.utils import print_tail_n @@ -152,7 +153,7 @@ class Test(object): it to stdout. Returns short status of the test as a string: 'skip', 'pass', - 'new', or 'fail'. There is also one possible value for + 'new', 'updated' or 'fail'. There is also one possible value for short_status, 'disabled', but it returned in the caller, TestSuite.run_test(). """ @@ -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') + 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') 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" -- 2.23.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output 2020-05-19 10:25 [Tarantool-patches] [PATCH v2] Add option to update file with reference output sergeyb @ 2020-05-20 8:03 ` Alexander V. Tikhonov 2020-05-20 8:23 ` Sergey Bronnikov 2020-06-03 17:27 ` Alexander Turenko 1 sibling, 1 reply; 7+ messages in thread From: Alexander V. Tikhonov @ 2020-05-20 8:03 UTC (permalink / raw) To: sergeyb; +Cc: tarantool-patches Hi Sergey, thanks for the patch. In general LGTM, but want to ask if it is needed to mention in help message of the new --update-result option that the new result file will be in the new result style, I mean the following difference, please check: mv replication/misc.result replication/misc.saved ./test-run.py replication/misc.test.lua --update-result diff replication/misc.result replication/misc.saved On Tue, May 19, 2020 at 01:25:46PM +0300, sergeyb@tarantool.org wrote: > From: Sergey Bronnikov <sergeyb@tarantool.org> > > New option covers two cases: > - 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 behaviour. With option --update-result test-run.py > will do it automatically. > - in case of abcense reference result file test-run.py forced to create > such file in a source directory and set test status "new". This patch > changes behaviour. With option --update-result test status is "new" > and result file is created, without option test status is "fail" and > result file is not created. > > Fixes https://github.com/tarantool/tarantool/issues/4654 > Fixes https://github.com/tarantool/tarantool/issues/4258 > Closes #194 > --- > GH branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4654-update-ref-output > > lib/options.py | 8 ++++++++ > lib/test.py | 21 +++++++++++++++++---- > 2 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/lib/options.py b/lib/options.py > index 8bacb4a..47bbc0f 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-result", > + dest="update_result", > + action="store_true", > + default=False, > + help="""Update or create file with reference output (.result). > + Default: false.""") > + > # 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 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 > from lib.colorer import color_stdout > from lib.utils import non_empty_valgrind_logs > from lib.utils import print_tail_n > @@ -152,7 +153,7 @@ class Test(object): > it to stdout. > > Returns short status of the test as a string: 'skip', 'pass', > - 'new', or 'fail'. There is also one possible value for > + 'new', 'updated' or 'fail'. There is also one possible value for > short_status, 'disabled', but it returned in the caller, > TestSuite.run_test(). > """ > @@ -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') > + 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') > 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" > -- > 2.23.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output 2020-05-20 8:03 ` Alexander V. Tikhonov @ 2020-05-20 8:23 ` Sergey Bronnikov 0 siblings, 0 replies; 7+ messages in thread From: Sergey Bronnikov @ 2020-05-20 8:23 UTC (permalink / raw) To: Alexander V. Tikhonov; +Cc: tarantool-patches Hi, Alexander On 11:03 Wed 20 May , Alexander V. Tikhonov wrote: > Hi Sergey, thanks for the patch. In general LGTM, but want to ask if > it is needed to mention in help message of the new --update-result > option that the new result file will be in the new result style, I > mean the following difference, please check: > mv replication/misc.result replication/misc.saved > ./test-run.py replication/misc.test.lua --update-result > diff replication/misc.result replication/misc.saved Thanks for review! I suppose it's not a problem when we will have an option to update .result files. > On Tue, May 19, 2020 at 01:25:46PM +0300, sergeyb@tarantool.org wrote: > > From: Sergey Bronnikov <sergeyb@tarantool.org> > > > > New option covers two cases: > > - 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 behaviour. With option --update-result test-run.py > > will do it automatically. > > - in case of abcense reference result file test-run.py forced to create > > such file in a source directory and set test status "new". This patch > > changes behaviour. With option --update-result test status is "new" > > and result file is created, without option test status is "fail" and > > result file is not created. > > > > Fixes https://github.com/tarantool/tarantool/issues/4654 > > Fixes https://github.com/tarantool/tarantool/issues/4258 > > Closes #194 <snipped> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output 2020-05-19 10:25 [Tarantool-patches] [PATCH v2] Add option to update file with reference output sergeyb 2020-05-20 8:03 ` Alexander V. Tikhonov @ 2020-06-03 17:27 ` Alexander Turenko 2020-06-04 13:07 ` Sergey Bronnikov 1 sibling, 1 reply; 7+ messages in thread From: Alexander Turenko @ 2020-06-03 17:27 UTC (permalink / raw) To: sergeyb; +Cc: o.piskunov, tarantool-patches 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output 2020-06-03 17:27 ` Alexander Turenko @ 2020-06-04 13:07 ` Sergey Bronnikov 2020-06-08 9:19 ` Alexander Turenko 0 siblings, 1 reply; 7+ messages in thread From: Sergey Bronnikov @ 2020-06-04 13:07 UTC (permalink / raw) To: Alexander Turenko; +Cc: o.piskunov, tarantool-patches 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@ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output 2020-06-04 13:07 ` Sergey Bronnikov @ 2020-06-08 9:19 ` Alexander Turenko 2020-06-08 15:31 ` Sergey Bronnikov 0 siblings, 1 reply; 7+ messages in thread From: Alexander Turenko @ 2020-06-08 9:19 UTC (permalink / raw) To: Sergey Bronnikov; +Cc: o.piskunov, tarantool-patches Sergey, please squash fixup commits before sending a next version of a patchset to review. Please, put flake8 configuration update first: so each commit will pass CI. The 'new' status disappears in the new patchset version. Can you look at this again? It looks as unintentional change. > - elif not self.is_crash_reported and not self.is_equal_result: > + elif (not self.is_crash_reported and not self.is_equal_result): Please, avoid unneeded diffs. WBR, Alexander Turenko. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output 2020-06-08 9:19 ` Alexander Turenko @ 2020-06-08 15:31 ` Sergey Bronnikov 0 siblings, 0 replies; 7+ messages in thread From: Sergey Bronnikov @ 2020-06-08 15:31 UTC (permalink / raw) To: Alexander Turenko; +Cc: o.piskunov, tarantool-patches Hi, Alexander, I send a new version of patches with fixes after review. On 12:19 Mon 08 Jun , Alexander Turenko wrote: > Sergey, please squash fixup commits before sending a next version of a > patchset to review. > > Please, put flake8 configuration update first: so each commit will pass > CI. put it first on updated patchset > The 'new' status disappears in the new patchset version. Can you look at > this again? It looks as unintentional change. Yeah, my bad. Fixed it. > > - elif not self.is_crash_reported and not self.is_equal_result: > > + elif (not self.is_crash_reported and not self.is_equal_result): > > Please, avoid unneeded diffs. reverted change > WBR, Alexander Turenko. -- sergeyb@ ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-08 15:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-19 10:25 [Tarantool-patches] [PATCH v2] Add option to update file with reference output sergeyb 2020-05-20 8:03 ` Alexander V. Tikhonov 2020-05-20 8:23 ` Sergey Bronnikov 2020-06-03 17:27 ` Alexander Turenko 2020-06-04 13:07 ` Sergey Bronnikov 2020-06-08 9:19 ` Alexander Turenko 2020-06-08 15:31 ` Sergey Bronnikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox