Re: [PATCH v7 09/11] iotests: add testrunner.py

[Eric Blake <eblake@redhat.com>]

On 1/16/21 7:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add TestRunner class, which will run tests in a new python iotests
> running framework.
> 
> There are some differences with current ./check behavior, most
> significant are:
> - Consider all tests self-executable, just run them, don't run python
>   by hand.
> - Elapsed time is cached in json file
> - Elapsed time precision increased a bit
> - use python difflib instead of "diff -w", to ignore spaces at line
>   ends strip lines by hand. Do not ignore other spaces.

Awkward wording.  Maybe:

Instead of using "diff -w" which ignores all whitespace differences,
manually strip whitespace at line end then use python difflib, which no
longer ignores spacing mid-line

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/testrunner.py | 344 +++++++++++++++++++++++++++++++
>  1 file changed, 344 insertions(+)
>  create mode 100644 tests/qemu-iotests/testrunner.py
> 
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> new file mode 100644
> index 0000000000..92722cc68b
> --- /dev/null
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -0,0 +1,344 @@
> +# Class for actual tests running.

for actually running tests

again, should this file be 755 with #! python?

> +
> +def file_diff(file1: str, file2: str) -> List[str]:
> +    with open(file1) as f1, open(file2) as f2:
> +        # We want to ignore spaces at line ends. There are a lot of mess about
> +        # it in iotests.
> +        # TODO: fix all tests to not produce extra spaces, fix all .out files
> +        # and use strict diff here!
> +        seq1 = [line.rstrip() for line in f1]
> +        seq2 = [line.rstrip() for line in f2]
> +        return list(difflib.unified_diff(seq1, seq2, file1, file2))

Offhand, do you have the list of tests where (actual/expected) output
has trailing whitespace and would fail without the .rstrip()?

> +
> +
> +# We want to save current tty settings during test run,
> +# since an aborting qemu call may leave things screwed up.
> +@contextmanager
> +def savetty() -> Iterator[None]:
> +    isterm = sys.stdin.isatty()
> +    if isterm:
> +        fd = sys.stdin.fileno()
> +        attr = termios.tcgetattr(0)

Isn't fd always going to be 0?  It looks odd to hard-code zero in the
very next line; either we should s/0/fd/ here, or...

> +
> +    try:
> +        yield
> +    finally:
> +        if isterm:
> +            termios.tcsetattr(fd, termios.TCSADRAIN, attr)

... s/fd/0/ here and drop fd altogether.

> +
> +
> +class LastElapsedTime(AbstractContextManager['LastElapsedTime']):
> +    """ Cache for elapsed time for tests, to show it during new test run
> +
> +    Use get() in any time. But, if use update you should then call save(),
> +    or use update() inside with-block.

Grammar is hard, maybe:

It is safe to use get() at any time.  To use update(), you must either
use a with-block or first use save().


> +    def test_print_one_line(self, test: str, starttime: str,

> +
> +        if status == 'pass':
> +            col = '\033[32m'
> +        elif status == 'fail':
> +            col = '\033[1m\033[31m'
> +        elif status == 'not run':
> +            col = '\033[33m'

This hard-codes the use of ANSI escape sequences without first checking
that we are writing to a terminal.  Is that wise?  Should we have this
be tunable by a tri-state command-line option, similar to ls --color?
(--color=auto is default, and bases decision on istty(), --color=off
turns color off even for a terminal, --color=on uses color even when
outputting to a pipe, which can be useful depending on the other end of
the pipeline...)


> +        with f_test.open() as f:
> +            try:
> +                if f.readline() == '#!/usr/bin/env python3':
> +                    args.insert(0, self.env.python)
> +            except UnicodeDecodeError:  # binary test? for future.
> +                pass

Is pass the right action here?  Or will it silently skip a test file
with encoding errors?

Again, I'm not comfortable enough to give a full review of the python,
but it looks fairly similar to the existing shell code, and with the
series applied, things still work.  So I can offer

Tested-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org