* [PATCH v10 00/14] iotests: use python logging
@ 2020-03-31 0:00 John Snow
2020-03-31 0:00 ` [PATCH v10 01/14] iotests: do a light delinting John Snow
` (15 more replies)
0 siblings, 16 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.
It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.
Also, I got lost and accidentally delinted iotests while I was here.
Sorry about that. By version 9, it's now the overwhelming focus of
this series. No good deed, etc.
V10:
001/14:[0004] [FC] 'iotests: do a light delinting'
002/14:[----] [--] 'iotests: don't use 'format' for drive_add'
003/14:[----] [--] 'iotests: ignore import warnings from pylint'
004/14:[----] [--] 'iotests: replace mutable list default args'
005/14:[0006] [FC] 'iotests: add pylintrc file'
006/14:[----] [--] 'iotests: alphabetize standard imports'
007/14:[----] [--] 'iotests: drop pre-Python 3.4 compatibility code'
008/14:[----] [--] 'iotests: touch up log function signature'
009/14:[----] [-C] 'iotests: limit line length to 79 chars'
010/14:[0009] [FC] 'iotests: add hmp helper with logging'
011/14:[0019] [FC] 'iotests: add script_initialize'
012/14:[----] [--] 'iotest 258: use script_main'
013/14:[0013] [FC] 'iotests: Mark verify functions as private'
014/14:[0001] [FC] 'iotests: use python logging for iotests.log()'
001: replace "atom" name with "item". Kept RBs.
005: Alphabetized excluded warnings list. Kept RBs.
Kevin's comments addressed by using pylint >= 2.2.0
009: Added Max's RB.
Updated commit message based on Max's response
Kevin's comments addressed by mypy >= 0.620
010: Fixed type hints (Kevin)
011: Replace 'Collection' with 'Sequence' to work around pylint/python 3.6
013: Update type signatures of _verify functions (Kevin)
014: Minor whitespace changes as the fault handler gets shuffled around.
V9:
006: New.
007: Split from old patch.
008: Split from old patch; enhanced a little to justify its own patch.
010: New, pulled in from bitmap-populate series. Helps line length.
011: Reflow columns for long `typing` import list. (Kept RB.)
014: New blank line. (Kept RB.)
V8:
- Split out the little drop of Python 3.4 code. (Phil)
- Change line continuation styles (QEMU Memorial Choir)
- Rebase changes; remove use_log from more places, adjust test output.
V7:
- All delinting patches are now entirely front-loaded.
- Redid delinting to avoid "correcting" no-else-return statements.
- Moved more mutable list corrections into patch 4, to make it standalone.
- Moved pylintrc up to patch 5. Disabled no-else-return.
- Added patch 6 to require line length checks.
(Some python 3.4 compatibility code is removed as a consequence.)
- Patch 7 changes slightly as a result of patch 4 changes.
- Added some logging explainer into patch 10.
(Patch changes slightly because of patch 6.)
V6:
- It's been so long since V5, let's just look at it anew.
John Snow (14):
iotests: do a light delinting
iotests: don't use 'format' for drive_add
iotests: ignore import warnings from pylint
iotests: replace mutable list default args
iotests: add pylintrc file
iotests: alphabetize standard imports
iotests: drop pre-Python 3.4 compatibility code
iotests: touch up log function signature
iotests: limit line length to 79 chars
iotests: add hmp helper with logging
iotests: add script_initialize
iotest 258: use script_main
iotests: Mark verify functions as private
iotests: use python logging for iotests.log()
tests/qemu-iotests/030 | 4 +-
tests/qemu-iotests/055 | 3 +-
tests/qemu-iotests/149 | 3 +-
tests/qemu-iotests/155 | 2 +-
tests/qemu-iotests/194 | 4 +-
tests/qemu-iotests/202 | 4 +-
tests/qemu-iotests/203 | 4 +-
tests/qemu-iotests/206 | 2 +-
tests/qemu-iotests/207 | 6 +-
tests/qemu-iotests/208 | 2 +-
tests/qemu-iotests/209 | 2 +-
tests/qemu-iotests/210 | 6 +-
tests/qemu-iotests/211 | 6 +-
tests/qemu-iotests/212 | 6 +-
tests/qemu-iotests/213 | 6 +-
tests/qemu-iotests/216 | 4 +-
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/219 | 2 +-
tests/qemu-iotests/222 | 7 +-
tests/qemu-iotests/224 | 4 +-
tests/qemu-iotests/228 | 6 +-
tests/qemu-iotests/234 | 4 +-
tests/qemu-iotests/235 | 4 +-
tests/qemu-iotests/236 | 2 +-
tests/qemu-iotests/237 | 2 +-
tests/qemu-iotests/238 | 2 +
tests/qemu-iotests/242 | 2 +-
tests/qemu-iotests/245 | 1 +
tests/qemu-iotests/245.out | 24 +--
tests/qemu-iotests/246 | 2 +-
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/254 | 2 +-
tests/qemu-iotests/255 | 2 +-
tests/qemu-iotests/256 | 2 +-
tests/qemu-iotests/258 | 10 +-
tests/qemu-iotests/260 | 4 +-
tests/qemu-iotests/262 | 4 +-
tests/qemu-iotests/264 | 4 +-
tests/qemu-iotests/277 | 2 +
tests/qemu-iotests/280 | 8 +-
tests/qemu-iotests/283 | 4 +-
tests/qemu-iotests/iotests.py | 366 ++++++++++++++++++++--------------
tests/qemu-iotests/pylintrc | 26 +++
43 files changed, 343 insertions(+), 221 deletions(-)
create mode 100644 tests/qemu-iotests/pylintrc
--
2.21.1
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v10 01/14] iotests: do a light delinting
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 02/14] iotests: don't use 'format' for drive_add John Snow
` (14 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
This doesn't fix everything in here, but it does help clean up the
pylint report considerably.
This should be 100% style changes only; the intent is to make pylint
more useful by working on establishing a baseline for iotests that we
can gate against in the future.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 83 ++++++++++++++++++-----------------
1 file changed, 43 insertions(+), 40 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bc4934cd2..0641bbbc1f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,11 +16,9 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
-import errno
import os
import re
import subprocess
-import string
import unittest
import sys
import struct
@@ -35,7 +33,7 @@
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu import qtest
-assert sys.version_info >= (3,6)
+assert sys.version_info >= (3, 6)
faulthandler.enable()
@@ -141,11 +139,11 @@ def qemu_img_log(*args):
return result
def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
- args = [ 'info' ]
+ args = ['info']
if imgopts:
args.append('--image-opts')
else:
- args += [ '-f', imgfmt ]
+ args += ['-f', imgfmt]
args += extra_args
args.append(filename)
@@ -224,7 +222,7 @@ def cmd(self, cmd):
# quit command is in close(), '\n' is added automatically
assert '\n' not in cmd
cmd = cmd.strip()
- assert cmd != 'q' and cmd != 'quit'
+ assert cmd not in ('q', 'quit')
self._p.stdin.write(cmd + '\n')
self._p.stdin.flush()
return self._read_output()
@@ -246,10 +244,8 @@ def qemu_nbd_early_pipe(*args):
sys.stderr.write('qemu-nbd received signal %i: %s\n' %
(-exitcode,
' '.join(qemu_nbd_args + ['--fork'] + list(args))))
- if exitcode == 0:
- return exitcode, ''
- else:
- return exitcode, subp.communicate()[0]
+
+ return exitcode, subp.communicate()[0] if exitcode else ''
def qemu_nbd_popen(*args):
'''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -313,7 +309,7 @@ def filter_qmp(qmsg, filter_fn):
items = qmsg.items()
for k, v in items:
- if isinstance(v, list) or isinstance(v, dict):
+ if isinstance(v, (dict, list)):
qmsg[k] = filter_qmp(v, filter_fn)
else:
qmsg[k] = filter_fn(k, v)
@@ -324,7 +320,7 @@ def filter_testfiles(msg):
return msg.replace(prefix, 'TEST_DIR/PID-')
def filter_qmp_testfiles(qmsg):
- def _filter(key, value):
+ def _filter(_key, value):
if is_str(value):
return filter_testfiles(value)
return value
@@ -350,7 +346,7 @@ def filter_imgfmt(msg):
return msg.replace(imgfmt, 'IMGFMT')
def filter_qmp_imgfmt(qmsg):
- def _filter(key, value):
+ def _filter(_key, value):
if is_str(value):
return filter_imgfmt(value)
return value
@@ -361,7 +357,7 @@ def log(msg, filters=[], indent=None):
If indent is provided, JSON serializable messages are pretty-printed.'''
for flt in filters:
msg = flt(msg)
- if isinstance(msg, dict) or isinstance(msg, list):
+ if isinstance(msg, (dict, list)):
# Python < 3.4 needs to know not to add whitespace when pretty-printing:
separators = (', ', ': ') if indent is None else (',', ': ')
# Don't sort if it's already sorted
@@ -372,14 +368,14 @@ def log(msg, filters=[], indent=None):
print(msg)
class Timeout:
- def __init__(self, seconds, errmsg = "Timeout"):
+ def __init__(self, seconds, errmsg="Timeout"):
self.seconds = seconds
self.errmsg = errmsg
def __enter__(self):
signal.signal(signal.SIGALRM, self.timeout)
signal.setitimer(signal.ITIMER_REAL, self.seconds)
return self
- def __exit__(self, type, value, traceback):
+ def __exit__(self, exc_type, value, traceback):
signal.setitimer(signal.ITIMER_REAL, 0)
return False
def timeout(self, signum, frame):
@@ -388,7 +384,7 @@ def timeout(self, signum, frame):
def file_pattern(name):
return "{0}-{1}".format(os.getpid(), name)
-class FilePaths(object):
+class FilePaths:
"""
FilePaths is an auto-generated filename that cleans itself up.
@@ -535,11 +531,11 @@ def pause_drive(self, drive, event=None):
self.pause_drive(drive, "write_aio")
return
self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive))
+ command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive))
def resume_drive(self, drive):
self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive))
+ command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive))
def hmp_qemu_io(self, drive, cmd):
'''Write to a given drive using an HMP command'''
@@ -550,8 +546,8 @@ def flatten_qmp_object(self, obj, output=None, basestr=''):
if output is None:
output = dict()
if isinstance(obj, list):
- for i in range(len(obj)):
- self.flatten_qmp_object(obj[i], output, basestr + str(i) + '.')
+ for i, item in enumerate(obj):
+ self.flatten_qmp_object(item, output, basestr + str(i) + '.')
elif isinstance(obj, dict):
for key in obj:
self.flatten_qmp_object(obj[key], output, basestr + key + '.')
@@ -709,9 +705,7 @@ def get_bitmap(self, node_name, bitmap_name, recording=None, bitmaps=None):
for bitmap in bitmaps[node_name]:
if bitmap.get('name', '') == bitmap_name:
- if recording is None:
- return bitmap
- elif bitmap.get('recording') == recording:
+ if recording is None or bitmap.get('recording') == recording:
return bitmap
return None
@@ -762,12 +756,13 @@ def assert_block_path(self, root, path, expected_node, graph=None):
assert node is not None, 'Cannot follow path %s%s' % (root, path)
try:
- node_id = next(edge['child'] for edge in graph['edges'] \
- if edge['parent'] == node['id'] and
- edge['name'] == child_name)
+ node_id = next(edge['child'] for edge in graph['edges']
+ if (edge['parent'] == node['id'] and
+ edge['name'] == child_name))
+
+ node = next(node for node in graph['nodes']
+ if node['id'] == node_id)
- node = next(node for node in graph['nodes'] \
- if node['id'] == node_id)
except StopIteration:
node = None
@@ -785,6 +780,12 @@ def assert_block_path(self, root, path, expected_node, graph=None):
class QMPTestCase(unittest.TestCase):
'''Abstract base class for QMP test cases'''
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ # Many users of this class set a VM property we rely on heavily
+ # in the methods below.
+ self.vm = None
+
def dictpath(self, d, path):
'''Traverse a path in a nested dict'''
for component in path.split('/'):
@@ -830,7 +831,7 @@ def assert_qmp(self, d, path, value):
else:
self.assertEqual(result, value,
'"%s" is "%s", expected "%s"'
- % (path, str(result), str(value)))
+ % (path, str(result), str(value)))
def assert_no_active_block_jobs(self):
result = self.vm.qmp('query-block-jobs')
@@ -840,15 +841,15 @@ def assert_has_block_node(self, node_name=None, file_name=None):
"""Issue a query-named-block-nodes and assert node_name and/or
file_name is present in the result"""
def check_equal_or_none(a, b):
- return a == None or b == None or a == b
+ return a is None or b is None or a == b
assert node_name or file_name
result = self.vm.qmp('query-named-block-nodes')
for x in result["return"]:
if check_equal_or_none(x.get("node-name"), node_name) and \
check_equal_or_none(x.get("file"), file_name):
return
- self.assertTrue(False, "Cannot find %s %s in result:\n%s" % \
- (node_name, file_name, result))
+ self.fail("Cannot find %s %s in result:\n%s" %
+ (node_name, file_name, result))
def assert_json_filename_equal(self, json_filename, reference):
'''Asserts that the given filename is a json: filename and that its
@@ -897,13 +898,13 @@ def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0,
self.assert_qmp(event, 'data/error', error)
self.assert_no_active_block_jobs()
return event
- elif event['event'] == 'JOB_STATUS_CHANGE':
+ if event['event'] == 'JOB_STATUS_CHANGE':
self.assert_qmp(event, 'data/id', drive)
def wait_ready(self, drive='drive0'):
- '''Wait until a block job BLOCK_JOB_READY event'''
- f = {'data': {'type': 'mirror', 'device': drive } }
- event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
+ """Wait until a BLOCK_JOB_READY event, and return the event."""
+ f = {'data': {'type': 'mirror', 'device': drive}}
+ return self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
def wait_ready_and_cancel(self, drive='drive0'):
self.wait_ready(drive=drive)
@@ -932,7 +933,7 @@ def pause_wait(self, job_id='job0'):
for job in result['return']:
if job['device'] == job_id:
found = True
- if job['paused'] == True and job['busy'] == False:
+ if job['paused'] and not job['busy']:
return job
break
assert found
@@ -1029,8 +1030,8 @@ def qemu_pipe(*args):
universal_newlines=True)
exitcode = subp.wait()
if exitcode < 0:
- sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode,
- ' '.join(args)))
+ sys.stderr.write('qemu received signal %i: %s\n' %
+ (-exitcode, ' '.join(args)))
return subp.communicate()[0]
def supported_formats(read_only=False):
@@ -1062,6 +1063,7 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs):
if usf_list:
test_case.case_skip('{}: formats {} are not whitelisted'.format(
test_case, usf_list))
+ return None
else:
return func(test_case, *args, **kwargs)
return func_wrapper
@@ -1073,6 +1075,7 @@ def skip_if_user_is_root(func):
def func_wrapper(*args, **kwargs):
if os.getuid() == 0:
case_notrun('{}: cannot be run as root'.format(args[0]))
+ return None
else:
return func(*args, **kwargs)
return func_wrapper
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 02/14] iotests: don't use 'format' for drive_add
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
2020-03-31 0:00 ` [PATCH v10 01/14] iotests: do a light delinting John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 03/14] iotests: ignore import warnings from pylint John Snow
` (13 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
It shadows (with a different type) the built-in format.
Use something else.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/055 | 3 ++-
tests/qemu-iotests/iotests.py | 6 +++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..4175fff5e4 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -469,7 +469,8 @@ class TestDriveCompression(iotests.QMPTestCase):
qemu_img('create', '-f', fmt, blockdev_target_img,
str(TestDriveCompression.image_len), *args)
if attach_target:
- self.vm.add_drive(blockdev_target_img, format=fmt, interface="none")
+ self.vm.add_drive(blockdev_target_img,
+ img_format=fmt, interface="none")
self.vm.launch()
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0641bbbc1f..a026d3343e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -485,21 +485,21 @@ def add_drive_raw(self, opts):
self._args.append(opts)
return self
- def add_drive(self, path, opts='', interface='virtio', format=imgfmt):
+ def add_drive(self, path, opts='', interface='virtio', img_format=imgfmt):
'''Add a virtio-blk drive to the VM'''
options = ['if=%s' % interface,
'id=drive%d' % self._num_drives]
if path is not None:
options.append('file=%s' % path)
- options.append('format=%s' % format)
+ options.append('format=%s' % img_format)
options.append('cache=%s' % cachemode)
options.append('aio=%s' % aiomode)
if opts:
options.append(opts)
- if format == 'luks' and 'key-secret' not in opts:
+ if img_format == 'luks' and 'key-secret' not in opts:
# default luks support
if luks_default_secret_object not in self._args:
self.add_object(luks_default_secret_object)
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 03/14] iotests: ignore import warnings from pylint
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
2020-03-31 0:00 ` [PATCH v10 01/14] iotests: do a light delinting John Snow
2020-03-31 0:00 ` [PATCH v10 02/14] iotests: don't use 'format' for drive_add John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 04/14] iotests: replace mutable list default args John Snow
` (12 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
The right way to solve this is to come up with a virtual environment
infrastructure that sets all the paths correctly, and/or to create
installable python modules that can be imported normally.
That's hard, so just silence this error for now.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a026d3343e..4d848339a5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
from collections import OrderedDict
import faulthandler
+# pylint: disable=import-error, wrong-import-position
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu import qtest
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 04/14] iotests: replace mutable list default args
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (2 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 03/14] iotests: ignore import warnings from pylint John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 05/14] iotests: add pylintrc file John Snow
` (11 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
It's bad hygiene: if we modify this list, it will be modified across all
invocations.
(Remaining bad usages are fixed in a subsequent patch which changes the
function signature anyway.)
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4d848339a5..51f84475d9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -139,7 +139,7 @@ def qemu_img_log(*args):
log(result, filters=[filter_testfiles])
return result
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
+def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
args = ['info']
if imgopts:
args.append('--image-opts')
@@ -353,7 +353,7 @@ def _filter(_key, value):
return value
return filter_qmp(qmsg, _filter)
-def log(msg, filters=[], indent=None):
+def log(msg, filters=(), indent=None):
'''Logs either a string message or a JSON serializable message (like QMP).
If indent is provided, JSON serializable messages are pretty-printed.'''
for flt in filters:
@@ -569,7 +569,7 @@ def get_qmp_events_filtered(self, wait=60.0):
result.append(filter_qmp_event(ev))
return result
- def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
+ def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
full_cmd = OrderedDict((
("execute", cmd),
("arguments", ordered_qmp(kwargs))
@@ -973,7 +973,7 @@ def case_notrun(reason):
open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
' [case not run] ' + reason + '\n')
-def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
+def verify_image_format(supported_fmts=(), unsupported_fmts=()):
assert not (supported_fmts and unsupported_fmts)
if 'generic' in supported_fmts and \
@@ -987,7 +987,7 @@ def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
if not_sup or (imgfmt in unsupported_fmts):
notrun('not suitable for this image format: %s' % imgfmt)
-def verify_protocol(supported=[], unsupported=[]):
+def verify_protocol(supported=(), unsupported=()):
assert not (supported and unsupported)
if 'generic' in supported:
@@ -1006,11 +1006,11 @@ def verify_platform(supported=None, unsupported=None):
if not any((sys.platform.startswith(x) for x in supported)):
notrun('not suitable for this OS: %s' % sys.platform)
-def verify_cache_mode(supported_cache_modes=[]):
+def verify_cache_mode(supported_cache_modes=()):
if supported_cache_modes and (cachemode not in supported_cache_modes):
notrun('not suitable for this cache mode: %s' % cachemode)
-def verify_aio_mode(supported_aio_modes=[]):
+def verify_aio_mode(supported_aio_modes=()):
if supported_aio_modes and (aiomode not in supported_aio_modes):
notrun('not suitable for this aio mode: %s' % aiomode)
@@ -1050,7 +1050,7 @@ def supported_formats(read_only=False):
return supported_formats.formats[read_only]
-def skip_if_unsupported(required_formats=[], read_only=False):
+def skip_if_unsupported(required_formats=(), read_only=False):
'''Skip Test Decorator
Runs the test if all the required formats are whitelisted'''
def skip_test_decorator(func):
@@ -1101,11 +1101,11 @@ def execute_unittest(output, verbosity, debug):
sys.stderr.write(out)
def execute_test(test_function=None,
- supported_fmts=[],
+ supported_fmts=(),
supported_platforms=None,
- supported_cache_modes=[], supported_aio_modes={},
- unsupported_fmts=[], supported_protocols=[],
- unsupported_protocols=[]):
+ supported_cache_modes=(), supported_aio_modes=(),
+ unsupported_fmts=(), supported_protocols=(),
+ unsupported_protocols=()):
"""Run either unittest or script-style tests."""
# We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 05/14] iotests: add pylintrc file
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (3 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 04/14] iotests: replace mutable list default args John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 06/14] iotests: alphabetize standard imports John Snow
` (10 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
This allows others to get repeatable results with pylint. If you run
`pylint iotests.py`, you should see a 100% pass.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/pylintrc | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 tests/qemu-iotests/pylintrc
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
new file mode 100644
index 0000000000..daec2c4942
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,22 @@
+[MESSAGES CONTROL]
+
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=invalid-name,
+ no-else-return,
+ too-few-public-methods,
+ too-many-arguments,
+ too-many-branches,
+ too-many-lines,
+ too-many-locals,
+ too-many-public-methods,
+ # These are temporary, and should be removed:
+ line-too-long,
+ missing-docstring,
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 06/14] iotests: alphabetize standard imports
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (4 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 05/14] iotests: add pylintrc file John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 8:00 ` Philippe Mathieu-Daudé
2020-03-31 0:00 ` [PATCH v10 07/14] iotests: drop pre-Python 3.4 compatibility code John Snow
` (9 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
I had to fix a merge conflict, so do this tiny harmless thing while I'm
here.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 51f84475d9..e6f9f62b2b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,19 +16,19 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#
+import atexit
+from collections import OrderedDict
+import faulthandler
+import io
+import json
+import logging
import os
import re
+import signal
+import struct
import subprocess
-import unittest
import sys
-import struct
-import json
-import signal
-import logging
-import atexit
-import io
-from collections import OrderedDict
-import faulthandler
+import unittest
# pylint: disable=import-error, wrong-import-position
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 07/14] iotests: drop pre-Python 3.4 compatibility code
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (5 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 06/14] iotests: alphabetize standard imports John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 08/14] iotests: touch up log function signature John Snow
` (8 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
We no longer need to accommodate <3.4, drop this code.
(The lines were > 79 chars and it stood out.)
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e6f9f62b2b..010bca526c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -359,12 +359,9 @@ def log(msg, filters=(), indent=None):
for flt in filters:
msg = flt(msg)
if isinstance(msg, (dict, list)):
- # Python < 3.4 needs to know not to add whitespace when pretty-printing:
- separators = (', ', ': ') if indent is None else (',', ': ')
# Don't sort if it's already sorted
do_sort = not isinstance(msg, OrderedDict)
- print(json.dumps(msg, sort_keys=do_sort,
- indent=indent, separators=separators))
+ print(json.dumps(msg, sort_keys=do_sort, indent=indent))
else:
print(msg)
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 08/14] iotests: touch up log function signature
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (6 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 07/14] iotests: drop pre-Python 3.4 compatibility code John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 09/14] iotests: limit line length to 79 chars John Snow
` (7 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
Representing nested, recursive data structures in mypy is notoriously
difficult; the best we can reliably do right now is denote the leaf
types as "Any" while describing the general shape of the data.
Regardless, this fully annotates the log() function.
Typing notes:
TypeVar is a Type variable that can optionally be constrained by a
sequence of possible types. This variable is bound to a specific type
per-invocation, like a Generic.
log() behaves as log<Msg>() now, where the incoming type informs the
signature it expects for any filter arguments passed in. If Msg is a
str, then filter should take and return a str.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 010bca526c..e2d3e89574 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@
import struct
import subprocess
import sys
+from typing import (Any, Callable, Dict, Iterable, List, Optional, TypeVar)
import unittest
# pylint: disable=import-error, wrong-import-position
@@ -353,9 +354,16 @@ def _filter(_key, value):
return value
return filter_qmp(qmsg, _filter)
-def log(msg, filters=(), indent=None):
- '''Logs either a string message or a JSON serializable message (like QMP).
- If indent is provided, JSON serializable messages are pretty-printed.'''
+
+Msg = TypeVar('Msg', Dict[str, Any], List[Any], str)
+
+def log(msg: Msg,
+ filters: Iterable[Callable[[Msg], Msg]] = (),
+ indent: Optional[int] = None) -> None:
+ """
+ Logs either a string message or a JSON serializable message (like QMP).
+ If indent is provided, JSON serializable messages are pretty-printed.
+ """
for flt in filters:
msg = flt(msg)
if isinstance(msg, (dict, list)):
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 09/14] iotests: limit line length to 79 chars
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (7 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 08/14] iotests: touch up log function signature John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 10/14] iotests: add hmp helper with logging John Snow
` (6 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------
tests/qemu-iotests/pylintrc | 6 +++-
2 files changed, 47 insertions(+), 23 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e2d3e89574..b08bcb87e1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -80,9 +80,11 @@
def qemu_img(*args):
'''Run qemu-img and return the exit code'''
devnull = open('/dev/null', 'r+')
- exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, stdout=devnull)
+ exitcode = subprocess.call(qemu_img_args + list(args),
+ stdin=devnull, stdout=devnull)
if exitcode < 0:
- sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
+ sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args))))
return exitcode
def ordered_qmp(qmsg, conv_keys=True):
@@ -121,7 +123,8 @@ def qemu_img_verbose(*args):
'''Run qemu-img without suppressing its output and return the exit code'''
exitcode = subprocess.call(qemu_img_args + list(args))
if exitcode < 0:
- sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
+ sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args))))
return exitcode
def qemu_img_pipe(*args):
@@ -132,7 +135,8 @@ def qemu_img_pipe(*args):
universal_newlines=True)
exitcode = subp.wait()
if exitcode < 0:
- sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
+ sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args))))
return subp.communicate()[0]
def qemu_img_log(*args):
@@ -162,7 +166,8 @@ def qemu_io(*args):
universal_newlines=True)
exitcode = subp.wait()
if exitcode < 0:
- sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
+ sys.stderr.write('qemu-io received signal %i: %s\n'
+ % (-exitcode, ' '.join(args)))
return subp.communicate()[0]
def qemu_io_log(*args):
@@ -284,10 +289,13 @@ def filter_test_dir(msg):
def filter_win32(msg):
return win32_re.sub("", msg)
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+ r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+ r"and [0-9\/.inf]* ops\/sec\)")
def filter_qemu_io(msg):
msg = filter_win32(msg)
- return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", msg)
+ return qemu_io_re.sub("X ops; XX:XX:XX.X "
+ "(XXX YYY/sec and XXX ops/sec)", msg)
chown_re = re.compile(r"chown [0-9]+:[0-9]+")
def filter_chown(msg):
@@ -339,7 +347,9 @@ def filter_img_info(output, filename):
line = line.replace(filename, 'TEST_IMG') \
.replace(imgfmt, 'IMGFMT')
line = re.sub('iters: [0-9]+', 'iters: XXX', line)
- line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line)
+ line = re.sub('uuid: [-a-f0-9]+',
+ 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX',
+ line)
line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line)
lines.append(line)
return '\n'.join(lines)
@@ -537,11 +547,13 @@ def pause_drive(self, drive, event=None):
self.pause_drive(drive, "write_aio")
return
self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive))
+ command_line='qemu-io %s "break %s bp_%s"'
+ % (drive, event, drive))
def resume_drive(self, drive):
self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive))
+ command_line='qemu-io %s "remove_break bp_%s"'
+ % (drive, drive))
def hmp_qemu_io(self, drive, cmd):
'''Write to a given drive using an HMP command'''
@@ -801,16 +813,18 @@ def dictpath(self, d, path):
idx = int(idx)
if not isinstance(d, dict) or component not in d:
- self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+ self.fail(f'failed path traversal for "{path}" in "{d}"')
d = d[component]
if m:
if not isinstance(d, list):
- self.fail('path component "%s" in "%s" is not a list in "%s"' % (component, path, str(d)))
+ self.fail(f'path component "{component}" in "{path}" '
+ f'is not a list in "{d}"')
try:
d = d[idx]
except IndexError:
- self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
+ self.fail(f'invalid index "{idx}" in path "{path}" '
+ f'in "{d}"')
return d
def assert_qmp_absent(self, d, path):
@@ -861,10 +875,13 @@ def assert_json_filename_equal(self, json_filename, reference):
'''Asserts that the given filename is a json: filename and that its
content is equal to the given reference object'''
self.assertEqual(json_filename[:5], 'json:')
- self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
- self.vm.flatten_qmp_object(reference))
+ self.assertEqual(
+ self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
+ self.vm.flatten_qmp_object(reference)
+ )
- def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
+ def cancel_and_wait(self, drive='drive0', force=False,
+ resume=False, wait=60.0):
'''Cancel a block job and wait for it to finish, returning the event'''
result = self.vm.qmp('block-job-cancel', device=drive, force=force)
self.assert_qmp(result, 'return', {})
@@ -888,8 +905,8 @@ def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
self.assert_no_active_block_jobs()
return result
- def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0,
- error=None):
+ def wait_until_completed(self, drive='drive0', check_offset=True,
+ wait=60.0, error=None):
'''Wait for a block job to finish, returning the event'''
while True:
for event in self.vm.get_qmp_events(wait=wait):
@@ -1028,8 +1045,11 @@ def verify_quorum():
notrun('quorum support missing')
def qemu_pipe(*args):
- '''Run qemu with an option to print something and exit (e.g. a help option),
- and return its output'''
+ """
+ Run qemu with an option to print something and exit (e.g. a help option).
+
+ :return: QEMU's stdout output.
+ """
args = [qemu_prog] + qemu_opts + list(args)
subp = subprocess.Popen(args, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
@@ -1067,8 +1087,8 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs):
usf_list = list(set(fmts) - set(supported_formats(read_only)))
if usf_list:
- test_case.case_skip('{}: formats {} are not whitelisted'.format(
- test_case, usf_list))
+ msg = f'{test_case}: formats {usf_list} are not whitelisted'
+ test_case.case_skip(msg)
return None
else:
return func(test_case, *args, **kwargs)
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index daec2c4942..5481afe528 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -18,5 +18,9 @@ disable=invalid-name,
too-many-locals,
too-many-public-methods,
# These are temporary, and should be removed:
- line-too-long,
missing-docstring,
+
+[FORMAT]
+
+# Maximum number of characters on a single line.
+max-line-length=79
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (8 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 09/14] iotests: limit line length to 79 chars John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 10:21 ` Max Reitz
2020-03-31 0:00 ` [PATCH v10 11/14] iotests: add script_initialize John Snow
` (5 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
Minor cleanup for HMP functions; helps with line length and consolidates
HMP helpers through one implementation function.
Although we are adding a universal toggle to turn QMP logging on or off,
many existing callers to hmp functions don't expect that output to be
logged, which causes quite a few changes in the test output.
For now, offer a use_log parameter.
Typing notes:
QMPResponse is just an alias for Dict[str, Any]. It holds no special
meanings and it is not a formal subtype of Dict[str, Any]. It is best
thought of as a lexical synonym.
We may well wish to add stricter subtypes in the future for certain
shapes of data that are not formalized as Python objects, at which point
we can simply retire the alias and allow mypy to more strictly check
usages of the name.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b08bcb87e1..dfc753c319 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -37,6 +37,10 @@
assert sys.version_info >= (3, 6)
+# Type Aliases
+QMPResponse = Dict[str, Any]
+
+
faulthandler.enable()
# This will not work if arguments contain spaces but is necessary if we
@@ -540,25 +544,30 @@ def add_incoming(self, addr):
self._args.append(addr)
return self
- def pause_drive(self, drive, event=None):
- '''Pause drive r/w operations'''
+ def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
+ cmd = 'human-monitor-command'
+ kwargs = {'command-line': command_line}
+ if use_log:
+ return self.qmp_log(cmd, **kwargs)
+ else:
+ return self.qmp(cmd, **kwargs)
+
+ def pause_drive(self, drive: str, event: Optional[str] = None) -> None:
+ """Pause drive r/w operations"""
if not event:
self.pause_drive(drive, "read_aio")
self.pause_drive(drive, "write_aio")
return
- self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"'
- % (drive, event, drive))
+ self.hmp(f'qemu-io {drive} "break {event} bp_{drive}"')
- def resume_drive(self, drive):
- self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"'
- % (drive, drive))
+ def resume_drive(self, drive: str) -> None:
+ """Resume drive r/w operations"""
+ self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
- def hmp_qemu_io(self, drive, cmd):
- '''Write to a given drive using an HMP command'''
- return self.qmp('human-monitor-command',
- command_line='qemu-io %s "%s"' % (drive, cmd))
+ def hmp_qemu_io(self, drive: str, cmd: str,
+ use_log: bool = False) -> QMPResponse:
+ """Write to a given drive using an HMP command"""
+ return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)
def flatten_qmp_object(self, obj, output=None, basestr=''):
if output is None:
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 11/14] iotests: add script_initialize
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (9 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 10/14] iotests: add hmp helper with logging John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 12/14] iotest 258: use script_main John Snow
` (4 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
Like script_main, but doesn't require a single point of entry.
Replace all existing initialization sections with this drop-in replacement.
This brings debug support to all existing script-style iotests.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/149 | 3 +-
tests/qemu-iotests/194 | 4 +-
tests/qemu-iotests/202 | 4 +-
tests/qemu-iotests/203 | 4 +-
tests/qemu-iotests/206 | 2 +-
tests/qemu-iotests/207 | 6 ++-
tests/qemu-iotests/208 | 2 +-
tests/qemu-iotests/209 | 2 +-
tests/qemu-iotests/210 | 6 ++-
tests/qemu-iotests/211 | 6 ++-
tests/qemu-iotests/212 | 6 ++-
tests/qemu-iotests/213 | 6 ++-
tests/qemu-iotests/216 | 4 +-
tests/qemu-iotests/218 | 2 +-
tests/qemu-iotests/219 | 2 +-
tests/qemu-iotests/222 | 7 ++--
tests/qemu-iotests/224 | 4 +-
tests/qemu-iotests/228 | 6 ++-
tests/qemu-iotests/234 | 4 +-
tests/qemu-iotests/235 | 4 +-
tests/qemu-iotests/236 | 2 +-
tests/qemu-iotests/237 | 2 +-
tests/qemu-iotests/238 | 2 +
tests/qemu-iotests/242 | 2 +-
tests/qemu-iotests/246 | 2 +-
tests/qemu-iotests/248 | 2 +-
tests/qemu-iotests/254 | 2 +-
tests/qemu-iotests/255 | 2 +-
tests/qemu-iotests/256 | 2 +-
tests/qemu-iotests/258 | 7 ++--
tests/qemu-iotests/260 | 4 +-
tests/qemu-iotests/262 | 4 +-
tests/qemu-iotests/264 | 4 +-
tests/qemu-iotests/277 | 2 +
tests/qemu-iotests/280 | 8 ++--
tests/qemu-iotests/283 | 4 +-
tests/qemu-iotests/iotests.py | 76 +++++++++++++++++++++++------------
37 files changed, 130 insertions(+), 81 deletions(-)
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index b4a21bf7b7..852768f80a 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -382,8 +382,7 @@ def test_once(config, qemu_img=False):
# Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
# We need sudo in order to run cryptsetup to create
# dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 9dc1bd3510..8b1f720af4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,8 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
+ supported_platforms=['linux'])
with iotests.FilePath('source.img') as source_img_path, \
iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 920a8683ef..e3900a44d1 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,8 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_platforms=['linux'])
with iotests.FilePath('disk0.img') as disk0_img_path, \
iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 49eff5d405..4b4bd3307d 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,8 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_platforms=['linux'])
with iotests.FilePath('disk0.img') as disk0_img_path, \
iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e2b50ae24d..f42432a838 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
with iotests.FilePath('t.qcow2') as disk_path, \
iotests.FilePath('t.qcow2.base') as backing_path, \
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 3d9c1208ca..a6621410da 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,10 @@ import iotests
import subprocess
import re
-iotests.verify_image_format(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(
+ supported_fmts=['raw'],
+ supported_protocols=['ssh'],
+)
def filter_hash(qmsg):
def _filter(key, value):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1c3fc8c7fd..6cb642f821 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
import iotests
-iotests.verify_image_format(supported_fmts=['generic'])
+iotests.script_initialize(supported_fmts=['generic'])
with iotests.FilePath('disk.img') as disk_img_path, \
iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
diff --git a/tests/qemu-iotests/209 b/tests/qemu-iotests/209
index 65c1a1e70a..8c804f4a30 100755
--- a/tests/qemu-iotests/209
+++ b/tests/qemu-iotests/209
@@ -22,7 +22,7 @@ import iotests
from iotests import qemu_img_create, qemu_io, qemu_img_verbose, qemu_nbd, \
file_path
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
disk = file_path('disk')
nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index e49896e23d..7bf591f313 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -23,8 +23,10 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(
+ supported_fmts=['luks'],
+ supported_protocols=['file'],
+)
with iotests.FilePath('t.luks') as disk_path, \
iotests.VM() as vm:
diff --git a/tests/qemu-iotests/211 b/tests/qemu-iotests/211
index 163994d559..4969edb00c 100755
--- a/tests/qemu-iotests/211
+++ b/tests/qemu-iotests/211
@@ -23,8 +23,10 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['vdi'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(
+ supported_fmts=['vdi'],
+ supported_protocols=['file'],
+)
def blockdev_create(vm, options):
error = vm.blockdev_create(options)
diff --git a/tests/qemu-iotests/212 b/tests/qemu-iotests/212
index 800f92dd84..45d08842bb 100755
--- a/tests/qemu-iotests/212
+++ b/tests/qemu-iotests/212
@@ -23,8 +23,10 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['parallels'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(
+ supported_fmts=['parallels'],
+ supported_protocols=['file'],
+)
with iotests.FilePath('t.parallels') as disk_path, \
iotests.VM() as vm:
diff --git a/tests/qemu-iotests/213 b/tests/qemu-iotests/213
index 1eee45276a..cf638eb927 100755
--- a/tests/qemu-iotests/213
+++ b/tests/qemu-iotests/213
@@ -23,8 +23,10 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['vhdx'])
-iotests.verify_protocol(supported=['file'])
+iotests.script_initialize(
+ supported_fmts=['vhdx'],
+ supported_protocols=['file'],
+)
with iotests.FilePath('t.vhdx') as disk_path, \
iotests.VM() as vm:
diff --git a/tests/qemu-iotests/216 b/tests/qemu-iotests/216
index 372f042d3e..de11d85b5d 100755
--- a/tests/qemu-iotests/216
+++ b/tests/qemu-iotests/216
@@ -23,8 +23,8 @@ import iotests
from iotests import log, qemu_img, qemu_io_silent
# Need backing file support
-iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'],
+ supported_platforms=['linux'])
log('')
log('=== Copy-on-read across nodes ===')
diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 1325ba9eaa..5586870456 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -29,7 +29,7 @@
import iotests
from iotests import log, qemu_img, qemu_io_silent
-iotests.verify_image_format(supported_fmts=['qcow2', 'raw'])
+iotests.script_initialize(supported_fmts=['qcow2', 'raw'])
# Launches the VM, adds two null-co nodes (source and target), and
diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index b8774770c4..db272c5249 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -21,7 +21,7 @@
import iotests
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
img_size = 4 * 1024 * 1024
diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index bf1718e179..6602f6b4ba 100755
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -24,9 +24,10 @@
import iotests
from iotests import log, qemu_img, qemu_io, qemu_io_silent
-iotests.verify_platform(['linux'])
-iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk',
- 'vhdx', 'raw'])
+iotests.script_initialize(
+ supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk', 'vhdx', 'raw'],
+ supported_platforms=['linux'],
+)
patterns = [("0x5d", "0", "64k"),
("0xd5", "1M", "64k"),
diff --git a/tests/qemu-iotests/224 b/tests/qemu-iotests/224
index e91fb26fd8..81ca1e4898 100755
--- a/tests/qemu-iotests/224
+++ b/tests/qemu-iotests/224
@@ -26,8 +26,8 @@ from iotests import log, qemu_img, qemu_io_silent, filter_qmp_testfiles, \
import json
# Need backing file support (for arbitrary backing formats)
-iotests.verify_image_format(supported_fmts=['qcow2', 'qcow', 'qed'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed'],
+ supported_platforms=['linux'])
# There are two variations of this test:
diff --git a/tests/qemu-iotests/228 b/tests/qemu-iotests/228
index 64bc82ee23..da0900fb82 100755
--- a/tests/qemu-iotests/228
+++ b/tests/qemu-iotests/228
@@ -25,8 +25,10 @@ from iotests import log, qemu_img, filter_testfiles, filter_imgfmt, \
filter_qmp_testfiles, filter_qmp_imgfmt
# Need backing file and change-backing-file support
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(
+ supported_fmts=['qcow2', 'qed'],
+ supported_platforms=['linux'],
+)
def log_node_info(node):
diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
index 324c1549fd..73c899ddae 100755
--- a/tests/qemu-iotests/234
+++ b/tests/qemu-iotests/234
@@ -23,8 +23,8 @@
import iotests
import os
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_platforms=['linux'])
with iotests.FilePath('img') as img_path, \
iotests.FilePath('backing') as backing_path, \
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 760826128e..d1b10ac36b 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -27,6 +27,8 @@ sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu.machine import QEMUMachine
+iotests.script_initialize(supported_fmts=['qcow2'])
+
# Note:
# This test was added to check that mirror dead-lock was fixed (see previous
# commit before this test addition).
@@ -40,8 +42,6 @@ from qemu.machine import QEMUMachine
size = 1 * 1024 * 1024 * 1024
-iotests.verify_image_format(supported_fmts=['qcow2'])
-
disk = file_path('disk')
# prepare source image
diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
index 8ce927a16c..6f5cee2444 100755
--- a/tests/qemu-iotests/236
+++ b/tests/qemu-iotests/236
@@ -22,7 +22,7 @@
import iotests
from iotests import log
-iotests.verify_image_format(supported_fmts=['generic'])
+iotests.script_initialize(supported_fmts=['generic'])
size = 64 * 1024 * 1024
granularity = 64 * 1024
diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
index 50ba364a3e..5b21ad3509 100755
--- a/tests/qemu-iotests/237
+++ b/tests/qemu-iotests/237
@@ -24,7 +24,7 @@ import math
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['vmdk'])
+iotests.script_initialize(supported_fmts=['vmdk'])
with iotests.FilePath('t.vmdk') as disk_path, \
iotests.FilePath('t.vmdk.1') as extent1_path, \
diff --git a/tests/qemu-iotests/238 b/tests/qemu-iotests/238
index d4e060228c..b8fcf15a1f 100755
--- a/tests/qemu-iotests/238
+++ b/tests/qemu-iotests/238
@@ -23,6 +23,8 @@ import os
import iotests
from iotests import log
+iotests.script_initialize()
+
virtio_scsi_device = iotests.get_virtio_scsi_device()
vm = iotests.VM()
diff --git a/tests/qemu-iotests/242 b/tests/qemu-iotests/242
index 97617876bc..64f1bd95e4 100755
--- a/tests/qemu-iotests/242
+++ b/tests/qemu-iotests/242
@@ -24,7 +24,7 @@ import struct
from iotests import qemu_img_create, qemu_io, qemu_img_pipe, \
file_path, img_info_log, log, filter_qemu_io
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
disk = file_path('disk')
chunk = 256 * 1024
diff --git a/tests/qemu-iotests/246 b/tests/qemu-iotests/246
index 59a216a839..58a479cc1f 100755
--- a/tests/qemu-iotests/246
+++ b/tests/qemu-iotests/246
@@ -22,7 +22,7 @@
import iotests
from iotests import log
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
size = 64 * 1024 * 1024 * 1024
gran_small = 32 * 1024
gran_large = 128 * 1024
diff --git a/tests/qemu-iotests/248 b/tests/qemu-iotests/248
index 68c374692e..18ba03467e 100755
--- a/tests/qemu-iotests/248
+++ b/tests/qemu-iotests/248
@@ -21,7 +21,7 @@
import iotests
from iotests import qemu_img_create, qemu_io, file_path, filter_qmp_testfiles
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
source, target = file_path('source', 'target')
size = 5 * 1024 * 1024
diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index ee66c986db..150e58be8e 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -21,7 +21,7 @@
import iotests
from iotests import qemu_img_create, file_path, log
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
disk, top = file_path('disk', 'top')
size = 1024 * 1024
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 4a4818bafb..8f08f741da 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -23,7 +23,7 @@
import iotests
from iotests import imgfmt
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
iotests.log('Finishing a commit job with background reads')
iotests.log('============================================')
diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
index e34074c83e..db8d6f31cf 100755
--- a/tests/qemu-iotests/256
+++ b/tests/qemu-iotests/256
@@ -23,7 +23,7 @@ import os
import iotests
from iotests import log
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
size = 64 * 1024 * 1024
with iotests.FilePath('img0') as img0_path, \
diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index 091755a45c..a65151dda6 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -24,9 +24,10 @@ from iotests import log, qemu_img, qemu_io_silent, \
filter_qmp_testfiles, filter_qmp_imgfmt
# Need backing file and change-backing-file support
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed'])
-iotests.verify_platform(['linux'])
-
+iotests.script_initialize(
+ supported_fmts=['qcow2', 'qed'],
+ supported_platforms=['linux'],
+)
# Returns a node for blockdev-add
def node(node_name, path, backing=None, fmt=None, throttle=None):
diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
index 30c0de380d..804a7addb9 100755
--- a/tests/qemu-iotests/260
+++ b/tests/qemu-iotests/260
@@ -21,7 +21,9 @@
import iotests
from iotests import qemu_img_create, file_path, log, filter_qmp_event
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(
+ supported_fmts=['qcow2']
+)
base, top = file_path('base', 'top')
size = 64 * 1024 * 3
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 8835dce7be..03af061f94 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -23,8 +23,8 @@
import iotests
import os
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+ supported_platforms=['linux'])
with iotests.FilePath('img') as img_path, \
iotests.FilePath('mig_fifo') as fifo, \
diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
index 879123a343..304a7443d7 100755
--- a/tests/qemu-iotests/264
+++ b/tests/qemu-iotests/264
@@ -24,7 +24,9 @@ import iotests
from iotests import qemu_img_create, qemu_io_silent_check, file_path, \
qemu_nbd_popen, log
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(
+ supported_fmts=['qcow2'],
+)
disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
nbd_uri = 'nbd+unix:///?socket=' + nbd_sock
diff --git a/tests/qemu-iotests/277 b/tests/qemu-iotests/277
index 04aa15a3d5..d34f87021f 100755
--- a/tests/qemu-iotests/277
+++ b/tests/qemu-iotests/277
@@ -23,6 +23,8 @@ import subprocess
import iotests
from iotests import file_path, log
+iotests.script_initialize()
+
nbd_sock, conf_file = file_path('nbd-sock', 'nbd-fault-injector.conf')
diff --git a/tests/qemu-iotests/280 b/tests/qemu-iotests/280
index 69288fdd0e..f594bb9ed2 100755
--- a/tests/qemu-iotests/280
+++ b/tests/qemu-iotests/280
@@ -22,9 +22,11 @@
import iotests
import os
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_protocol(supported=['file'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(
+ supported_fmts=['qcow2'],
+ supported_protocols=['file'],
+ supported_platforms=['linux'],
+)
with iotests.FilePath('base') as base_path , \
iotests.FilePath('top') as top_path, \
diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
index 55b7cff953..e17b953333 100644
--- a/tests/qemu-iotests/283
+++ b/tests/qemu-iotests/283
@@ -21,7 +21,9 @@
import iotests
# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(
+ supported_fmts=['qcow2'],
+)
size = 1024 * 1024
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dfc753c319..21f96b35e4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,7 +28,8 @@
import struct
import subprocess
import sys
-from typing import (Any, Callable, Dict, Iterable, List, Optional, TypeVar)
+from typing import (Any, Callable, Dict, Iterable,
+ List, Optional, Sequence, TypeVar)
import unittest
# pylint: disable=import-error, wrong-import-position
@@ -1028,12 +1029,11 @@ def verify_protocol(supported=(), unsupported=()):
if not_sup or (imgproto in unsupported):
notrun('not suitable for this protocol: %s' % imgproto)
-def verify_platform(supported=None, unsupported=None):
- if unsupported is not None:
- if any((sys.platform.startswith(x) for x in unsupported)):
- notrun('not suitable for this OS: %s' % sys.platform)
+def verify_platform(supported=(), unsupported=()):
+ if any((sys.platform.startswith(x) for x in unsupported)):
+ notrun('not suitable for this OS: %s' % sys.platform)
- if supported is not None:
+ if supported:
if not any((sys.platform.startswith(x) for x in supported)):
notrun('not suitable for this OS: %s' % sys.platform)
@@ -1115,7 +1115,18 @@ def func_wrapper(*args, **kwargs):
return func(*args, **kwargs)
return func_wrapper
-def execute_unittest(output, verbosity, debug):
+def execute_unittest(debug=False):
+ """Executes unittests within the calling module."""
+
+ verbosity = 2 if debug else 1
+
+ if debug:
+ output = sys.stdout
+ else:
+ # We need to filter out the time taken from the output so that
+ # qemu-iotest can reliably diff the results against master output.
+ output = io.StringIO()
+
runner = unittest.TextTestRunner(stream=output, descriptions=True,
verbosity=verbosity)
try:
@@ -1123,6 +1134,8 @@ def execute_unittest(output, verbosity, debug):
# exception
unittest.main(testRunner=runner)
finally:
+ # We need to filter out the time taken from the output so that
+ # qemu-iotest can reliably diff the results against master output.
if not debug:
out = output.getvalue()
out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', out)
@@ -1134,13 +1147,19 @@ def execute_unittest(output, verbosity, debug):
sys.stderr.write(out)
-def execute_test(test_function=None,
- supported_fmts=(),
- supported_platforms=None,
- supported_cache_modes=(), supported_aio_modes=(),
- unsupported_fmts=(), supported_protocols=(),
- unsupported_protocols=()):
- """Run either unittest or script-style tests."""
+def execute_setup_common(supported_fmts: Sequence[str] = (),
+ supported_platforms: Sequence[str] = (),
+ supported_cache_modes: Sequence[str] = (),
+ supported_aio_modes: Sequence[str] = (),
+ unsupported_fmts: Sequence[str] = (),
+ supported_protocols: Sequence[str] = (),
+ unsupported_protocols: Sequence[str] = ()) -> bool:
+ """
+ Perform necessary setup for either script-style or unittest-style tests.
+
+ :return: Bool; Whether or not debug mode has been requested via the CLI.
+ """
+ # Note: Python 3.6 and pylint do not like 'Collection' so use 'Sequence'.
# We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
# indicate that we're not being run via "check". There may be
@@ -1150,34 +1169,39 @@ def execute_test(test_function=None,
sys.stderr.write('Please run this test via the "check" script\n')
sys.exit(os.EX_USAGE)
- debug = '-d' in sys.argv
- verbosity = 1
verify_image_format(supported_fmts, unsupported_fmts)
verify_protocol(supported_protocols, unsupported_protocols)
verify_platform(supported=supported_platforms)
verify_cache_mode(supported_cache_modes)
verify_aio_mode(supported_aio_modes)
+ debug = '-d' in sys.argv
if debug:
- output = sys.stdout
- verbosity = 2
sys.argv.remove('-d')
- else:
- # We need to filter out the time taken from the output so that
- # qemu-iotest can reliably diff the results against master output.
- output = io.StringIO()
-
logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
+ return debug
+
+def execute_test(*args, test_function=None, **kwargs):
+ """Run either unittest or script-style tests."""
+
+ debug = execute_setup_common(*args, **kwargs)
if not test_function:
- execute_unittest(output, verbosity, debug)
+ execute_unittest(debug)
else:
test_function()
+# This is called from script-style iotests without a single point of entry
+def script_initialize(*args, **kwargs):
+ """Initialize script-style tests without running any tests."""
+ execute_setup_common(*args, **kwargs)
+
+# This is called from script-style iotests with a single point of entry
def script_main(test_function, *args, **kwargs):
"""Run script-style tests outside of the unittest framework"""
- execute_test(test_function, *args, **kwargs)
+ execute_test(*args, test_function=test_function, **kwargs)
+# This is called from unittest style iotests
def main(*args, **kwargs):
"""Run tests using the unittest framework"""
- execute_test(None, *args, **kwargs)
+ execute_test(*args, **kwargs)
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 12/14] iotest 258: use script_main
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (10 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 11/14] iotests: add script_initialize John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 0:00 ` [PATCH v10 13/14] iotests: Mark verify functions as private John Snow
` (3 subsequent siblings)
15 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
Since this one is nicely factored to use a single entry point,
use script_main to run the tests.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/258 | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index a65151dda6..e305a1502f 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -23,12 +23,6 @@ import iotests
from iotests import log, qemu_img, qemu_io_silent, \
filter_qmp_testfiles, filter_qmp_imgfmt
-# Need backing file and change-backing-file support
-iotests.script_initialize(
- supported_fmts=['qcow2', 'qed'],
- supported_platforms=['linux'],
-)
-
# Returns a node for blockdev-add
def node(node_name, path, backing=None, fmt=None, throttle=None):
if fmt is None:
@@ -161,4 +155,7 @@ def main():
test_concurrent_finish(False)
if __name__ == '__main__':
- main()
+ # Need backing file and change-backing-file support
+ iotests.script_main(main,
+ supported_fmts=['qcow2', 'qed'],
+ supported_platforms=['linux'])
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 13/14] iotests: Mark verify functions as private
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (11 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 12/14] iotest 258: use script_main John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 10:40 ` Max Reitz
2020-03-31 0:00 ` [PATCH v10 14/14] iotests: use python logging for iotests.log() John Snow
` (2 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
Mark the verify functions as "private" with a leading underscore, to
discourage their use. Update type signatures while we're here.
(Also, make pending patches not yet using the new entry points fail in a
very obvious way.)
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 21f96b35e4..fd8cb36622 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1005,7 +1005,8 @@ def case_notrun(reason):
open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
' [case not run] ' + reason + '\n')
-def verify_image_format(supported_fmts=(), unsupported_fmts=()):
+def _verify_image_format(supported_fmts: Sequence[str] = (),
+ unsupported_fmts: Sequence[str] = ()) -> None:
assert not (supported_fmts and unsupported_fmts)
if 'generic' in supported_fmts and \
@@ -1019,7 +1020,8 @@ def verify_image_format(supported_fmts=(), unsupported_fmts=()):
if not_sup or (imgfmt in unsupported_fmts):
notrun('not suitable for this image format: %s' % imgfmt)
-def verify_protocol(supported=(), unsupported=()):
+def _verify_protocol(supported: Sequence[str] = (),
+ unsupported: Sequence[str] = ()) -> None:
assert not (supported and unsupported)
if 'generic' in supported:
@@ -1029,7 +1031,8 @@ def verify_protocol(supported=(), unsupported=()):
if not_sup or (imgproto in unsupported):
notrun('not suitable for this protocol: %s' % imgproto)
-def verify_platform(supported=(), unsupported=()):
+def _verify_platform(supported: Sequence[str] = (),
+ unsupported: Sequence[str] = ()) -> None:
if any((sys.platform.startswith(x) for x in unsupported)):
notrun('not suitable for this OS: %s' % sys.platform)
@@ -1037,11 +1040,11 @@ def verify_platform(supported=(), unsupported=()):
if not any((sys.platform.startswith(x) for x in supported)):
notrun('not suitable for this OS: %s' % sys.platform)
-def verify_cache_mode(supported_cache_modes=()):
+def _verify_cache_mode(supported_cache_modes: Sequence[str] = ()) -> None:
if supported_cache_modes and (cachemode not in supported_cache_modes):
notrun('not suitable for this cache mode: %s' % cachemode)
-def verify_aio_mode(supported_aio_modes=()):
+def _verify_aio_mode(supported_aio_modes: Sequence[str] = ()):
if supported_aio_modes and (aiomode not in supported_aio_modes):
notrun('not suitable for this aio mode: %s' % aiomode)
@@ -1169,11 +1172,11 @@ def execute_setup_common(supported_fmts: Sequence[str] = (),
sys.stderr.write('Please run this test via the "check" script\n')
sys.exit(os.EX_USAGE)
- verify_image_format(supported_fmts, unsupported_fmts)
- verify_protocol(supported_protocols, unsupported_protocols)
- verify_platform(supported=supported_platforms)
- verify_cache_mode(supported_cache_modes)
- verify_aio_mode(supported_aio_modes)
+ _verify_image_format(supported_fmts, unsupported_fmts)
+ _verify_protocol(supported_protocols, unsupported_protocols)
+ _verify_platform(supported=supported_platforms)
+ _verify_cache_mode(supported_cache_modes)
+ _verify_aio_mode(supported_aio_modes)
debug = '-d' in sys.argv
if debug:
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (12 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 13/14] iotests: Mark verify functions as private John Snow
@ 2020-03-31 0:00 ` John Snow
2020-03-31 13:44 ` Kevin Wolf
2020-03-31 15:07 ` [PATCH v10 00/14] iotests: use python logging Kevin Wolf
2020-04-28 11:46 ` Max Reitz
15 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-03-31 0:00 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz, John Snow, philmd
We can turn logging on/off globally instead of per-function.
Remove use_log from run_job, and use python logging to turn on
diffable output when we run through a script entry point.
iotest 245 changes output order due to buffering reasons.
An extended note on python logging:
A NullHandler is added to `qemu.iotests` to stop output from being
generated if this code is used as a library without configuring logging.
A NullHandler is only needed at the root, so a duplicate handler is not
needed for `qemu.iotests.diff_io`.
When logging is not configured, messages at the 'WARNING' levels or
above are printed with default settings. The NullHandler stops this from
occurring, which is considered good hygiene for code used as a library.
See https://docs.python.org/3/howto/logging.html#library-config
When logging is actually enabled (always at the behest of an explicit
call by a client script), a root logger is implicitly created at the
root, which allows messages to propagate upwards and be handled/emitted
from the root logger with default settings.
When we want iotest logging, we attach a handler to the
qemu.iotests.diff_io logger and disable propagation to avoid possible
double-printing.
For more information on python logging infrastructure, I highly
recommend downloading the pip package `logging_tree`, which provides
convenient visualizations of the hierarchical logging configuration
under different circumstances.
See https://pypi.org/project/logging_tree/ for more information.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/030 | 4 +--
tests/qemu-iotests/155 | 2 +-
tests/qemu-iotests/245 | 1 +
tests/qemu-iotests/245.out | 24 ++++++++--------
tests/qemu-iotests/iotests.py | 53 ++++++++++++++++++++---------------
5 files changed, 46 insertions(+), 38 deletions(-)
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a..104e3cee1b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
self.assert_qmp(result, 'return', {})
- self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
- self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+ self.vm.run_job(job='drive0', auto_dismiss=True)
+ self.vm.run_job(job='node4', auto_dismiss=True)
self.assert_no_active_block_jobs()
# Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 571bce9de4..cb371d4649 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -188,7 +188,7 @@ class MirrorBaseClass(BaseClass):
self.assert_qmp(result, 'return', {})
- self.vm.run_job('mirror-job', use_log=False, auto_finalize=False,
+ self.vm.run_job('mirror-job', auto_finalize=False,
pre_finalize=self.openBacking, auto_dismiss=True)
def testFull(self):
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 1001275a44..4f5f0bb901 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1027,5 +1027,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.run_test_iothreads(None, 'iothread0')
if __name__ == '__main__':
+ iotests.activate_logging()
iotests.main(supported_fmts=["qcow2"],
supported_protocols=["file"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 682b93394d..4b33dcaf5c 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
.....................
----------------------------------------------------------------------
Ran 21 tests
OK
-{"execute": "job-finalize", "arguments": {"id": "commit0"}}
-{"return": {}}
-{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"execute": "job-finalize", "arguments": {"id": "stream0"}}
-{"return": {}}
-{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
-{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fd8cb36622..56179605a9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -42,6 +42,14 @@
QMPResponse = Dict[str, Any]
+# Use this logger for logging messages directly from the iotests module
+logger = logging.getLogger('qemu.iotests')
+logger.addHandler(logging.NullHandler())
+
+# Use this logger for messages that ought to be used for diff output.
+test_logger = logging.getLogger('qemu.iotests.diff_io')
+
+
faulthandler.enable()
# This will not work if arguments contain spaces but is necessary if we
@@ -384,9 +392,9 @@ def log(msg: Msg,
if isinstance(msg, (dict, list)):
# Don't sort if it's already sorted
do_sort = not isinstance(msg, OrderedDict)
- print(json.dumps(msg, sort_keys=do_sort, indent=indent))
+ test_logger.info(json.dumps(msg, sort_keys=do_sort, indent=indent))
else:
- print(msg)
+ test_logger.info(msg)
class Timeout:
def __init__(self, seconds, errmsg="Timeout"):
@@ -608,7 +616,7 @@ def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
# Returns None on success, and an error string on failure
def run_job(self, job, auto_finalize=True, auto_dismiss=False,
- pre_finalize=None, cancel=False, use_log=True, wait=60.0):
+ pre_finalize=None, cancel=False, wait=60.0):
"""
run_job moves a job from creation through to dismissal.
@@ -621,7 +629,6 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
invoked prior to issuing job-finalize, if any.
:param cancel: Bool. When true, cancels the job after the pre_finalize
callback.
- :param use_log: Bool. When false, does not log QMP messages.
:param wait: Float. Timeout value specifying how long to wait for any
event, in seconds. Defaults to 60.0.
"""
@@ -639,8 +646,7 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
while True:
ev = filter_qmp_event(self.events_wait(events, timeout=wait))
if ev['event'] != 'JOB_STATUS_CHANGE':
- if use_log:
- log(ev)
+ log(ev)
continue
status = ev['data']['status']
if status == 'aborting':
@@ -648,29 +654,18 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False,
for j in result['return']:
if j['id'] == job:
error = j['error']
- if use_log:
- log('Job failed: %s' % (j['error']))
+ log('Job failed: %s' % (j['error']))
elif status == 'ready':
- if use_log:
- self.qmp_log('job-complete', id=job)
- else:
- self.qmp('job-complete', id=job)
+ self.qmp_log('job-complete', id=job)
elif status == 'pending' and not auto_finalize:
if pre_finalize:
pre_finalize()
- if cancel and use_log:
+ if cancel:
self.qmp_log('job-cancel', id=job)
- elif cancel:
- self.qmp('job-cancel', id=job)
- elif use_log:
+ else:
self.qmp_log('job-finalize', id=job)
- else:
- self.qmp('job-finalize', id=job)
elif status == 'concluded' and not auto_dismiss:
- if use_log:
- self.qmp_log('job-dismiss', id=job)
- else:
- self.qmp('job-dismiss', id=job)
+ self.qmp_log('job-dismiss', id=job)
elif status == 'null':
return error
@@ -990,7 +985,7 @@ def notrun(reason):
seq = os.path.basename(sys.argv[0])
open('%s/%s.notrun' % (output_dir, seq), 'w').write(reason + '\n')
- print('%s not run: %s' % (seq, reason))
+ logger.warning("%s not run: %s", seq, reason)
sys.exit(0)
def case_notrun(reason):
@@ -1182,6 +1177,7 @@ def execute_setup_common(supported_fmts: Sequence[str] = (),
if debug:
sys.argv.remove('-d')
logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
+ logger.debug("iotests debugging messages active")
return debug
@@ -1194,14 +1190,25 @@ def execute_test(*args, test_function=None, **kwargs):
else:
test_function()
+def activate_logging():
+ """Activate iotests.log() output to stdout for script-style tests."""
+ handler = logging.StreamHandler(stream=sys.stdout)
+ formatter = logging.Formatter('%(message)s')
+ handler.setFormatter(formatter)
+ test_logger.addHandler(handler)
+ test_logger.setLevel(logging.INFO)
+ test_logger.propagate = False
+
# This is called from script-style iotests without a single point of entry
def script_initialize(*args, **kwargs):
"""Initialize script-style tests without running any tests."""
+ activate_logging()
execute_setup_common(*args, **kwargs)
# This is called from script-style iotests with a single point of entry
def script_main(test_function, *args, **kwargs):
"""Run script-style tests outside of the unittest framework"""
+ activate_logging()
execute_test(*args, test_function=test_function, **kwargs)
# This is called from unittest style iotests
--
2.21.1
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v10 06/14] iotests: alphabetize standard imports
2020-03-31 0:00 ` [PATCH v10 06/14] iotests: alphabetize standard imports John Snow
@ 2020-03-31 8:00 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 41+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-31 8:00 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, ehabkost, qemu-block, armbru, Max Reitz
On 3/31/20 2:00 AM, John Snow wrote:
> I had to fix a merge conflict, so do this tiny harmless thing while I'm
> here.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 51f84475d9..e6f9f62b2b 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -16,19 +16,19 @@
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
> #
>
> +import atexit
> +from collections import OrderedDict
> +import faulthandler
> +import io
> +import json
> +import logging
> import os
> import re
> +import signal
> +import struct
> import subprocess
> -import unittest
> import sys
> -import struct
> -import json
> -import signal
> -import logging
> -import atexit
> -import io
> -from collections import OrderedDict
> -import faulthandler
> +import unittest
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> # pylint: disable=import-error, wrong-import-position
> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 0:00 ` [PATCH v10 10/14] iotests: add hmp helper with logging John Snow
@ 2020-03-31 10:21 ` Max Reitz
2020-03-31 14:00 ` Kevin Wolf
2020-03-31 17:23 ` John Snow
0 siblings, 2 replies; 41+ messages in thread
From: Max Reitz @ 2020-03-31 10:21 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, ehabkost, qemu-block, philmd, armbru
[-- Attachment #1.1: Type: text/plain, Size: 2570 bytes --]
On 31.03.20 02:00, John Snow wrote:
> Minor cleanup for HMP functions; helps with line length and consolidates
> HMP helpers through one implementation function.
>
> Although we are adding a universal toggle to turn QMP logging on or off,
> many existing callers to hmp functions don't expect that output to be
> logged, which causes quite a few changes in the test output.
>
> For now, offer a use_log parameter.
>
>
> Typing notes:
>
> QMPResponse is just an alias for Dict[str, Any]. It holds no special
> meanings and it is not a formal subtype of Dict[str, Any]. It is best
> thought of as a lexical synonym.
>
> We may well wish to add stricter subtypes in the future for certain
> shapes of data that are not formalized as Python objects, at which point
> we can simply retire the alias and allow mypy to more strictly check
> usages of the name.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b08bcb87e1..dfc753c319 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -37,6 +37,10 @@
>
> assert sys.version_info >= (3, 6)
>
> +# Type Aliases
> +QMPResponse = Dict[str, Any]
> +
> +
> faulthandler.enable()
>
> # This will not work if arguments contain spaces but is necessary if we
> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
> self._args.append(addr)
> return self
>
> - def pause_drive(self, drive, event=None):
> - '''Pause drive r/w operations'''
> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> + cmd = 'human-monitor-command'
> + kwargs = {'command-line': command_line}
> + if use_log:
> + return self.qmp_log(cmd, **kwargs)
> + else:
> + return self.qmp(cmd, **kwargs)
Hm. I suppose I should take this chance to understand something about
mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
really returns QMPResponse. Is there some flag to make it? Like
--actually-check-types?
(--strict seems, well, overly strict? Like not allowing generics, I
don’t see why. Or I suppose for the time being we want to allow untyped
definitions, as long as they don’t break type assertions such as it kind
of does here...?)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 13/14] iotests: Mark verify functions as private
2020-03-31 0:00 ` [PATCH v10 13/14] iotests: Mark verify functions as private John Snow
@ 2020-03-31 10:40 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2020-03-31 10:40 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, ehabkost, qemu-block, philmd, armbru
[-- Attachment #1.1: Type: text/plain, Size: 500 bytes --]
On 31.03.20 02:00, John Snow wrote:
> Mark the verify functions as "private" with a leading underscore, to
> discourage their use. Update type signatures while we're here.
>
> (Also, make pending patches not yet using the new entry points fail in a
> very obvious way.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-03-31 0:00 ` [PATCH v10 14/14] iotests: use python logging for iotests.log() John Snow
@ 2020-03-31 13:44 ` Kevin Wolf
2020-05-14 6:24 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-03-31 13:44 UTC (permalink / raw)
To: John Snow; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
Am 31.03.2020 um 02:00 hat John Snow geschrieben:
> We can turn logging on/off globally instead of per-function.
>
> Remove use_log from run_job, and use python logging to turn on
> diffable output when we run through a script entry point.
>
> iotest 245 changes output order due to buffering reasons.
>
>
> An extended note on python logging:
>
> A NullHandler is added to `qemu.iotests` to stop output from being
> generated if this code is used as a library without configuring logging.
> A NullHandler is only needed at the root, so a duplicate handler is not
> needed for `qemu.iotests.diff_io`.
>
> When logging is not configured, messages at the 'WARNING' levels or
> above are printed with default settings. The NullHandler stops this from
> occurring, which is considered good hygiene for code used as a library.
>
> See https://docs.python.org/3/howto/logging.html#library-config
>
> When logging is actually enabled (always at the behest of an explicit
> call by a client script), a root logger is implicitly created at the
> root, which allows messages to propagate upwards and be handled/emitted
> from the root logger with default settings.
>
> When we want iotest logging, we attach a handler to the
> qemu.iotests.diff_io logger and disable propagation to avoid possible
> double-printing.
>
> For more information on python logging infrastructure, I highly
> recommend downloading the pip package `logging_tree`, which provides
> convenient visualizations of the hierarchical logging configuration
> under different circumstances.
>
> See https://pypi.org/project/logging_tree/ for more information.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Should we enable logger if -d is given?
Previously we had:
$ ./check -d -T -raw 281
[...]
281 not run: not suitable for this image format: raw
281 not run [15:39:03] [15:39:04] not suitable for this image format: raw
Not run: 281
After this series, the first line of output from notrun() is missing.
Not that I think it's important to have the line, but as long as we
bother to call logger.warning(), I thought that maybe we want to be able
to actually see the effect of it somehwere?
Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 10:21 ` Max Reitz
@ 2020-03-31 14:00 ` Kevin Wolf
2020-04-01 12:28 ` Max Reitz
2020-03-31 17:23 ` John Snow
1 sibling, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-03-31 14:00 UTC (permalink / raw)
To: Max Reitz; +Cc: ehabkost, qemu-block, John Snow, qemu-devel, armbru, philmd
[-- Attachment #1: Type: text/plain, Size: 3425 bytes --]
Am 31.03.2020 um 12:21 hat Max Reitz geschrieben:
> On 31.03.20 02:00, John Snow wrote:
> > Minor cleanup for HMP functions; helps with line length and consolidates
> > HMP helpers through one implementation function.
> >
> > Although we are adding a universal toggle to turn QMP logging on or off,
> > many existing callers to hmp functions don't expect that output to be
> > logged, which causes quite a few changes in the test output.
> >
> > For now, offer a use_log parameter.
> >
> >
> > Typing notes:
> >
> > QMPResponse is just an alias for Dict[str, Any]. It holds no special
> > meanings and it is not a formal subtype of Dict[str, Any]. It is best
> > thought of as a lexical synonym.
> >
> > We may well wish to add stricter subtypes in the future for certain
> > shapes of data that are not formalized as Python objects, at which point
> > we can simply retire the alias and allow mypy to more strictly check
> > usages of the name.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> > tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
> > 1 file changed, 22 insertions(+), 13 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index b08bcb87e1..dfc753c319 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -37,6 +37,10 @@
> >
> > assert sys.version_info >= (3, 6)
> >
> > +# Type Aliases
> > +QMPResponse = Dict[str, Any]
> > +
> > +
> > faulthandler.enable()
> >
> > # This will not work if arguments contain spaces but is necessary if we
> > @@ -540,25 +544,30 @@ def add_incoming(self, addr):
> > self._args.append(addr)
> > return self
> >
> > - def pause_drive(self, drive, event=None):
> > - '''Pause drive r/w operations'''
> > + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> > + cmd = 'human-monitor-command'
> > + kwargs = {'command-line': command_line}
> > + if use_log:
> > + return self.qmp_log(cmd, **kwargs)
> > + else:
> > + return self.qmp(cmd, **kwargs)
>
> Hm. I suppose I should take this chance to understand something about
> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
> really returns QMPResponse. Is there some flag to make it? Like
> --actually-check-types?
There is --check-untyped-defs, but I'm not sure if that actually changes
the return type of untyped functions from Any to an inferred type. I
kind of doubt it.
> (--strict seems, well, overly strict? Like not allowing generics, I
> don’t see why. Or I suppose for the time being we want to allow untyped
> definitions, as long as they don’t break type assertions such as it kind
> of does here...?)
At least, --strict does actually complain about this one because Any
isn't good enough any more (it includes --warn-return-any):
iotests.py:560: warning: Returning Any from function declared to return "Dict[str, Any]"
iotests.py:560: error: Call to untyped function "qmp_log" in typed context
iotests.py:562: warning: Returning Any from function declared to return "Dict[str, Any]"
Not sure why you think it doesn't allow generics? I never had problems
with that, even in my Python museum. :-)
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 00/14] iotests: use python logging
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (13 preceding siblings ...)
2020-03-31 0:00 ` [PATCH v10 14/14] iotests: use python logging for iotests.log() John Snow
@ 2020-03-31 15:07 ` Kevin Wolf
2020-04-28 11:46 ` Max Reitz
15 siblings, 0 replies; 41+ messages in thread
From: Kevin Wolf @ 2020-03-31 15:07 UTC (permalink / raw)
To: John Snow; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
Am 31.03.2020 um 02:00 hat John Snow geschrieben:
> This series uses python logging to enable output conditionally on
> iotests.log(). We unify an initialization call (which also enables
> debugging output for those tests with -d) and then make the switch
> inside of iotests.
>
> It will help alleviate the need to create logged/unlogged versions
> of all the various helpers we have made.
>
> Also, I got lost and accidentally delinted iotests while I was here.
> Sorry about that. By version 9, it's now the overwhelming focus of
> this series. No good deed, etc.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 10:21 ` Max Reitz
2020-03-31 14:00 ` Kevin Wolf
@ 2020-03-31 17:23 ` John Snow
2020-03-31 17:39 ` Kevin Wolf
1 sibling, 1 reply; 41+ messages in thread
From: John Snow @ 2020-03-31 17:23 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, ehabkost, qemu-block, philmd, armbru
On 3/31/20 6:21 AM, Max Reitz wrote:
> On 31.03.20 02:00, John Snow wrote:
>> Minor cleanup for HMP functions; helps with line length and consolidates
>> HMP helpers through one implementation function.
>>
>> Although we are adding a universal toggle to turn QMP logging on or off,
>> many existing callers to hmp functions don't expect that output to be
>> logged, which causes quite a few changes in the test output.
>>
>> For now, offer a use_log parameter.
>>
>>
>> Typing notes:
>>
>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
>> thought of as a lexical synonym.
>>
>> We may well wish to add stricter subtypes in the future for certain
>> shapes of data that are not formalized as Python objects, at which point
>> we can simply retire the alias and allow mypy to more strictly check
>> usages of the name.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
>> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index b08bcb87e1..dfc753c319 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -37,6 +37,10 @@
>>
>> assert sys.version_info >= (3, 6)
>>
>> +# Type Aliases
>> +QMPResponse = Dict[str, Any]
>> +
>> +
>> faulthandler.enable()
>>
>> # This will not work if arguments contain spaces but is necessary if we
>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
>> self._args.append(addr)
>> return self
>>
>> - def pause_drive(self, drive, event=None):
>> - '''Pause drive r/w operations'''
>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>> + cmd = 'human-monitor-command'
>> + kwargs = {'command-line': command_line}
>> + if use_log:
>> + return self.qmp_log(cmd, **kwargs)
>> + else:
>> + return self.qmp(cmd, **kwargs)
>
> Hm. I suppose I should take this chance to understand something about
> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
> really returns QMPResponse. Is there some flag to make it? Like
> --actually-check-types?
>
One of --strict's implied options, I'm not sure which. Otherwise, mypy
is geared towards a 'gradual typing' discipline.
In truth, I'm a little thankful for that because it helps avoid yak
shaving marathons.
It does mean that sometimes the annotations don't "do anything" yet,
apart from offering hints and documentation in e.g. pycharm. Which does
mean that sometimes they can be completely wrong...
The more we add, the more we'll catch problems.
Once this series is dusted I'll try to tackle more conversions for
iotests, qmp, etc. I've got a few WIP patches to tackle conversions for
tests/qemu-iotests/*.py but I am trying to shepherd this one in first
before I go bananas.
> (--strict seems, well, overly strict? Like not allowing generics, I
> don’t see why. Or I suppose for the time being we want to allow untyped
> definitions, as long as they don’t break type assertions such as it kind
> of does here...?)
>
"disallow-any-generics" means disallowing `Any` generics, not
disallowing generics ... in general. (I think? I've been using mypy in
strict mode for a personal project a lot lately and I use generics in a
few places, it seems OK.)
> Max
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 17:23 ` John Snow
@ 2020-03-31 17:39 ` Kevin Wolf
2020-04-01 12:40 ` Max Reitz
0 siblings, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-03-31 17:39 UTC (permalink / raw)
To: John Snow; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
Am 31.03.2020 um 19:23 hat John Snow geschrieben:
>
>
> On 3/31/20 6:21 AM, Max Reitz wrote:
> > On 31.03.20 02:00, John Snow wrote:
> >> Minor cleanup for HMP functions; helps with line length and consolidates
> >> HMP helpers through one implementation function.
> >>
> >> Although we are adding a universal toggle to turn QMP logging on or off,
> >> many existing callers to hmp functions don't expect that output to be
> >> logged, which causes quite a few changes in the test output.
> >>
> >> For now, offer a use_log parameter.
> >>
> >>
> >> Typing notes:
> >>
> >> QMPResponse is just an alias for Dict[str, Any]. It holds no special
> >> meanings and it is not a formal subtype of Dict[str, Any]. It is best
> >> thought of as a lexical synonym.
> >>
> >> We may well wish to add stricter subtypes in the future for certain
> >> shapes of data that are not formalized as Python objects, at which point
> >> we can simply retire the alias and allow mypy to more strictly check
> >> usages of the name.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
> >> 1 file changed, 22 insertions(+), 13 deletions(-)
> >
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> >
> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >> index b08bcb87e1..dfc753c319 100644
> >> --- a/tests/qemu-iotests/iotests.py
> >> +++ b/tests/qemu-iotests/iotests.py
> >> @@ -37,6 +37,10 @@
> >>
> >> assert sys.version_info >= (3, 6)
> >>
> >> +# Type Aliases
> >> +QMPResponse = Dict[str, Any]
> >> +
> >> +
> >> faulthandler.enable()
> >>
> >> # This will not work if arguments contain spaces but is necessary if we
> >> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
> >> self._args.append(addr)
> >> return self
> >>
> >> - def pause_drive(self, drive, event=None):
> >> - '''Pause drive r/w operations'''
> >> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> >> + cmd = 'human-monitor-command'
> >> + kwargs = {'command-line': command_line}
> >> + if use_log:
> >> + return self.qmp_log(cmd, **kwargs)
> >> + else:
> >> + return self.qmp(cmd, **kwargs)
> >
> > Hm. I suppose I should take this chance to understand something about
> > mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
> > really returns QMPResponse. Is there some flag to make it? Like
> > --actually-check-types?
> >
>
> One of --strict's implied options, I'm not sure which. Otherwise, mypy
> is geared towards a 'gradual typing' discipline.
>
> In truth, I'm a little thankful for that because it helps avoid yak
> shaving marathons.
>
> It does mean that sometimes the annotations don't "do anything" yet,
> apart from offering hints and documentation in e.g. pycharm. Which does
> mean that sometimes they can be completely wrong...
>
> The more we add, the more we'll catch problems.
>
> Once this series is dusted I'll try to tackle more conversions for
> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for
> tests/qemu-iotests/*.py but I am trying to shepherd this one in first
> before I go bananas.
>
> > (--strict seems, well, overly strict? Like not allowing generics, I
> > don’t see why. Or I suppose for the time being we want to allow untyped
> > definitions, as long as they don’t break type assertions such as it kind
> > of does here...?)
> >
>
> "disallow-any-generics" means disallowing `Any` generics, not
> disallowing generics ... in general. (I think? I've been using mypy in
> strict mode for a personal project a lot lately and I use generics in a
> few places, it seems OK.)
--disallow-any-generics
disallow usage of generic types that do not specify explicit type parameters
So it will complain if you say just List, and you need to be explicit if
you really want List[Any]. Which I think is a reasonable thing to
require.
Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 14:00 ` Kevin Wolf
@ 2020-04-01 12:28 ` Max Reitz
2020-04-01 12:42 ` Max Reitz
2020-04-01 13:51 ` Kevin Wolf
0 siblings, 2 replies; 41+ messages in thread
From: Max Reitz @ 2020-04-01 12:28 UTC (permalink / raw)
To: Kevin Wolf; +Cc: ehabkost, qemu-block, John Snow, qemu-devel, armbru, philmd
[-- Attachment #1.1: Type: text/plain, Size: 3848 bytes --]
On 31.03.20 16:00, Kevin Wolf wrote:
> Am 31.03.2020 um 12:21 hat Max Reitz geschrieben:
>> On 31.03.20 02:00, John Snow wrote:
>>> Minor cleanup for HMP functions; helps with line length and consolidates
>>> HMP helpers through one implementation function.
>>>
>>> Although we are adding a universal toggle to turn QMP logging on or off,
>>> many existing callers to hmp functions don't expect that output to be
>>> logged, which causes quite a few changes in the test output.
>>>
>>> For now, offer a use_log parameter.
>>>
>>>
>>> Typing notes:
>>>
>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
>>> thought of as a lexical synonym.
>>>
>>> We may well wish to add stricter subtypes in the future for certain
>>> shapes of data that are not formalized as Python objects, at which point
>>> we can simply retire the alias and allow mypy to more strictly check
>>> usages of the name.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index b08bcb87e1..dfc753c319 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -37,6 +37,10 @@
>>>
>>> assert sys.version_info >= (3, 6)
>>>
>>> +# Type Aliases
>>> +QMPResponse = Dict[str, Any]
>>> +
>>> +
>>> faulthandler.enable()
>>>
>>> # This will not work if arguments contain spaces but is necessary if we
>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
>>> self._args.append(addr)
>>> return self
>>>
>>> - def pause_drive(self, drive, event=None):
>>> - '''Pause drive r/w operations'''
>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>>> + cmd = 'human-monitor-command'
>>> + kwargs = {'command-line': command_line}
>>> + if use_log:
>>> + return self.qmp_log(cmd, **kwargs)
>>> + else:
>>> + return self.qmp(cmd, **kwargs)
>>
>> Hm. I suppose I should take this chance to understand something about
>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
>> really returns QMPResponse. Is there some flag to make it? Like
>> --actually-check-types?
>
> There is --check-untyped-defs, but I'm not sure if that actually changes
> the return type of untyped functions from Any to an inferred type. I
> kind of doubt it.
Well, but Any doesn’t fit into QMPResponse, so there should be an error
reported somewhere.
>> (--strict seems, well, overly strict? Like not allowing generics, I
>> don’t see why. Or I suppose for the time being we want to allow untyped
>> definitions, as long as they don’t break type assertions such as it kind
>> of does here...?)
>
> At least, --strict does actually complain about this one because Any
> isn't good enough any more (it includes --warn-return-any):
Hm, yes, but we’re not at a point where it’s really feasible to enable
--strict...
> iotests.py:560: warning: Returning Any from function declared to return "Dict[str, Any]"
> iotests.py:560: error: Call to untyped function "qmp_log" in typed context
> iotests.py:562: warning: Returning Any from function declared to return "Dict[str, Any]"
>
> Not sure why you think it doesn't allow generics? I never had problems
> with that, even in my Python museum. :-)
I thought --disallow-any-generics would mean that. But I suppose mypy
understands a “generic” to be something else than I do, as John
described... *shrug*
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-03-31 17:39 ` Kevin Wolf
@ 2020-04-01 12:40 ` Max Reitz
2020-04-02 18:27 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Max Reitz @ 2020-04-01 12:40 UTC (permalink / raw)
To: Kevin Wolf, John Snow; +Cc: ehabkost, qemu-block, philmd, qemu-devel, armbru
[-- Attachment #1.1: Type: text/plain, Size: 4675 bytes --]
On 31.03.20 19:39, Kevin Wolf wrote:
> Am 31.03.2020 um 19:23 hat John Snow geschrieben:
>>
>>
>> On 3/31/20 6:21 AM, Max Reitz wrote:
>>> On 31.03.20 02:00, John Snow wrote:
>>>> Minor cleanup for HMP functions; helps with line length and consolidates
>>>> HMP helpers through one implementation function.
>>>>
>>>> Although we are adding a universal toggle to turn QMP logging on or off,
>>>> many existing callers to hmp functions don't expect that output to be
>>>> logged, which causes quite a few changes in the test output.
>>>>
>>>> For now, offer a use_log parameter.
>>>>
>>>>
>>>> Typing notes:
>>>>
>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
>>>> thought of as a lexical synonym.
>>>>
>>>> We may well wish to add stricter subtypes in the future for certain
>>>> shapes of data that are not formalized as Python objects, at which point
>>>> we can simply retire the alias and allow mypy to more strictly check
>>>> usages of the name.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>
>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>> index b08bcb87e1..dfc753c319 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>> @@ -37,6 +37,10 @@
>>>>
>>>> assert sys.version_info >= (3, 6)
>>>>
>>>> +# Type Aliases
>>>> +QMPResponse = Dict[str, Any]
>>>> +
>>>> +
>>>> faulthandler.enable()
>>>>
>>>> # This will not work if arguments contain spaces but is necessary if we
>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
>>>> self._args.append(addr)
>>>> return self
>>>>
>>>> - def pause_drive(self, drive, event=None):
>>>> - '''Pause drive r/w operations'''
>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>>>> + cmd = 'human-monitor-command'
>>>> + kwargs = {'command-line': command_line}
>>>> + if use_log:
>>>> + return self.qmp_log(cmd, **kwargs)
>>>> + else:
>>>> + return self.qmp(cmd, **kwargs)
>>>
>>> Hm. I suppose I should take this chance to understand something about
>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
>>> really returns QMPResponse. Is there some flag to make it? Like
>>> --actually-check-types?
>>>
>>
>> One of --strict's implied options, I'm not sure which. Otherwise, mypy
>> is geared towards a 'gradual typing' discipline.
>>
>> In truth, I'm a little thankful for that because it helps avoid yak
>> shaving marathons.
Sure. I was just looking into the different options. I was interested
in whether I could come up with a mode that leaves wholly untyped code
alone, but warns for code that mixes it. Or something.
>> It does mean that sometimes the annotations don't "do anything" yet,
>> apart from offering hints and documentation in e.g. pycharm. Which does
>> mean that sometimes they can be completely wrong...
>>
>> The more we add, the more we'll catch problems.
>>
>> Once this series is dusted I'll try to tackle more conversions for
>> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for
>> tests/qemu-iotests/*.py but I am trying to shepherd this one in first
>> before I go bananas.
Sure, sure.
>>> (--strict seems, well, overly strict? Like not allowing generics, I
>>> don’t see why. Or I suppose for the time being we want to allow untyped
>>> definitions, as long as they don’t break type assertions such as it kind
>>> of does here...?)
>>>
>>
>> "disallow-any-generics" means disallowing `Any` generics, not
>> disallowing generics ... in general. (I think? I've been using mypy in
>> strict mode for a personal project a lot lately and I use generics in a
>> few places, it seems OK.)
>
> --disallow-any-generics
> disallow usage of generic types that do not specify explicit type parameters
>
> So it will complain if you say just List, and you need to be explicit if
> you really want List[Any]. Which I think is a reasonable thing to
> require.
OK. So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”.
Not easy to parse. (Yes, yes, I should’ve actually read the man page...)
Good to know that mypy and me actually do seem to loosely agree on what
a generic is. :)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-01 12:28 ` Max Reitz
@ 2020-04-01 12:42 ` Max Reitz
2020-04-01 13:51 ` Kevin Wolf
1 sibling, 0 replies; 41+ messages in thread
From: Max Reitz @ 2020-04-01 12:42 UTC (permalink / raw)
To: Kevin Wolf; +Cc: ehabkost, qemu-block, John Snow, qemu-devel, armbru, philmd
[-- Attachment #1.1: Type: text/plain, Size: 4199 bytes --]
On 01.04.20 14:28, Max Reitz wrote:
> On 31.03.20 16:00, Kevin Wolf wrote:
>> Am 31.03.2020 um 12:21 hat Max Reitz geschrieben:
>>> On 31.03.20 02:00, John Snow wrote:
>>>> Minor cleanup for HMP functions; helps with line length and consolidates
>>>> HMP helpers through one implementation function.
>>>>
>>>> Although we are adding a universal toggle to turn QMP logging on or off,
>>>> many existing callers to hmp functions don't expect that output to be
>>>> logged, which causes quite a few changes in the test output.
>>>>
>>>> For now, offer a use_log parameter.
>>>>
>>>>
>>>> Typing notes:
>>>>
>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
>>>> thought of as a lexical synonym.
>>>>
>>>> We may well wish to add stricter subtypes in the future for certain
>>>> shapes of data that are not formalized as Python objects, at which point
>>>> we can simply retire the alias and allow mypy to more strictly check
>>>> usages of the name.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>
>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>> index b08bcb87e1..dfc753c319 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>> @@ -37,6 +37,10 @@
>>>>
>>>> assert sys.version_info >= (3, 6)
>>>>
>>>> +# Type Aliases
>>>> +QMPResponse = Dict[str, Any]
>>>> +
>>>> +
>>>> faulthandler.enable()
>>>>
>>>> # This will not work if arguments contain spaces but is necessary if we
>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
>>>> self._args.append(addr)
>>>> return self
>>>>
>>>> - def pause_drive(self, drive, event=None):
>>>> - '''Pause drive r/w operations'''
>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>>>> + cmd = 'human-monitor-command'
>>>> + kwargs = {'command-line': command_line}
>>>> + if use_log:
>>>> + return self.qmp_log(cmd, **kwargs)
>>>> + else:
>>>> + return self.qmp(cmd, **kwargs)
>>>
>>> Hm. I suppose I should take this chance to understand something about
>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
>>> really returns QMPResponse. Is there some flag to make it? Like
>>> --actually-check-types?
>>
>> There is --check-untyped-defs, but I'm not sure if that actually changes
>> the return type of untyped functions from Any to an inferred type. I
>> kind of doubt it.
>
> Well, but Any doesn’t fit into QMPResponse, so there should be an error
> reported somewhere.
>
>>> (--strict seems, well, overly strict? Like not allowing generics, I
>>> don’t see why. Or I suppose for the time being we want to allow untyped
>>> definitions, as long as they don’t break type assertions such as it kind
>>> of does here...?)
>>
>> At least, --strict does actually complain about this one because Any
>> isn't good enough any more (it includes --warn-return-any):
>
> Hm, yes, but we’re not at a point where it’s really feasible to enable
> --strict...
>
>> iotests.py:560: warning: Returning Any from function declared to return "Dict[str, Any]"
>> iotests.py:560: error: Call to untyped function "qmp_log" in typed context
>> iotests.py:562: warning: Returning Any from function declared to return "Dict[str, Any]"
>>
>> Not sure why you think it doesn't allow generics? I never had problems
>> with that, even in my Python museum. :-)
>
> I thought --disallow-any-generics would mean that. But I suppose mypy
> understands a “generic” to be something else than I do, as John
> described... *shrug*
Oh. John didn’t describe that. I just read the “Any” thing wrong,
again. (On my first read, I thought he just used the back ticks to
stress the word “any”, not to refer to the type “Any”.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-01 12:28 ` Max Reitz
2020-04-01 12:42 ` Max Reitz
@ 2020-04-01 13:51 ` Kevin Wolf
2020-04-01 14:53 ` Max Reitz
1 sibling, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-04-01 13:51 UTC (permalink / raw)
To: Max Reitz; +Cc: ehabkost, qemu-block, John Snow, qemu-devel, armbru, philmd
[-- Attachment #1: Type: text/plain, Size: 3661 bytes --]
Am 01.04.2020 um 14:28 hat Max Reitz geschrieben:
> On 31.03.20 16:00, Kevin Wolf wrote:
> > Am 31.03.2020 um 12:21 hat Max Reitz geschrieben:
> >> On 31.03.20 02:00, John Snow wrote:
> >>> Minor cleanup for HMP functions; helps with line length and consolidates
> >>> HMP helpers through one implementation function.
> >>>
> >>> Although we are adding a universal toggle to turn QMP logging on or off,
> >>> many existing callers to hmp functions don't expect that output to be
> >>> logged, which causes quite a few changes in the test output.
> >>>
> >>> For now, offer a use_log parameter.
> >>>
> >>>
> >>> Typing notes:
> >>>
> >>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
> >>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
> >>> thought of as a lexical synonym.
> >>>
> >>> We may well wish to add stricter subtypes in the future for certain
> >>> shapes of data that are not formalized as Python objects, at which point
> >>> we can simply retire the alias and allow mypy to more strictly check
> >>> usages of the name.
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>> ---
> >>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
> >>> 1 file changed, 22 insertions(+), 13 deletions(-)
> >>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>
> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >>> index b08bcb87e1..dfc753c319 100644
> >>> --- a/tests/qemu-iotests/iotests.py
> >>> +++ b/tests/qemu-iotests/iotests.py
> >>> @@ -37,6 +37,10 @@
> >>>
> >>> assert sys.version_info >= (3, 6)
> >>>
> >>> +# Type Aliases
> >>> +QMPResponse = Dict[str, Any]
> >>> +
> >>> +
> >>> faulthandler.enable()
> >>>
> >>> # This will not work if arguments contain spaces but is necessary if we
> >>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
> >>> self._args.append(addr)
> >>> return self
> >>>
> >>> - def pause_drive(self, drive, event=None):
> >>> - '''Pause drive r/w operations'''
> >>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> >>> + cmd = 'human-monitor-command'
> >>> + kwargs = {'command-line': command_line}
> >>> + if use_log:
> >>> + return self.qmp_log(cmd, **kwargs)
> >>> + else:
> >>> + return self.qmp(cmd, **kwargs)
> >>
> >> Hm. I suppose I should take this chance to understand something about
> >> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
> >> really returns QMPResponse. Is there some flag to make it? Like
> >> --actually-check-types?
> >
> > There is --check-untyped-defs, but I'm not sure if that actually changes
> > the return type of untyped functions from Any to an inferred type. I
> > kind of doubt it.
>
> Well, but Any doesn’t fit into QMPResponse, so there should be an error
> reported somewhere.
Any is the void* of Python typing. It's compatible with everything else,
in both directions.
> >> (--strict seems, well, overly strict? Like not allowing generics, I
> >> don’t see why. Or I suppose for the time being we want to allow untyped
> >> definitions, as long as they don’t break type assertions such as it kind
> >> of does here...?)
> >
> > At least, --strict does actually complain about this one because Any
> > isn't good enough any more (it includes --warn-return-any):
>
> Hm, yes, but we’re not at a point where it’s really feasible to enable
> --strict...
We're not yet, but I think it's a reasonable goal.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-01 13:51 ` Kevin Wolf
@ 2020-04-01 14:53 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2020-04-01 14:53 UTC (permalink / raw)
To: Kevin Wolf; +Cc: ehabkost, qemu-block, John Snow, qemu-devel, armbru, philmd
[-- Attachment #1.1: Type: text/plain, Size: 3199 bytes --]
On 01.04.20 15:51, Kevin Wolf wrote:
> Am 01.04.2020 um 14:28 hat Max Reitz geschrieben:
>> On 31.03.20 16:00, Kevin Wolf wrote:
>>> Am 31.03.2020 um 12:21 hat Max Reitz geschrieben:
>>>> On 31.03.20 02:00, John Snow wrote:
>>>>> Minor cleanup for HMP functions; helps with line length and consolidates
>>>>> HMP helpers through one implementation function.
>>>>>
>>>>> Although we are adding a universal toggle to turn QMP logging on or off,
>>>>> many existing callers to hmp functions don't expect that output to be
>>>>> logged, which causes quite a few changes in the test output.
>>>>>
>>>>> For now, offer a use_log parameter.
>>>>>
>>>>>
>>>>> Typing notes:
>>>>>
>>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
>>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
>>>>> thought of as a lexical synonym.
>>>>>
>>>>> We may well wish to add stricter subtypes in the future for certain
>>>>> shapes of data that are not formalized as Python objects, at which point
>>>>> we can simply retire the alias and allow mypy to more strictly check
>>>>> usages of the name.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
>>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>
>>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>>> index b08bcb87e1..dfc753c319 100644
>>>>> --- a/tests/qemu-iotests/iotests.py
>>>>> +++ b/tests/qemu-iotests/iotests.py
>>>>> @@ -37,6 +37,10 @@
>>>>>
>>>>> assert sys.version_info >= (3, 6)
>>>>>
>>>>> +# Type Aliases
>>>>> +QMPResponse = Dict[str, Any]
>>>>> +
>>>>> +
>>>>> faulthandler.enable()
>>>>>
>>>>> # This will not work if arguments contain spaces but is necessary if we
>>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
>>>>> self._args.append(addr)
>>>>> return self
>>>>>
>>>>> - def pause_drive(self, drive, event=None):
>>>>> - '''Pause drive r/w operations'''
>>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>>>>> + cmd = 'human-monitor-command'
>>>>> + kwargs = {'command-line': command_line}
>>>>> + if use_log:
>>>>> + return self.qmp_log(cmd, **kwargs)
>>>>> + else:
>>>>> + return self.qmp(cmd, **kwargs)
>>>>
>>>> Hm. I suppose I should take this chance to understand something about
>>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
>>>> really returns QMPResponse. Is there some flag to make it? Like
>>>> --actually-check-types?
>>>
>>> There is --check-untyped-defs, but I'm not sure if that actually changes
>>> the return type of untyped functions from Any to an inferred type. I
>>> kind of doubt it.
>>
>> Well, but Any doesn’t fit into QMPResponse, so there should be an error
>> reported somewhere.
>
> Any is the void* of Python typing. It's compatible with everything else,
> in both directions.
void* is handled differently by C and by C++.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-01 12:40 ` Max Reitz
@ 2020-04-02 18:27 ` John Snow
2020-04-03 7:46 ` Kevin Wolf
2020-04-03 8:57 ` Max Reitz
0 siblings, 2 replies; 41+ messages in thread
From: John Snow @ 2020-04-02 18:27 UTC (permalink / raw)
To: Max Reitz, Kevin Wolf; +Cc: ehabkost, qemu-block, philmd, qemu-devel, armbru
On 4/1/20 8:40 AM, Max Reitz wrote:
> On 31.03.20 19:39, Kevin Wolf wrote:
>> Am 31.03.2020 um 19:23 hat John Snow geschrieben:
>>>
>>>
>>> On 3/31/20 6:21 AM, Max Reitz wrote:
>>>> On 31.03.20 02:00, John Snow wrote:
>>>>> Minor cleanup for HMP functions; helps with line length and consolidates
>>>>> HMP helpers through one implementation function.
>>>>>
>>>>> Although we are adding a universal toggle to turn QMP logging on or off,
>>>>> many existing callers to hmp functions don't expect that output to be
>>>>> logged, which causes quite a few changes in the test output.
>>>>>
>>>>> For now, offer a use_log parameter.
>>>>>
>>>>>
>>>>> Typing notes:
>>>>>
>>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
>>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
>>>>> thought of as a lexical synonym.
>>>>>
>>>>> We may well wish to add stricter subtypes in the future for certain
>>>>> shapes of data that are not formalized as Python objects, at which point
>>>>> we can simply retire the alias and allow mypy to more strictly check
>>>>> usages of the name.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
>>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
>>>>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>
>>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>>> index b08bcb87e1..dfc753c319 100644
>>>>> --- a/tests/qemu-iotests/iotests.py
>>>>> +++ b/tests/qemu-iotests/iotests.py
>>>>> @@ -37,6 +37,10 @@
>>>>>
>>>>> assert sys.version_info >= (3, 6)
>>>>>
>>>>> +# Type Aliases
>>>>> +QMPResponse = Dict[str, Any]
>>>>> +
>>>>> +
>>>>> faulthandler.enable()
>>>>>
>>>>> # This will not work if arguments contain spaces but is necessary if we
>>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
>>>>> self._args.append(addr)
>>>>> return self
>>>>>
>>>>> - def pause_drive(self, drive, event=None):
>>>>> - '''Pause drive r/w operations'''
>>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>>>>> + cmd = 'human-monitor-command'
>>>>> + kwargs = {'command-line': command_line}
>>>>> + if use_log:
>>>>> + return self.qmp_log(cmd, **kwargs)
>>>>> + else:
>>>>> + return self.qmp(cmd, **kwargs)
>>>>
>>>> Hm. I suppose I should take this chance to understand something about
>>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
>>>> really returns QMPResponse. Is there some flag to make it? Like
>>>> --actually-check-types?
>>>>
>>>
>>> One of --strict's implied options, I'm not sure which. Otherwise, mypy
>>> is geared towards a 'gradual typing' discipline.
>>>
>>> In truth, I'm a little thankful for that because it helps avoid yak
>>> shaving marathons.
>
> Sure. I was just looking into the different options. I was interested
> in whether I could come up with a mode that leaves wholly untyped code
> alone, but warns for code that mixes it. Or something.
>
>>> It does mean that sometimes the annotations don't "do anything" yet,
>>> apart from offering hints and documentation in e.g. pycharm. Which does
>>> mean that sometimes they can be completely wrong...
>>>
>>> The more we add, the more we'll catch problems.
>>>
>>> Once this series is dusted I'll try to tackle more conversions for
>>> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for
>>> tests/qemu-iotests/*.py but I am trying to shepherd this one in first
>>> before I go bananas.
>
> Sure, sure.
>
>>>> (--strict seems, well, overly strict? Like not allowing generics, I
>>>> don’t see why. Or I suppose for the time being we want to allow untyped
>>>> definitions, as long as they don’t break type assertions such as it kind
>>>> of does here...?)
>>>>
>>>
>>> "disallow-any-generics" means disallowing `Any` generics, not
>>> disallowing generics ... in general. (I think? I've been using mypy in
>>> strict mode for a personal project a lot lately and I use generics in a
>>> few places, it seems OK.)
>>
>> --disallow-any-generics
>> disallow usage of generic types that do not specify explicit type parameters
>>
>> So it will complain if you say just List, and you need to be explicit if
>> you really want List[Any]. Which I think is a reasonable thing to
>> require.
>
> OK. So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”.
> Not easy to parse. (Yes, yes, I should’ve actually read the man page...)
>
> Good to know that mypy and me actually do seem to loosely agree on what
> a generic is. :)
>
> Max
>
Are we squared up for this series? I am actually not sure.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-02 18:27 ` John Snow
@ 2020-04-03 7:46 ` Kevin Wolf
2020-04-03 8:57 ` Max Reitz
1 sibling, 0 replies; 41+ messages in thread
From: Kevin Wolf @ 2020-04-03 7:46 UTC (permalink / raw)
To: John Snow; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
Am 02.04.2020 um 20:27 hat John Snow geschrieben:
> On 4/1/20 8:40 AM, Max Reitz wrote:
> > On 31.03.20 19:39, Kevin Wolf wrote:
> >> Am 31.03.2020 um 19:23 hat John Snow geschrieben:
> >>>
> >>>
> >>> On 3/31/20 6:21 AM, Max Reitz wrote:
> >>>> On 31.03.20 02:00, John Snow wrote:
> >>>>> Minor cleanup for HMP functions; helps with line length and consolidates
> >>>>> HMP helpers through one implementation function.
> >>>>>
> >>>>> Although we are adding a universal toggle to turn QMP logging on or off,
> >>>>> many existing callers to hmp functions don't expect that output to be
> >>>>> logged, which causes quite a few changes in the test output.
> >>>>>
> >>>>> For now, offer a use_log parameter.
> >>>>>
> >>>>>
> >>>>> Typing notes:
> >>>>>
> >>>>> QMPResponse is just an alias for Dict[str, Any]. It holds no special
> >>>>> meanings and it is not a formal subtype of Dict[str, Any]. It is best
> >>>>> thought of as a lexical synonym.
> >>>>>
> >>>>> We may well wish to add stricter subtypes in the future for certain
> >>>>> shapes of data that are not formalized as Python objects, at which point
> >>>>> we can simply retire the alias and allow mypy to more strictly check
> >>>>> usages of the name.
> >>>>>
> >>>>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>>>> ---
> >>>>> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
> >>>>> 1 file changed, 22 insertions(+), 13 deletions(-)
> >>>>
> >>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>>>
> >>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >>>>> index b08bcb87e1..dfc753c319 100644
> >>>>> --- a/tests/qemu-iotests/iotests.py
> >>>>> +++ b/tests/qemu-iotests/iotests.py
> >>>>> @@ -37,6 +37,10 @@
> >>>>>
> >>>>> assert sys.version_info >= (3, 6)
> >>>>>
> >>>>> +# Type Aliases
> >>>>> +QMPResponse = Dict[str, Any]
> >>>>> +
> >>>>> +
> >>>>> faulthandler.enable()
> >>>>>
> >>>>> # This will not work if arguments contain spaces but is necessary if we
> >>>>> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
> >>>>> self._args.append(addr)
> >>>>> return self
> >>>>>
> >>>>> - def pause_drive(self, drive, event=None):
> >>>>> - '''Pause drive r/w operations'''
> >>>>> + def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> >>>>> + cmd = 'human-monitor-command'
> >>>>> + kwargs = {'command-line': command_line}
> >>>>> + if use_log:
> >>>>> + return self.qmp_log(cmd, **kwargs)
> >>>>> + else:
> >>>>> + return self.qmp(cmd, **kwargs)
> >>>>
> >>>> Hm. I suppose I should take this chance to understand something about
> >>>> mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
> >>>> really returns QMPResponse. Is there some flag to make it? Like
> >>>> --actually-check-types?
> >>>>
> >>>
> >>> One of --strict's implied options, I'm not sure which. Otherwise, mypy
> >>> is geared towards a 'gradual typing' discipline.
> >>>
> >>> In truth, I'm a little thankful for that because it helps avoid yak
> >>> shaving marathons.
> >
> > Sure. I was just looking into the different options. I was interested
> > in whether I could come up with a mode that leaves wholly untyped code
> > alone, but warns for code that mixes it. Or something.
> >
> >>> It does mean that sometimes the annotations don't "do anything" yet,
> >>> apart from offering hints and documentation in e.g. pycharm. Which does
> >>> mean that sometimes they can be completely wrong...
> >>>
> >>> The more we add, the more we'll catch problems.
> >>>
> >>> Once this series is dusted I'll try to tackle more conversions for
> >>> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for
> >>> tests/qemu-iotests/*.py but I am trying to shepherd this one in first
> >>> before I go bananas.
> >
> > Sure, sure.
> >
> >>>> (--strict seems, well, overly strict? Like not allowing generics, I
> >>>> don’t see why. Or I suppose for the time being we want to allow untyped
> >>>> definitions, as long as they don’t break type assertions such as it kind
> >>>> of does here...?)
> >>>>
> >>>
> >>> "disallow-any-generics" means disallowing `Any` generics, not
> >>> disallowing generics ... in general. (I think? I've been using mypy in
> >>> strict mode for a personal project a lot lately and I use generics in a
> >>> few places, it seems OK.)
> >>
> >> --disallow-any-generics
> >> disallow usage of generic types that do not specify explicit type parameters
> >>
> >> So it will complain if you say just List, and you need to be explicit if
> >> you really want List[Any]. Which I think is a reasonable thing to
> >> require.
> >
> > OK. So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”.
> > Not easy to parse. (Yes, yes, I should’ve actually read the man page...)
> >
> > Good to know that mypy and me actually do seem to loosely agree on what
> > a generic is. :)
> >
> > Max
> >
>
> Are we squared up for this series? I am actually not sure.
I had a comment in patch 14 which you may or may not want to address (my
R-b was unconditional). I think everything else was just tangential
discussion.
Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-02 18:27 ` John Snow
2020-04-03 7:46 ` Kevin Wolf
@ 2020-04-03 8:57 ` Max Reitz
2020-04-04 2:38 ` John Snow
1 sibling, 1 reply; 41+ messages in thread
From: Max Reitz @ 2020-04-03 8:57 UTC (permalink / raw)
To: John Snow, Kevin Wolf; +Cc: ehabkost, qemu-block, philmd, qemu-devel, armbru
[-- Attachment #1.1: Type: text/plain, Size: 200 bytes --]
On 02.04.20 20:27, John Snow wrote:
[...]
> Are we squared up for this series? I am actually not sure.
As far as I’m concerned, yes. I just had this question on how to use mypy.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 10/14] iotests: add hmp helper with logging
2020-04-03 8:57 ` Max Reitz
@ 2020-04-04 2:38 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-04-04 2:38 UTC (permalink / raw)
To: Max Reitz, Kevin Wolf; +Cc: ehabkost, qemu-block, philmd, qemu-devel, armbru
On 4/3/20 4:57 AM, Max Reitz wrote:
> On 02.04.20 20:27, John Snow wrote:
>
> [...]
>
>> Are we squared up for this series? I am actually not sure.
>
> As far as I’m concerned, yes. I just had this question on how to use mypy.
Oh, whoops, Kevin's comment.
I do want to address that one.
Sorry, I am a bit distracted right now. The last few weeks have been
tough. Having a hard time keeping track of things right now.
--js
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 00/14] iotests: use python logging
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
` (14 preceding siblings ...)
2020-03-31 15:07 ` [PATCH v10 00/14] iotests: use python logging Kevin Wolf
@ 2020-04-28 11:46 ` Max Reitz
2020-04-28 12:21 ` Kevin Wolf
15 siblings, 1 reply; 41+ messages in thread
From: Max Reitz @ 2020-04-28 11:46 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, ehabkost, qemu-block, philmd, armbru
[-- Attachment #1.1: Type: text/plain, Size: 768 bytes --]
On 31.03.20 02:00, John Snow wrote:
> This series uses python logging to enable output conditionally on
> iotests.log(). We unify an initialization call (which also enables
> debugging output for those tests with -d) and then make the switch
> inside of iotests.
>
> It will help alleviate the need to create logged/unlogged versions
> of all the various helpers we have made.
>
> Also, I got lost and accidentally delinted iotests while I was here.
> Sorry about that. By version 9, it's now the overwhelming focus of
> this series. No good deed, etc.
Seems like nobody else wants it, so I thank you and let you know that
I’ve applied this series to my block-next branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 00/14] iotests: use python logging
2020-04-28 11:46 ` Max Reitz
@ 2020-04-28 12:21 ` Kevin Wolf
2020-04-28 17:36 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-04-28 12:21 UTC (permalink / raw)
To: Max Reitz; +Cc: ehabkost, qemu-block, John Snow, qemu-devel, armbru, philmd
[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]
Am 28.04.2020 um 13:46 hat Max Reitz geschrieben:
> On 31.03.20 02:00, John Snow wrote:
> > This series uses python logging to enable output conditionally on
> > iotests.log(). We unify an initialization call (which also enables
> > debugging output for those tests with -d) and then make the switch
> > inside of iotests.
> >
> > It will help alleviate the need to create logged/unlogged versions
> > of all the various helpers we have made.
> >
> > Also, I got lost and accidentally delinted iotests while I was here.
> > Sorry about that. By version 9, it's now the overwhelming focus of
> > this series. No good deed, etc.
>
> Seems like nobody else wants it, so I thank you and let you know that
> I’ve applied this series to my block-next branch:
>
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next
John said he wanted to address my comment on patch 14, so I expected him
to send another version. This need not stop this series (we can still
fix that on top), but just as an explanation why I didn't take it yet.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 00/14] iotests: use python logging
2020-04-28 12:21 ` Kevin Wolf
@ 2020-04-28 17:36 ` John Snow
2020-04-29 8:08 ` Max Reitz
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-04-28 17:36 UTC (permalink / raw)
To: Kevin Wolf, Max Reitz; +Cc: ehabkost, qemu-block, philmd, qemu-devel, armbru
On 4/28/20 8:21 AM, Kevin Wolf wrote:
> Am 28.04.2020 um 13:46 hat Max Reitz geschrieben:
>> On 31.03.20 02:00, John Snow wrote:
>>> This series uses python logging to enable output conditionally on
>>> iotests.log(). We unify an initialization call (which also enables
>>> debugging output for those tests with -d) and then make the switch
>>> inside of iotests.
>>>
>>> It will help alleviate the need to create logged/unlogged versions
>>> of all the various helpers we have made.
>>>
>>> Also, I got lost and accidentally delinted iotests while I was here.
>>> Sorry about that. By version 9, it's now the overwhelming focus of
>>> this series. No good deed, etc.
>>
>> Seems like nobody else wants it, so I thank you and let you know that
>> I’ve applied this series to my block-next branch:
>>
>> https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next
>
> John said he wanted to address my comment on patch 14, so I expected him
> to send another version. This need not stop this series (we can still
> fix that on top), but just as an explanation why I didn't take it yet.
>
> Kevin
>
Sorry, foggy memory. I will send a follow-up and we can try to squash it in.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 00/14] iotests: use python logging
2020-04-28 17:36 ` John Snow
@ 2020-04-29 8:08 ` Max Reitz
0 siblings, 0 replies; 41+ messages in thread
From: Max Reitz @ 2020-04-29 8:08 UTC (permalink / raw)
To: John Snow, Kevin Wolf; +Cc: ehabkost, qemu-block, philmd, qemu-devel, armbru
[-- Attachment #1.1: Type: text/plain, Size: 1307 bytes --]
On 28.04.20 19:36, John Snow wrote:
>
>
> On 4/28/20 8:21 AM, Kevin Wolf wrote:
>> Am 28.04.2020 um 13:46 hat Max Reitz geschrieben:
>>> On 31.03.20 02:00, John Snow wrote:
>>>> This series uses python logging to enable output conditionally on
>>>> iotests.log(). We unify an initialization call (which also enables
>>>> debugging output for those tests with -d) and then make the switch
>>>> inside of iotests.
>>>>
>>>> It will help alleviate the need to create logged/unlogged versions
>>>> of all the various helpers we have made.
>>>>
>>>> Also, I got lost and accidentally delinted iotests while I was here.
>>>> Sorry about that. By version 9, it's now the overwhelming focus of
>>>> this series. No good deed, etc.
>>>
>>> Seems like nobody else wants it, so I thank you and let you know that
>>> I’ve applied this series to my block-next branch:
>>>
>>> https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next
>>
>> John said he wanted to address my comment on patch 14, so I expected him
>> to send another version. This need not stop this series (we can still
>> fix that on top), but just as an explanation why I didn't take it yet.
>>
>> Kevin
>>
>
> Sorry, foggy memory. I will send a follow-up and we can try to squash it in.
All right.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-03-31 13:44 ` Kevin Wolf
@ 2020-05-14 6:24 ` John Snow
2020-05-14 10:06 ` Kevin Wolf
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-05-14 6:24 UTC (permalink / raw)
To: Kevin Wolf; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
On 3/31/20 9:44 AM, Kevin Wolf wrote:
> Am 31.03.2020 um 02:00 hat John Snow geschrieben:
>> We can turn logging on/off globally instead of per-function.
>>
>> Remove use_log from run_job, and use python logging to turn on
>> diffable output when we run through a script entry point.
>>
>> iotest 245 changes output order due to buffering reasons.
>>
>>
>> An extended note on python logging:
>>
>> A NullHandler is added to `qemu.iotests` to stop output from being
>> generated if this code is used as a library without configuring logging.
>> A NullHandler is only needed at the root, so a duplicate handler is not
>> needed for `qemu.iotests.diff_io`.
>>
>> When logging is not configured, messages at the 'WARNING' levels or
>> above are printed with default settings. The NullHandler stops this from
>> occurring, which is considered good hygiene for code used as a library.
>>
>> See https://docs.python.org/3/howto/logging.html#library-config
>>
>> When logging is actually enabled (always at the behest of an explicit
>> call by a client script), a root logger is implicitly created at the
>> root, which allows messages to propagate upwards and be handled/emitted
>> from the root logger with default settings.
>>
>> When we want iotest logging, we attach a handler to the
>> qemu.iotests.diff_io logger and disable propagation to avoid possible
>> double-printing.
>>
>> For more information on python logging infrastructure, I highly
>> recommend downloading the pip package `logging_tree`, which provides
>> convenient visualizations of the hierarchical logging configuration
>> under different circumstances.
>>
>> See https://pypi.org/project/logging_tree/ for more information.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Should we enable logger if -d is given?
>
> Previously we had:
>
> $ ./check -d -T -raw 281
> [...]
> 281 not run: not suitable for this image format: raw
> 281 not run [15:39:03] [15:39:04] not suitable for this image format: raw
> Not run: 281
>
> After this series, the first line of output from notrun() is missing.
> Not that I think it's important to have the line, but as long as we
> bother to call logger.warning(), I thought that maybe we want to be able
> to actually see the effect of it somehwere?
>
> Kevin
>
Uh, okay. So this is weirder than I thought it was going to be!
So, if you move the debug configuration up above the _verify calls,
you'll see the message printed out to the debug stream:
DEBUG:qemu.iotests:iotests debugging messages active
WARNING:qemu.iotests:281 not run: not suitable for this image format: raw
...but if you omit the `-d` flag, the message vanishes into a black
hole. Did it always work like that ...?
(I'll keep looking. --js)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-05-14 6:24 ` John Snow
@ 2020-05-14 10:06 ` Kevin Wolf
2020-05-14 19:54 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-05-14 10:06 UTC (permalink / raw)
To: John Snow; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
Am 14.05.2020 um 08:24 hat John Snow geschrieben:
> On 3/31/20 9:44 AM, Kevin Wolf wrote:
> > Am 31.03.2020 um 02:00 hat John Snow geschrieben:
> >> We can turn logging on/off globally instead of per-function.
> >>
> >> Remove use_log from run_job, and use python logging to turn on
> >> diffable output when we run through a script entry point.
> >>
> >> iotest 245 changes output order due to buffering reasons.
> >>
> >>
> >> An extended note on python logging:
> >>
> >> A NullHandler is added to `qemu.iotests` to stop output from being
> >> generated if this code is used as a library without configuring logging.
> >> A NullHandler is only needed at the root, so a duplicate handler is not
> >> needed for `qemu.iotests.diff_io`.
> >>
> >> When logging is not configured, messages at the 'WARNING' levels or
> >> above are printed with default settings. The NullHandler stops this from
> >> occurring, which is considered good hygiene for code used as a library.
> >>
> >> See https://docs.python.org/3/howto/logging.html#library-config
> >>
> >> When logging is actually enabled (always at the behest of an explicit
> >> call by a client script), a root logger is implicitly created at the
> >> root, which allows messages to propagate upwards and be handled/emitted
> >> from the root logger with default settings.
> >>
> >> When we want iotest logging, we attach a handler to the
> >> qemu.iotests.diff_io logger and disable propagation to avoid possible
> >> double-printing.
> >>
> >> For more information on python logging infrastructure, I highly
> >> recommend downloading the pip package `logging_tree`, which provides
> >> convenient visualizations of the hierarchical logging configuration
> >> under different circumstances.
> >>
> >> See https://pypi.org/project/logging_tree/ for more information.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >
> > Should we enable logger if -d is given?
> >
> > Previously we had:
> >
> > $ ./check -d -T -raw 281
> > [...]
> > 281 not run: not suitable for this image format: raw
> > 281 not run [15:39:03] [15:39:04] not suitable for this image format: raw
> > Not run: 281
> >
> > After this series, the first line of output from notrun() is missing.
> > Not that I think it's important to have the line, but as long as we
> > bother to call logger.warning(), I thought that maybe we want to be able
> > to actually see the effect of it somehwere?
> >
> > Kevin
> >
>
> Uh, okay. So this is weirder than I thought it was going to be!
>
> So, if you move the debug configuration up above the _verify calls,
> you'll see the message printed out to the debug stream:
>
> DEBUG:qemu.iotests:iotests debugging messages active
> WARNING:qemu.iotests:281 not run: not suitable for this image format: raw
>
> ...but if you omit the `-d` flag, the message vanishes into a black
> hole. Did it always work like that ...?
Yes, this is how it used to work. It's a result of ./check only printing
the test output with -d, and such log messages are basically just test
output.
And I think it's exactly what we want: Without -d, you want only the
summary, i.e. a single line that says "pass", "fail" or "notrun",
potentially with a small note at the end of the line, but that's it.
Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-05-14 10:06 ` Kevin Wolf
@ 2020-05-14 19:54 ` John Snow
2020-05-15 9:03 ` Kevin Wolf
0 siblings, 1 reply; 41+ messages in thread
From: John Snow @ 2020-05-14 19:54 UTC (permalink / raw)
To: Kevin Wolf; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
On 5/14/20 6:06 AM, Kevin Wolf wrote:
> Am 14.05.2020 um 08:24 hat John Snow geschrieben:
>> On 3/31/20 9:44 AM, Kevin Wolf wrote:
>>> Am 31.03.2020 um 02:00 hat John Snow geschrieben:
>>>> We can turn logging on/off globally instead of per-function.
>>>>
>>>> Remove use_log from run_job, and use python logging to turn on
>>>> diffable output when we run through a script entry point.
>>>>
>>>> iotest 245 changes output order due to buffering reasons.
>>>>
>>>>
>>>> An extended note on python logging:
>>>>
>>>> A NullHandler is added to `qemu.iotests` to stop output from being
>>>> generated if this code is used as a library without configuring logging.
>>>> A NullHandler is only needed at the root, so a duplicate handler is not
>>>> needed for `qemu.iotests.diff_io`.
>>>>
>>>> When logging is not configured, messages at the 'WARNING' levels or
>>>> above are printed with default settings. The NullHandler stops this from
>>>> occurring, which is considered good hygiene for code used as a library.
>>>>
>>>> See https://docs.python.org/3/howto/logging.html#library-config
>>>>
>>>> When logging is actually enabled (always at the behest of an explicit
>>>> call by a client script), a root logger is implicitly created at the
>>>> root, which allows messages to propagate upwards and be handled/emitted
>>>> from the root logger with default settings.
>>>>
>>>> When we want iotest logging, we attach a handler to the
>>>> qemu.iotests.diff_io logger and disable propagation to avoid possible
>>>> double-printing.
>>>>
>>>> For more information on python logging infrastructure, I highly
>>>> recommend downloading the pip package `logging_tree`, which provides
>>>> convenient visualizations of the hierarchical logging configuration
>>>> under different circumstances.
>>>>
>>>> See https://pypi.org/project/logging_tree/ for more information.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>
>>> Should we enable logger if -d is given?
>>>
>>> Previously we had:
>>>
>>> $ ./check -d -T -raw 281
>>> [...]
>>> 281 not run: not suitable for this image format: raw
>>> 281 not run [15:39:03] [15:39:04] not suitable for this image format: raw
>>> Not run: 281
>>>
>>> After this series, the first line of output from notrun() is missing.
>>> Not that I think it's important to have the line, but as long as we
>>> bother to call logger.warning(), I thought that maybe we want to be able
>>> to actually see the effect of it somehwere?
>>>
>>> Kevin
>>>
>>
>> Uh, okay. So this is weirder than I thought it was going to be!
>>
>> So, if you move the debug configuration up above the _verify calls,
>> you'll see the message printed out to the debug stream:
>>
>> DEBUG:qemu.iotests:iotests debugging messages active
>> WARNING:qemu.iotests:281 not run: not suitable for this image format: raw
>>
>> ...but if you omit the `-d` flag, the message vanishes into a black
>> hole. Did it always work like that ...?
>
> Yes, this is how it used to work. It's a result of ./check only printing
> the test output with -d, and such log messages are basically just test
> output.
>
> And I think it's exactly what we want: Without -d, you want only the
> summary, i.e. a single line that says "pass", "fail" or "notrun",
> potentially with a small note at the end of the line, but that's it.
>
OK, maybe. So I guess what happens here is that if you don't use -d, the
output gets redirected to file, and that file is summarily deleted.
Your phrase "but as long as we bother to call logger.warning(), I
thought that maybe we want to be able to actually see the effect of it
somewhere" stuck with me -- I think you're right.
I kind of do expect that if I call a function called warning() that it's
gonna do some damage. principle of least surprise, etc.
So two things:
(1) Maybe the iotest logger ought to always use stderr, and we should
see any calls to warning() or error() even when debugging is off.
(2) These skip notifications are not warnings, they are informational
and can be disabled when `-d` is omitted. (Especially because they are
represented through another channel.)
--js
(I'll send the fixup for the simpler thing first, and you can take or
leave the second thing.)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-05-14 19:54 ` John Snow
@ 2020-05-15 9:03 ` Kevin Wolf
2020-05-18 18:29 ` John Snow
0 siblings, 1 reply; 41+ messages in thread
From: Kevin Wolf @ 2020-05-15 9:03 UTC (permalink / raw)
To: John Snow; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
Am 14.05.2020 um 21:54 hat John Snow geschrieben:
>
>
> On 5/14/20 6:06 AM, Kevin Wolf wrote:
> > Am 14.05.2020 um 08:24 hat John Snow geschrieben:
> >> On 3/31/20 9:44 AM, Kevin Wolf wrote:
> >>> Am 31.03.2020 um 02:00 hat John Snow geschrieben:
> >>>> We can turn logging on/off globally instead of per-function.
> >>>>
> >>>> Remove use_log from run_job, and use python logging to turn on
> >>>> diffable output when we run through a script entry point.
> >>>>
> >>>> iotest 245 changes output order due to buffering reasons.
> >>>>
> >>>>
> >>>> An extended note on python logging:
> >>>>
> >>>> A NullHandler is added to `qemu.iotests` to stop output from being
> >>>> generated if this code is used as a library without configuring logging.
> >>>> A NullHandler is only needed at the root, so a duplicate handler is not
> >>>> needed for `qemu.iotests.diff_io`.
> >>>>
> >>>> When logging is not configured, messages at the 'WARNING' levels or
> >>>> above are printed with default settings. The NullHandler stops this from
> >>>> occurring, which is considered good hygiene for code used as a library.
> >>>>
> >>>> See https://docs.python.org/3/howto/logging.html#library-config
> >>>>
> >>>> When logging is actually enabled (always at the behest of an explicit
> >>>> call by a client script), a root logger is implicitly created at the
> >>>> root, which allows messages to propagate upwards and be handled/emitted
> >>>> from the root logger with default settings.
> >>>>
> >>>> When we want iotest logging, we attach a handler to the
> >>>> qemu.iotests.diff_io logger and disable propagation to avoid possible
> >>>> double-printing.
> >>>>
> >>>> For more information on python logging infrastructure, I highly
> >>>> recommend downloading the pip package `logging_tree`, which provides
> >>>> convenient visualizations of the hierarchical logging configuration
> >>>> under different circumstances.
> >>>>
> >>>> See https://pypi.org/project/logging_tree/ for more information.
> >>>>
> >>>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>>
> >>> Should we enable logger if -d is given?
> >>>
> >>> Previously we had:
> >>>
> >>> $ ./check -d -T -raw 281
> >>> [...]
> >>> 281 not run: not suitable for this image format: raw
> >>> 281 not run [15:39:03] [15:39:04] not suitable for this image format: raw
> >>> Not run: 281
> >>>
> >>> After this series, the first line of output from notrun() is missing.
> >>> Not that I think it's important to have the line, but as long as we
> >>> bother to call logger.warning(), I thought that maybe we want to be able
> >>> to actually see the effect of it somehwere?
> >>>
> >>> Kevin
> >>>
> >>
> >> Uh, okay. So this is weirder than I thought it was going to be!
> >>
> >> So, if you move the debug configuration up above the _verify calls,
> >> you'll see the message printed out to the debug stream:
> >>
> >> DEBUG:qemu.iotests:iotests debugging messages active
> >> WARNING:qemu.iotests:281 not run: not suitable for this image format: raw
> >>
> >> ...but if you omit the `-d` flag, the message vanishes into a black
> >> hole. Did it always work like that ...?
> >
> > Yes, this is how it used to work. It's a result of ./check only printing
> > the test output with -d, and such log messages are basically just test
> > output.
> >
> > And I think it's exactly what we want: Without -d, you want only the
> > summary, i.e. a single line that says "pass", "fail" or "notrun",
> > potentially with a small note at the end of the line, but that's it.
>
> OK, maybe. So I guess what happens here is that if you don't use -d, the
> output gets redirected to file, and that file is summarily deleted.
>
> Your phrase "but as long as we bother to call logger.warning(), I
> thought that maybe we want to be able to actually see the effect of it
> somewhere" stuck with me -- I think you're right.
Yes, and I still think the same, but "somewhere" includes -d for me. I
just wouldn't want it to be effectively dead code that doesn't have an
effect no matter what options you use.
> I kind of do expect that if I call a function called warning() that it's
> gonna do some damage. principle of least surprise, etc.
>
> So two things:
>
> (1) Maybe the iotest logger ought to always use stderr, and we should
> see any calls to warning() or error() even when debugging is off.
Even stderr is considered test output. This is not an accident, but we
actually want to test that we get error messages. So this wouldn't
change the visible result.
Maybe what we should do is print a small notice "warnings/errors were
logged" at the end of the line like we do for notrun, so you know you
should rerun the test with -d?
But anyway, why would we ever get error() with a test that passes?
> (2) These skip notifications are not warnings, they are informational
> and can be disabled when `-d` is omitted. (Especially because they are
> represented through another channel.)
>
> (I'll send the fixup for the simpler thing first, and you can take or
> leave the second thing.)
I would be perfectly happy with just a fix that makes the messages
appear again with -d.
Kevin
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v10 14/14] iotests: use python logging for iotests.log()
2020-05-15 9:03 ` Kevin Wolf
@ 2020-05-18 18:29 ` John Snow
0 siblings, 0 replies; 41+ messages in thread
From: John Snow @ 2020-05-18 18:29 UTC (permalink / raw)
To: Kevin Wolf; +Cc: ehabkost, qemu-block, armbru, qemu-devel, Max Reitz, philmd
On 5/15/20 5:03 AM, Kevin Wolf wrote:
> Am 14.05.2020 um 21:54 hat John Snow geschrieben:
>>
>>
>> On 5/14/20 6:06 AM, Kevin Wolf wrote:
>>> Am 14.05.2020 um 08:24 hat John Snow geschrieben:
>>>> On 3/31/20 9:44 AM, Kevin Wolf wrote:
>>>>> Am 31.03.2020 um 02:00 hat John Snow geschrieben:
>>>>>> We can turn logging on/off globally instead of per-function.
>>>>>>
>>>>>> Remove use_log from run_job, and use python logging to turn on
>>>>>> diffable output when we run through a script entry point.
>>>>>>
>>>>>> iotest 245 changes output order due to buffering reasons.
>>>>>>
>>>>>>
>>>>>> An extended note on python logging:
>>>>>>
>>>>>> A NullHandler is added to `qemu.iotests` to stop output from being
>>>>>> generated if this code is used as a library without configuring logging.
>>>>>> A NullHandler is only needed at the root, so a duplicate handler is not
>>>>>> needed for `qemu.iotests.diff_io`.
>>>>>>
>>>>>> When logging is not configured, messages at the 'WARNING' levels or
>>>>>> above are printed with default settings. The NullHandler stops this from
>>>>>> occurring, which is considered good hygiene for code used as a library.
>>>>>>
>>>>>> See https://docs.python.org/3/howto/logging.html#library-config
>>>>>>
>>>>>> When logging is actually enabled (always at the behest of an explicit
>>>>>> call by a client script), a root logger is implicitly created at the
>>>>>> root, which allows messages to propagate upwards and be handled/emitted
>>>>>> from the root logger with default settings.
>>>>>>
>>>>>> When we want iotest logging, we attach a handler to the
>>>>>> qemu.iotests.diff_io logger and disable propagation to avoid possible
>>>>>> double-printing.
>>>>>>
>>>>>> For more information on python logging infrastructure, I highly
>>>>>> recommend downloading the pip package `logging_tree`, which provides
>>>>>> convenient visualizations of the hierarchical logging configuration
>>>>>> under different circumstances.
>>>>>>
>>>>>> See https://pypi.org/project/logging_tree/ for more information.
>>>>>>
>>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>>
>>>>> Should we enable logger if -d is given?
>>>>>
>>>>> Previously we had:
>>>>>
>>>>> $ ./check -d -T -raw 281
>>>>> [...]
>>>>> 281 not run: not suitable for this image format: raw
>>>>> 281 not run [15:39:03] [15:39:04] not suitable for this image format: raw
>>>>> Not run: 281
>>>>>
>>>>> After this series, the first line of output from notrun() is missing.
>>>>> Not that I think it's important to have the line, but as long as we
>>>>> bother to call logger.warning(), I thought that maybe we want to be able
>>>>> to actually see the effect of it somehwere?
>>>>>
>>>>> Kevin
>>>>>
>>>>
>>>> Uh, okay. So this is weirder than I thought it was going to be!
>>>>
>>>> So, if you move the debug configuration up above the _verify calls,
>>>> you'll see the message printed out to the debug stream:
>>>>
>>>> DEBUG:qemu.iotests:iotests debugging messages active
>>>> WARNING:qemu.iotests:281 not run: not suitable for this image format: raw
>>>>
>>>> ...but if you omit the `-d` flag, the message vanishes into a black
>>>> hole. Did it always work like that ...?
>>>
>>> Yes, this is how it used to work. It's a result of ./check only printing
>>> the test output with -d, and such log messages are basically just test
>>> output.
>>>
>>> And I think it's exactly what we want: Without -d, you want only the
>>> summary, i.e. a single line that says "pass", "fail" or "notrun",
>>> potentially with a small note at the end of the line, but that's it.
>>
>> OK, maybe. So I guess what happens here is that if you don't use -d, the
>> output gets redirected to file, and that file is summarily deleted.
>>
>> Your phrase "but as long as we bother to call logger.warning(), I
>> thought that maybe we want to be able to actually see the effect of it
>> somewhere" stuck with me -- I think you're right.
>
> Yes, and I still think the same, but "somewhere" includes -d for me. I
> just wouldn't want it to be effectively dead code that doesn't have an
> effect no matter what options you use.
>
>> I kind of do expect that if I call a function called warning() that it's
>> gonna do some damage. principle of least surprise, etc.
>>
>> So two things:
>>
>> (1) Maybe the iotest logger ought to always use stderr, and we should
>> see any calls to warning() or error() even when debugging is off.
>
> Even stderr is considered test output. This is not an accident, but we
> actually want to test that we get error messages. So this wouldn't
> change the visible result.
>
I learned this after I wrote this idea and went to go implement it.
Makes sense, if we want to compare stderr messages to known failure
cases. Oops.
> Maybe what we should do is print a small notice "warnings/errors were
> logged" at the end of the line like we do for notrun, so you know you
> should rerun the test with -d?
>
> But anyway, why would we ever get error() with a test that passes?
>
Bad tests? :)
>> (2) These skip notifications are not warnings, they are informational
>> and can be disabled when `-d` is omitted. (Especially because they are
>> represented through another channel.)
>>
>> (I'll send the fixup for the simpler thing first, and you can take or
>> leave the second thing.)
>
> I would be perfectly happy with just a fix that makes the messages
> appear again with -d.
>
> Kevin
>
Yup, just wanted to take a look at it and give it proper consideration.
Patch 1/3 from that series ought to do the trick all by itself.
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2020-05-18 18:31 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31 0:00 [PATCH v10 00/14] iotests: use python logging John Snow
2020-03-31 0:00 ` [PATCH v10 01/14] iotests: do a light delinting John Snow
2020-03-31 0:00 ` [PATCH v10 02/14] iotests: don't use 'format' for drive_add John Snow
2020-03-31 0:00 ` [PATCH v10 03/14] iotests: ignore import warnings from pylint John Snow
2020-03-31 0:00 ` [PATCH v10 04/14] iotests: replace mutable list default args John Snow
2020-03-31 0:00 ` [PATCH v10 05/14] iotests: add pylintrc file John Snow
2020-03-31 0:00 ` [PATCH v10 06/14] iotests: alphabetize standard imports John Snow
2020-03-31 8:00 ` Philippe Mathieu-Daudé
2020-03-31 0:00 ` [PATCH v10 07/14] iotests: drop pre-Python 3.4 compatibility code John Snow
2020-03-31 0:00 ` [PATCH v10 08/14] iotests: touch up log function signature John Snow
2020-03-31 0:00 ` [PATCH v10 09/14] iotests: limit line length to 79 chars John Snow
2020-03-31 0:00 ` [PATCH v10 10/14] iotests: add hmp helper with logging John Snow
2020-03-31 10:21 ` Max Reitz
2020-03-31 14:00 ` Kevin Wolf
2020-04-01 12:28 ` Max Reitz
2020-04-01 12:42 ` Max Reitz
2020-04-01 13:51 ` Kevin Wolf
2020-04-01 14:53 ` Max Reitz
2020-03-31 17:23 ` John Snow
2020-03-31 17:39 ` Kevin Wolf
2020-04-01 12:40 ` Max Reitz
2020-04-02 18:27 ` John Snow
2020-04-03 7:46 ` Kevin Wolf
2020-04-03 8:57 ` Max Reitz
2020-04-04 2:38 ` John Snow
2020-03-31 0:00 ` [PATCH v10 11/14] iotests: add script_initialize John Snow
2020-03-31 0:00 ` [PATCH v10 12/14] iotest 258: use script_main John Snow
2020-03-31 0:00 ` [PATCH v10 13/14] iotests: Mark verify functions as private John Snow
2020-03-31 10:40 ` Max Reitz
2020-03-31 0:00 ` [PATCH v10 14/14] iotests: use python logging for iotests.log() John Snow
2020-03-31 13:44 ` Kevin Wolf
2020-05-14 6:24 ` John Snow
2020-05-14 10:06 ` Kevin Wolf
2020-05-14 19:54 ` John Snow
2020-05-15 9:03 ` Kevin Wolf
2020-05-18 18:29 ` John Snow
2020-03-31 15:07 ` [PATCH v10 00/14] iotests: use python logging Kevin Wolf
2020-04-28 11:46 ` Max Reitz
2020-04-28 12:21 ` Kevin Wolf
2020-04-28 17:36 ` John Snow
2020-04-29 8:08 ` Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.