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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

21.01.2021 20:02, Eric Blake wrote:
> 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?

In my latest considerations - no it shouldn't.. We still make implement __main__ things later if needed.

> 
>> +
>> +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()?

No.. But it's simple to make it

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

agree that's strange.

> 
>> +
>> +    try:
>> +        yield
>> +    finally:
>> +        if isterm:
>> +            termios.tcsetattr(fd, termios.TCSADRAIN, attr)
> 
> ... s/fd/0/ here and drop fd altogether.

Either way is OK for me, I think, I'll do s/0/fd/

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

OK, thanks

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

Hmm, yes. It's preexisting in old bash check script I think. I can add a separate patch for it

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

No, we'll not skip it. Here we just failed to recognize python test, so, it will be executed as self-executable. So, if there are real problems, we'll see them when try to execute the file.

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


Thanks a lot for testing and reviewing so many my patches!!

-- 
Best regards,
Vladimir