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

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

[Sergey Bronnikov]

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

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",
+                dest="update_reference_output",
+                action="store_true",
+                default=False,
+                help="""Update file with reference output (.reject) in case of fail
+                and set status pass. 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 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
             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'
             color_stdout("[ fail ]\n", schema='test_fail')
 
             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:
                 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:
                 self.print_unidiff()
                 server.print_log(15)
                 where = ": wrong test output"
-- 
2.23.0


-- 
sergeyb@

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

[Cyrill Gorcunov]

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
> 
> GitHub branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4654-update-ref-output
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>

This is a really wanted feature. Thanks a huge, Sergey!

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

[Sergey Bronnikov]

added Alexander Turenko and Alexander Tikhonov

On 11:01 Tue 24 Mar , 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
> 
> 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",
> +                dest="update_reference_output",
> +                action="store_true",
> +                default=False,
> +                help="""Update file with reference output (.reject) in case of fail
> +                and set status pass. 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 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
>              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'
>              color_stdout("[ fail ]\n", schema='test_fail')
>  
>              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:
>                  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:
>                  self.print_unidiff()
>                  server.print_log(15)
>                  where = ": wrong test output"
> -- 
> 2.23.0
> 
> 
> -- 
> sergeyb@

-- 
sergeyb@

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

[Sergey Bronnikov]

On 11:01 Tue 24 Mar , 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
> 
> 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",
> +                dest="update_reference_output",
> +                action="store_true",
> +                default=False,
> +                help="""Update file with reference output (.reject) in case of fail
> +                and set status pass. 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 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
>              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'
>              color_stdout("[ fail ]\n", schema='test_fail')
>  
>              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:
>                  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:
>                  self.print_unidiff()
>                  server.print_log(15)
>                  where = ": wrong test output"
> -- 
> 2.23.0
> 
> 
> -- 
> sergeyb@

-- 
sergeyb@

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

[Alexander Tikhonov]

[-- Attachment #1: Type: text/plain, Size: 3866 bytes --]


Hi Sergey, thank you for the patch, LGTM, just minor corrections in comments
behaviout → behaviour
automagically → automaically

  
>Вторник, 24 марта 2020, 11:01 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>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
>
>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",
>+ dest="update_reference_output",
>+ action="store_true",
>+ default=False,
>+ help="""Update file with reference output (.reject) in case of fail
>+ and set status pass. 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 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
>             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'
>             color_stdout("[ fail ]\n", schema='test_fail')
> 
>             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:
>                 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:
>                 self.print_unidiff()
>                 server.print_log(15)
>                 where = ": wrong test output"
>--
>2.23.0
>
>
>--
>sergeyb@ 
 
 
--
Alexander Tikhonov
 

[-- Attachment #2: Type: text/html, Size: 6280 bytes --]

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

[Sergey Bronnikov]

Thanks for review!
I have updated typos in the branch.

On 17:34 Mon 30 Mar , Alexander Tikhonov wrote:
> 
> Hi Sergey, thank you for the patch, LGTM, just minor corrections in comments
> behaviout → behaviour
> automagically → automaically
> 
>   
> >Вторник, 24 марта 2020, 11:01 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> > 
> >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
> >
> >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",
> >+ dest="update_reference_output",
> >+ action="store_true",
> >+ default=False,
> >+ help="""Update file with reference output (.reject) in case of fail
> >+ and set status pass. 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 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
> >             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'
> >             color_stdout("[ fail ]\n", schema='test_fail')
> > 
> >             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:
> >                 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:
> >                 self.print_unidiff()
> >                 server.print_log(15)
> >                 where = ": wrong test output"
> >--
> >2.23.0
> >
> >
> >--
> >sergeyb@ 
>  
>  
> --
> Alexander Tikhonov
>  

-- 
sergeyb@

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

[Oleg Piskunov]

[-- Attachment #1: Type: text/plain, Size: 4439 bytes --]


LGTM.
  
>Понедельник, 30 марта 2020, 17:45 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>Thanks for review!
>I have updated typos in the branch.
>
>On 17:34 Mon 30 Mar , Alexander Tikhonov wrote:
>>
>> Hi Sergey, thank you for the patch, LGTM, just minor corrections in comments
>> behaviout → behaviour
>> automagically → automaically
>>
>>  
>> >Вторник, 24 марта 2020, 11:01 +03:00 от Sergey Bronnikov < sergeyb@tarantool.org >:
>> > 
>> >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
>> >
>> >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",
>> >+ dest="update_reference_output",
>> >+ action="store_true",
>> >+ default=False,
>> >+ help="""Update file with reference output (.reject) in case of fail
>> >+ and set status pass. 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 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
>> >             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'
>> >             color_stdout("[ fail ]\n", schema='test_fail')
>> > 
>> >             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:
>> >                 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:
>> >                 self.print_unidiff()
>> >                 server.print_log(15)
>> >                 where = ": wrong test output"
>> >--
>> >2.23.0
>> >
>> >
>> >--
>> >sergeyb@
>>  
>>  
>> --
>> Alexander Tikhonov
>>  
>--
>sergeyb@ 
 
 
--
Oleg Piskunov
 

[-- Attachment #2: Type: text/html, Size: 7568 bytes --]

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

[Alexander Turenko]

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.

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.

> 
> 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.

> +                dest="update_reference_output",
> +                action="store_true",
> +                default=False,
> +                help="""Update file with reference output (.reject) in case of fail

Typo: .reject -> .result.

> +                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.

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).

>          # 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.

>              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.

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.)

>              color_stdout("[ fail ]\n", schema='test_fail')

It will show '[ fail ]' even when a result fill will be updated?

>  
>              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.

>                  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).

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

[Sergey Bronnikov]

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@