[Tarantool-patches] [PATCH v2] Add option to update file with reference output
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jun 4 16:07:03 MSK 2020
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@
More information about the Tarantool-patches
mailing list