qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v7 09/11] iotests: add testrunner.py
Date: Thu, 21 Jan 2021 20:17:47 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]