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

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

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.