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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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