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

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

[sergeyb]

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

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

[Alexander V. Tikhonov]

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
> 

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

[Sergey Bronnikov]

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>

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

[Alexander Turenko]

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.

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

[Sergey Bronnikov]

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@

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

[Alexander Turenko]

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.

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

[Sergey Bronnikov]

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@