Re: [Tarantool-patches] [PATCH v2] Add option to update file with reference output

[Sergey Bronnikov <sergeyb@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@