* [PATCH v6 0/9] iotests: use python logging
@ 2020-02-27 0:06 John Snow
2020-02-27 0:06 ` [PATCH v6 1/9] iotests: do a light delinting John Snow
` (8 more replies)
0 siblings, 9 replies; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
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.
V6:
- It's been so long since V5, let's just look at it anew.
- Dropped patch 1, rebased, added more delinting.
- I'm not touching the supported_platforms thing.
Not interested in rehashing that debate.
V5:
- Rebased again
- Allow Python tests to run on any platform
V4:
- Rebased on top of kwolf/block at the behest of mreitz
V3:
- Rebased for 4.1+; now based on main branch.
V2:
- Added all of the other python tests I missed to use script_initialize
- Refactored the common setup as per Ehabkost's suggestion
- Added protocol arguments to common initialization,
but this isn't strictly required.
John Snow (9):
iotests: do a light delinting
iotests: add script_initialize
iotests: replace mutable list default args
iotest 258: use script_main
iotests: Mark verify functions as private
iotests: use python logging for iotests.log()
iotests: ignore import warnings from pylint
iotests: don't use 'format' for drive_add
iotests: add pylintrc file
tests/qemu-iotests/030 | 4 +-
tests/qemu-iotests/055 | 3 +-
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/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 | 244 +++++++++++++++++++---------------
tests/qemu-iotests/pylintrc | 20 +++
42 files changed, 257 insertions(+), 177 deletions(-)
create mode 100644 tests/qemu-iotests/pylintrc
--
2.21.1
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6 1/9] iotests: do a light delinting
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 12:59 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 2/9] iotests: add script_initialize John Snow
` (7 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
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. This will be important if (when?) we
begin adding type hints to our code base.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 88 ++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 43 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8815052eb5..e8a0ea14fc 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
@@ -34,7 +32,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)
# This will not work if arguments contain spaces but is necessary if we
# want to support the override options that ./check supports.
@@ -138,11 +136,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)
@@ -221,7 +219,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()
@@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
' '.join(qemu_nbd_args + ['--fork'] + list(args))))
if exitcode == 0:
return exitcode, ''
- else:
- return exitcode, subp.communicate()[0]
+ return exitcode, subp.communicate()[0]
def qemu_nbd_popen(*args):
'''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -310,7 +307,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)
@@ -321,7 +318,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
@@ -347,7 +344,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
@@ -358,7 +355,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
@@ -369,14 +366,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):
@@ -385,7 +382,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.
@@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
def remote_filename(path):
if imgproto == 'file':
return path
- elif imgproto == 'ssh':
+ if imgproto == 'ssh':
return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
- else:
- raise Exception("Protocol %s not supported" % (imgproto))
+ raise Exception("Protocol %s not supported" % (imgproto))
class VM(qtest.QEMUQtestMachine):
'''A QEMU VM'''
@@ -532,11 +528,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'''
@@ -547,8 +543,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, atom in enumerate(obj):
+ self.flatten_qmp_object(atom, output, basestr + str(i) + '.')
elif isinstance(obj, dict):
for key in obj:
self.flatten_qmp_object(obj[key], output, basestr + key + '.')
@@ -703,9 +699,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
@@ -756,12 +750,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
@@ -779,6 +774,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('/'):
@@ -824,7 +825,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')
@@ -834,15 +835,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
@@ -891,13 +892,14 @@ 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 } }
+ f = {'data': {'type': 'mirror', 'device': drive}}
event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
+ return event
def wait_ready_and_cancel(self, drive='drive0'):
self.wait_ready(drive=drive)
@@ -926,7 +928,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
@@ -1023,8 +1025,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):
@@ -1056,8 +1058,8 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs):
if usf_list:
test_case.case_skip('{}: formats {} are not whitelisted'.format(
test_case, usf_list))
- else:
- return func(test_case, *args, **kwargs)
+ return None
+ return func(test_case, *args, **kwargs)
return func_wrapper
return skip_test_decorator
@@ -1067,8 +1069,8 @@ 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]))
- else:
- return func(*args, **kwargs)
+ return None
+ return func(*args, **kwargs)
return func_wrapper
def execute_unittest(output, verbosity, debug):
--
2.21.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v6 2/9] iotests: add script_initialize
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
2020-02-27 0:06 ` [PATCH v6 1/9] iotests: do a light delinting John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 13:47 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 3/9] iotests: replace mutable list default args John Snow
` (6 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
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>
---
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 | 73 +++++++++++++++++++++++------------
37 files changed, 128 insertions(+), 80 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 e8a0ea14fc..fdcf8a940c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@
import atexit
import io
from collections import OrderedDict
+from typing import Collection
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
from qemu import qtest
@@ -991,12 +992,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)
@@ -1073,7 +1073,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:
@@ -1081,6 +1092,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)
@@ -1092,13 +1105,18 @@ 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: Collection[str] = (),
+ supported_platforms: Collection[str] = (),
+ supported_cache_modes: Collection[str] = (),
+ supported_aio_modes: Collection[str] = (),
+ unsupported_fmts: Collection[str] = (),
+ supported_protocols: Collection[str] = (),
+ unsupported_protocols: Collection[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.
+ """
# We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
# indicate that we're not being run via "check". There may be
@@ -1108,34 +1126,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] 34+ messages in thread
* [PATCH v6 3/9] iotests: replace mutable list default args
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
2020-02-27 0:06 ` [PATCH v6 1/9] iotests: do a light delinting John Snow
2020-02-27 0:06 ` [PATCH v6 2/9] iotests: add script_initialize John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 13:50 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 4/9] iotest 258: use script_main John Snow
` (5 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
It's bad hygiene: if we modify this list, it will be modified across all
invocations.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fdcf8a940c..9eb96561b6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,7 +136,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')
@@ -351,7 +351,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:
@@ -566,7 +566,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))
@@ -968,7 +968,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 \
@@ -982,7 +982,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:
@@ -1000,11 +1000,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=()):
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)
@@ -1044,7 +1044,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):
--
2.21.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v6 4/9] iotest 258: use script_main
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
` (2 preceding siblings ...)
2020-02-27 0:06 ` [PATCH v6 3/9] iotests: replace mutable list default args John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 13:55 ` Max Reitz
2020-02-27 14:10 ` Philippe Mathieu-Daudé
2020-02-27 0:06 ` [PATCH v6 5/9] iotests: Mark verify functions as private John Snow
` (4 subsequent siblings)
8 siblings, 2 replies; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
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>
---
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] 34+ messages in thread
* [PATCH v6 5/9] iotests: Mark verify functions as private
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
` (3 preceding siblings ...)
2020-02-27 0:06 ` [PATCH v6 4/9] iotest 258: use script_main John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 13:59 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 6/9] iotests: use python logging for iotests.log() John Snow
` (3 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
Discourage their use.
(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 | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9eb96561b6..b02d7932fa 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -968,7 +968,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 \
@@ -982,7 +982,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:
@@ -992,7 +992,7 @@ 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=(), unsupported=()):
if any((sys.platform.startswith(x) for x in unsupported)):
notrun('not suitable for this OS: %s' % sys.platform)
@@ -1000,11 +1000,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=()):
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)
@@ -1126,11 +1126,11 @@ def execute_setup_common(supported_fmts: Collection[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] 34+ messages in thread
* [PATCH v6 6/9] iotests: use python logging for iotests.log()
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
` (4 preceding siblings ...)
2020-02-27 0:06 ` [PATCH v6 5/9] iotests: Mark verify functions as private John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 14:21 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 7/9] iotests: ignore import warnings from pylint John Snow
` (2 subsequent siblings)
8 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
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.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/030 | 4 +--
tests/qemu-iotests/245 | 1 +
tests/qemu-iotests/245.out | 24 ++++++++---------
tests/qemu-iotests/iotests.py | 50 +++++++++++++++++++++--------------
4 files changed, 45 insertions(+), 34 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/245 b/tests/qemu-iotests/245
index 489bf78bd0..edb40a4ae8 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1002,5 +1002,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
self.reopen(opts, {'backing': 'hd2'})
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 a19de5214d..15c3630e92 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 18 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 b02d7932fa..60c4c7f736 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -35,6 +35,14 @@
assert sys.version_info >= (3, 6)
+# 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')
+
+
# This will not work if arguments contain spaces but is necessary if we
# want to support the override options that ./check supports.
qemu_img_args = [os.environ.get('QEMU_IMG_PROG', 'qemu-img')]
@@ -361,10 +369,10 @@ def log(msg, filters=(), indent=None):
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))
+ test_logger.info(json.dumps(msg, sort_keys=do_sort,
+ indent=indent, separators=separators))
else:
- print(msg)
+ test_logger.info(msg)
class Timeout:
def __init__(self, seconds, errmsg="Timeout"):
@@ -578,7 +586,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.
@@ -591,7 +599,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.
"""
@@ -609,8 +616,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':
@@ -618,26 +624,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':
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
@@ -953,7 +951,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):
@@ -1136,6 +1134,7 @@ def execute_setup_common(supported_fmts: Collection[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
@@ -1148,14 +1147,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] 34+ messages in thread
* [PATCH v6 7/9] iotests: ignore import warnings from pylint
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
` (5 preceding siblings ...)
2020-02-27 0:06 ` [PATCH v6 6/9] iotests: use python logging for iotests.log() John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 14:14 ` Philippe Mathieu-Daudé
2020-02-27 0:06 ` [PATCH v6 8/9] iotests: don't use 'format' for drive_add John Snow
2020-02-27 0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
8 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
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.
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 60c4c7f736..214f59995e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
from collections import OrderedDict
from typing import Collection
+# 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] 34+ messages in thread
* [PATCH v6 8/9] iotests: don't use 'format' for drive_add
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
` (6 preceding siblings ...)
2020-02-27 0:06 ` [PATCH v6 7/9] iotests: ignore import warnings from pylint John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 14:12 ` Philippe Mathieu-Daudé
2020-02-27 14:26 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
8 siblings, 2 replies; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
It shadows (with a different type) the built-in format.
Use something else.
Signed-off-by: John Snow <jsnow@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 214f59995e..8bf640c632 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -492,21 +492,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] 34+ messages in thread
* [PATCH v6 9/9] iotests: add pylintrc file
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
` (7 preceding siblings ...)
2020-02-27 0:06 ` [PATCH v6 8/9] iotests: don't use 'format' for drive_add John Snow
@ 2020-02-27 0:06 ` John Snow
2020-02-27 1:57 ` Eric Blake
` (2 more replies)
8 siblings, 3 replies; 34+ messages in thread
From: John Snow @ 2020-02-27 0:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, qemu-block, Max Reitz
Repeatable results. run `pylint iotests.py` and you should get a pass.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
1 file changed, 20 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..feed506f75
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,20 @@
+[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,
+ missing-docstring,
+ line-too-long,
+ too-many-lines,
+ too-few-public-methods,
+ too-many-arguments,
+ too-many-locals,
+ too-many-branches,
+ too-many-public-methods,
\ No newline at end of file
--
2.21.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v6 9/9] iotests: add pylintrc file
2020-02-27 0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
@ 2020-02-27 1:57 ` Eric Blake
2020-02-27 14:11 ` Philippe Mathieu-Daudé
2020-03-04 7:22 ` Markus Armbruster
2 siblings, 0 replies; 34+ messages in thread
From: Eric Blake @ 2020-02-27 1:57 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On 2/26/20 6:06 PM, John Snow wrote:
> Repeatable results. run `pylint iotests.py` and you should get a pass.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> create mode 100644 tests/qemu-iotests/pylintrc
> + too-many-branches,
> + too-many-public-methods,
> \ No newline at end of file
Intentional?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 1/9] iotests: do a light delinting
2020-02-27 0:06 ` [PATCH v6 1/9] iotests: do a light delinting John Snow
@ 2020-02-27 12:59 ` Max Reitz
2020-03-03 21:25 ` John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Max Reitz @ 2020-02-27 12:59 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 4026 bytes --]
On 27.02.20 01:06, John Snow wrote:
> 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. This will be important if (when?) we
> begin adding type hints to our code base.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 88 ++++++++++++++++++-----------------
> 1 file changed, 45 insertions(+), 43 deletions(-)
I feel like I’m the wrongest person there is for reviewing a Python
style-fixing patch, but here I am and so here I go:
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 8815052eb5..e8a0ea14fc 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
[...]
> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
> ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
> if exitcode == 0:
> return exitcode, ''
> - else:
> - return exitcode, subp.communicate()[0]
> + return exitcode, subp.communicate()[0]
If we want to make such a change (which I don’t doubt we want), I think
it should be the other way around: Make the condition “exitcode != 0”,
so the final return is the return for the successful case. (Just
because I think that’s how we usually do it, at least in the qemu code?)
[...]
> @@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
> def remote_filename(path):
> if imgproto == 'file':
> return path
> - elif imgproto == 'ssh':
> + if imgproto == 'ssh':
This seems like a weird thing to complain about for a coding style
check, but whatever.
(As in, I prefer the elif form)
> return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
> - else:
> - raise Exception("Protocol %s not supported" % (imgproto))
> + raise Exception("Protocol %s not supported" % (imgproto))
>
> class VM(qtest.QEMUQtestMachine):
> '''A QEMU VM'''
[...]
> @@ -756,12 +750,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)
I don’t mind the if alignment, but I do mind not aligning the third line
to the “edge” above it (i.e. the third line is part of the condition, so
I’d align it to the “if” condition).
But then again it’s nothing new that I like to disagree with commonly
agreed-upon Python coding styles, so.
[...]
> @@ -891,13 +892,14 @@ 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 } }
> + f = {'data': {'type': 'mirror', 'device': drive}}
> event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
> + return event
Why not just “return self.vm.event_wait…”?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 2/9] iotests: add script_initialize
2020-02-27 0:06 ` [PATCH v6 2/9] iotests: add script_initialize John Snow
@ 2020-02-27 13:47 ` Max Reitz
2020-03-03 21:12 ` John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Max Reitz @ 2020-02-27 13:47 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 3376 bytes --]
On 27.02.20 01:06, John Snow wrote:
> 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>
> ---
> 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 | 73 +++++++++++++++++++++++------------
> 37 files changed, 128 insertions(+), 80 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[...]
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e8a0ea14fc..fdcf8a940c 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
[...]
> @@ -1092,13 +1105,18 @@ 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: Collection[str] = (),
First time I see something like this, but I suppose it means any
collection (i.e. list or tuple in this case) that has str values?
Max
> + supported_platforms: Collection[str] = (),
> + supported_cache_modes: Collection[str] = (),
> + supported_aio_modes: Collection[str] = (),
> + unsupported_fmts: Collection[str] = (),
> + supported_protocols: Collection[str] = (),
> + unsupported_protocols: Collection[str] = ()) -> bool:
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 3/9] iotests: replace mutable list default args
2020-02-27 0:06 ` [PATCH v6 3/9] iotests: replace mutable list default args John Snow
@ 2020-02-27 13:50 ` Max Reitz
0 siblings, 0 replies; 34+ messages in thread
From: Max Reitz @ 2020-02-27 13:50 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 343 bytes --]
On 27.02.20 01:06, John Snow wrote:
> It's bad hygiene: if we modify this list, it will be modified across all
> invocations.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 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] 34+ messages in thread
* Re: [PATCH v6 4/9] iotest 258: use script_main
2020-02-27 0:06 ` [PATCH v6 4/9] iotest 258: use script_main John Snow
@ 2020-02-27 13:55 ` Max Reitz
2020-02-27 14:10 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Max Reitz @ 2020-02-27 13:55 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 342 bytes --]
On 27.02.20 01:06, John Snow wrote:
> 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>
> ---
> tests/qemu-iotests/258 | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 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] 34+ messages in thread
* Re: [PATCH v6 5/9] iotests: Mark verify functions as private
2020-02-27 0:06 ` [PATCH v6 5/9] iotests: Mark verify functions as private John Snow
@ 2020-02-27 13:59 ` Max Reitz
0 siblings, 0 replies; 34+ messages in thread
From: Max Reitz @ 2020-02-27 13:59 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 384 bytes --]
On 27.02.20 01:06, John Snow wrote:
> Discourage their use.
>
> (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 | 20 ++++++++++----------
> 1 file changed, 10 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] 34+ messages in thread
* Re: [PATCH v6 4/9] iotest 258: use script_main
2020-02-27 0:06 ` [PATCH v6 4/9] iotest 258: use script_main John Snow
2020-02-27 13:55 ` Max Reitz
@ 2020-02-27 14:10 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-27 14:10 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On 2/27/20 1:06 AM, John Snow wrote:
> 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>
> ---
> 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'])
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 9/9] iotests: add pylintrc file
2020-02-27 0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
2020-02-27 1:57 ` Eric Blake
@ 2020-02-27 14:11 ` Philippe Mathieu-Daudé
2020-03-03 19:52 ` John Snow
2020-03-04 7:22 ` Markus Armbruster
2 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-27 14:11 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On 2/27/20 1:06 AM, John Snow wrote:
> Repeatable results. run `pylint iotests.py` and you should get a pass.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
> 1 file changed, 20 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..feed506f75
> --- /dev/null
> +++ b/tests/qemu-iotests/pylintrc
> @@ -0,0 +1,20 @@
> +[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,
> + missing-docstring,
> + line-too-long,
> + too-many-lines,
> + too-few-public-methods,
> + too-many-arguments,
> + too-many-locals,
> + too-many-branches,
> + too-many-public-methods,
> \ No newline at end of file
>
Can you run it in one of the CI jobs?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 8/9] iotests: don't use 'format' for drive_add
2020-02-27 0:06 ` [PATCH v6 8/9] iotests: don't use 'format' for drive_add John Snow
@ 2020-02-27 14:12 ` Philippe Mathieu-Daudé
2020-02-27 14:26 ` Max Reitz
1 sibling, 0 replies; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-27 14:12 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On 2/27/20 1:06 AM, John Snow wrote:
> It shadows (with a different type) the built-in format.
> Use something else.
>
> Signed-off-by: John Snow <jsnow@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 214f59995e..8bf640c632 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -492,21 +492,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)
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint
2020-02-27 0:06 ` [PATCH v6 7/9] iotests: ignore import warnings from pylint John Snow
@ 2020-02-27 14:14 ` Philippe Mathieu-Daudé
2020-03-03 19:57 ` John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-27 14:14 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, Cleber Rosa, qemu-block, Max Reitz
On 2/27/20 1:06 AM, John Snow wrote:
> 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.
I'm tempted to NAck this and require an "installable python module"...
Let's discuss why it is that hard!
>
> 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 60c4c7f736..214f59995e 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -30,6 +30,7 @@
> from collections import OrderedDict
> from typing import Collection
>
> +# pylint: disable=import-error, wrong-import-position
> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> from qemu import qtest
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 6/9] iotests: use python logging for iotests.log()
2020-02-27 0:06 ` [PATCH v6 6/9] iotests: use python logging for iotests.log() John Snow
@ 2020-02-27 14:21 ` Max Reitz
2020-03-03 20:00 ` John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Max Reitz @ 2020-02-27 14:21 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 1797 bytes --]
On 27.02.20 01:06, John Snow wrote:
> 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.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/030 | 4 +--
> tests/qemu-iotests/245 | 1 +
> tests/qemu-iotests/245.out | 24 ++++++++---------
> tests/qemu-iotests/iotests.py | 50 +++++++++++++++++++++--------------
> 4 files changed, 45 insertions(+), 34 deletions(-)
[...]
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b02d7932fa..60c4c7f736 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -35,6 +35,14 @@
>
> assert sys.version_info >= (3, 6)
>
> +# Use this logger for logging messages directly from the iotests module
> +logger = logging.getLogger('qemu.iotests')
> +logger.addHandler(logging.NullHandler())
Hm, I never see another handler added to this, so how can these messages
actually be printed? Will enabling debug mode somehow make all loggers
print everything?
> +# Use this logger for messages that ought to be used for diff output.
> +test_logger = logging.getLogger('qemu.iotests.diff_io')
Also, why does logger get a null handler and this by default does not?
I’m asking because test_logger makes it look like you don’t necessarily
need a handler for output to be silently discarded.
Max
> # This will not work if arguments contain spaces but is necessary if we
> # want to support the override options that ./check supports.
> qemu_img_args = [os.environ.get('QEMU_IMG_PROG', 'qemu-img')]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 8/9] iotests: don't use 'format' for drive_add
2020-02-27 0:06 ` [PATCH v6 8/9] iotests: don't use 'format' for drive_add John Snow
2020-02-27 14:12 ` Philippe Mathieu-Daudé
@ 2020-02-27 14:26 ` Max Reitz
1 sibling, 0 replies; 34+ messages in thread
From: Max Reitz @ 2020-02-27 14:26 UTC (permalink / raw)
To: John Snow, qemu-devel; +Cc: Kevin Wolf, qemu-block
[-- Attachment #1.1: Type: text/plain, Size: 365 bytes --]
On 27.02.20 01:06, John Snow wrote:
> It shadows (with a different type) the built-in format.
> Use something else.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/055 | 3 ++-
> tests/qemu-iotests/iotests.py | 6 +++---
> 2 files changed, 5 insertions(+), 4 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] 34+ messages in thread
* Re: [PATCH v6 9/9] iotests: add pylintrc file
2020-02-27 14:11 ` Philippe Mathieu-Daudé
@ 2020-03-03 19:52 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-03-03 19:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz
On 2/27/20 9:11 AM, Philippe Mathieu-Daudé wrote:
> On 2/27/20 1:06 AM, John Snow wrote:
>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>> 1 file changed, 20 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..feed506f75
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,20 @@
>> +[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,
>> + missing-docstring,
>> + line-too-long,
>> + too-many-lines,
>> + too-few-public-methods,
>> + too-many-arguments,
>> + too-many-locals,
>> + too-many-branches,
>> + too-many-public-methods,
>> \ No newline at end of file
>>
>
> Can you run it in one of the CI jobs?
>
Tell me where to add it and I will do so.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint
2020-02-27 14:14 ` Philippe Mathieu-Daudé
@ 2020-03-03 19:57 ` John Snow
2020-03-04 0:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-03-03 19:57 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, qemu-block, Max Reitz
On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:
> On 2/27/20 1:06 AM, John Snow wrote:
>> 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.
>
> I'm tempted to NAck this and require an "installable python module"...
>
> Let's discuss why it is that hard!
>
I've been tricked into this before. It's not work I am interested in
doing right now; it's WAY beyond the scope of what I am doing here.
It involves properly factoring all of our python code, deciding which
portions are meant to be installed separately from QEMU itself, coming
up with a versioning scheme, packaging code, and moving code around in
many places.
Then it involves coming up with tooling and infrastructure for creating
virtual environments, installing the right packages to it, and using it
to run our python tests.
No, that's way too invasive. I'm not doing it and I will scream loudly
if you make me.
A less invasive hack involves setting the python import path in a
consolidated spot so that python knows where it can import from. This
works, but might break the ability to run such tests as one-offs without
executing the environment setup.
Again, not work I care to do right now and so I won't. The benefit of
these patches is to provide some minimum viable CI CQA for Python where
we had none before, NOT fix every possible Python problem in one shot.
--js
>>
>> 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 60c4c7f736..214f59995e 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -30,6 +30,7 @@
>> from collections import OrderedDict
>> from typing import Collection
>> +# pylint: disable=import-error, wrong-import-position
>> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
>> 'python'))
>> from qemu import qtest
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 6/9] iotests: use python logging for iotests.log()
2020-02-27 14:21 ` Max Reitz
@ 2020-03-03 20:00 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-03-03 20:00 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block
On 2/27/20 9:21 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/030 | 4 +--
>> tests/qemu-iotests/245 | 1 +
>> tests/qemu-iotests/245.out | 24 ++++++++---------
>> tests/qemu-iotests/iotests.py | 50 +++++++++++++++++++++--------------
>> 4 files changed, 45 insertions(+), 34 deletions(-)
>
> [...]
>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index b02d7932fa..60c4c7f736 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -35,6 +35,14 @@
>>
>> assert sys.version_info >= (3, 6)
>>
>> +# Use this logger for logging messages directly from the iotests module
>> +logger = logging.getLogger('qemu.iotests')
>> +logger.addHandler(logging.NullHandler())
>
> Hm, I never see another handler added to this, so how can these messages
> actually be printed? Will enabling debug mode somehow make all loggers
> print everything?
>
Implicit fallback to root logger which handles them when the root logger
is configured, which happens when logging.basicConfig is called.
>> +# Use this logger for messages that ought to be used for diff output.
>> +test_logger = logging.getLogger('qemu.iotests.diff_io')
>
> Also, why does logger get a null handler and this by default does not?
> I’m asking because test_logger makes it look like you don’t necessarily
> need a handler for output to be silently discarded.
>
> Max
>
It's a library trick. By adding a null handler at `qemu.iotests` I add a
default handler to everything produced by iotests. When logging is not
configured, this stops messages from being printed -- there IS a default
"nonconfigured" logging behavior and this stops it.
What I learned since the last time I wrote this patchset is that you
only need a NullHandler at some particular root, so "qemu.iotests"
suffices. "qemu.iotests.diff_io" is a child of that other logger.
>> # This will not work if arguments contain spaces but is necessary if we
>> # want to support the override options that ./check supports.
>> qemu_img_args = [os.environ.get('QEMU_IMG_PROG', 'qemu-img')]
>
--
—js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 2/9] iotests: add script_initialize
2020-02-27 13:47 ` Max Reitz
@ 2020-03-03 21:12 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-03-03 21:12 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block
On 2/27/20 8:47 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> 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>
>> ---
>> 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 | 73 +++++++++++++++++++++++------------
>> 37 files changed, 128 insertions(+), 80 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> [...]
>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index e8a0ea14fc..fdcf8a940c 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>
> [...]
>
>> @@ -1092,13 +1105,18 @@ 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: Collection[str] = (),
>
> First time I see something like this, but I suppose it means any
> collection (i.e. list or tuple in this case) that has str values?
>
> Max
>
Yes. Testing the waters for idiomatic type annotations. We chose our
python version to allow us to use them, so I'm pushing on that boundary.
A Collection in this case is a Python ABC that collects the Sized,
Iterable and Container ABCs.
Roughly:
Sized: provides __len__ (len(x))
Iterable: provides __iter__ ("for x in y")
Container: provides __contains__ ("if 'x' in y")
In this case, we want Iterable (to enumerate the atoms) and Sized (to
provide truth-testing that returns False when the collection has a size
of zero.) "Collection" is the nearest available type that describes all
of the desired types of duck, without becoming overly specific for
features of the type we are not using.
More information:
https://docs.python.org/3.6/library/collections.abc.html#module-collections.abc
>> + supported_platforms: Collection[str] = (),
>> + supported_cache_modes: Collection[str] = (),
>> + supported_aio_modes: Collection[str] = (),
>> + unsupported_fmts: Collection[str] = (),
>> + supported_protocols: Collection[str] = (),
>> + unsupported_protocols: Collection[str] = ()) -> bool:
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 1/9] iotests: do a light delinting
2020-02-27 12:59 ` Max Reitz
@ 2020-03-03 21:25 ` John Snow
2020-03-04 11:12 ` Kevin Wolf
0 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-03-03 21:25 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, qemu-block
On 2/27/20 7:59 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> 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. This will be important if (when?) we
>> begin adding type hints to our code base.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/iotests.py | 88 ++++++++++++++++++-----------------
>> 1 file changed, 45 insertions(+), 43 deletions(-)
>
> I feel like I’m the wrongest person there is for reviewing a Python
> style-fixing patch, but here I am and so here I go:
>
No, it's good.
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 8815052eb5..e8a0ea14fc 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>
> [...]
>
>> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
>> ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
>> if exitcode == 0:
>> return exitcode, ''
>> - else:
>> - return exitcode, subp.communicate()[0]
>> + return exitcode, subp.communicate()[0]
>
> If we want to make such a change (which I don’t doubt we want), I think
> it should be the other way around: Make the condition “exitcode != 0”,
> so the final return is the return for the successful case. (Just
> because I think that’s how we usually do it, at least in the qemu code?)
>
> [...]
>
Yes, makes sense. I was behaving a little more mechanically.
>> @@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
>> def remote_filename(path):
>> if imgproto == 'file':
>> return path
>> - elif imgproto == 'ssh':
>> + if imgproto == 'ssh':
>
> This seems like a weird thing to complain about for a coding style
> check, but whatever.
>
> (As in, I prefer the elif form)
>
Honestly, I do too. We can silence the warning instead.
This warning option doesn't like "if return else return" constructs,
preferring instead:
if x:
return 0
return 1
but I have to admit that I often like to see the branches laid out as
branches, too.
Other Pythonistas (Eduardo, Philippe, Markus?) -- strong feelings one
way or the other?
>> return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
>> - else:
>> - raise Exception("Protocol %s not supported" % (imgproto))
>> + raise Exception("Protocol %s not supported" % (imgproto))
>>
>> class VM(qtest.QEMUQtestMachine):
>> '''A QEMU VM'''
>
> [...]
>
>> @@ -756,12 +750,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)
>
> I don’t mind the if alignment, but I do mind not aligning the third line
> to the “edge” above it (i.e. the third line is part of the condition, so
> I’d align it to the “if” condition).
>
> But then again it’s nothing new that I like to disagree with commonly
> agreed-upon Python coding styles, so.
>
> [...]
>
OK, that can be addressed by highlighting the sub-expression with
parentheses:
node_id = next(edge['child'] for edge in graph['edges']
if (edge['parent'] == node['id'] and
edge['name'] == child_name))
>> @@ -891,13 +892,14 @@ 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 } }
>> + f = {'data': {'type': 'mirror', 'device': drive}}
>> event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
>> + return event
>
> Why not just “return self.vm.event_wait…”?
>
Shrug. Sometimes I name my return variables when working in Python to
give some semantic clue over what exactly I'm even returning.
I can change it; but the docstring will grow to describe what it returns
to re-document the same.
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint
2020-03-03 19:57 ` John Snow
@ 2020-03-04 0:02 ` Philippe Mathieu-Daudé
2020-03-04 18:59 ` John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-03-04 0:02 UTC (permalink / raw)
To: John Snow, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 3/3/20 8:57 PM, John Snow wrote:
> On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:
>> On 2/27/20 1:06 AM, John Snow wrote:
>>> 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.
>>
>> I'm tempted to NAck this and require an "installable python module"...
>>
>> Let's discuss why it is that hard!
>>
>
> I've been tricked into this before. It's not work I am interested in
> doing right now; it's WAY beyond the scope of what I am doing here.
>
> It involves properly factoring all of our python code, deciding which
> portions are meant to be installed separately from QEMU itself, coming
> up with a versioning scheme, packaging code, and moving code around in
> many places.
>
> Then it involves coming up with tooling and infrastructure for creating
> virtual environments, installing the right packages to it, and using it
> to run our python tests.
>
> No, that's way too invasive. I'm not doing it and I will scream loudly
> if you make me.
>
> A less invasive hack involves setting the python import path in a
> consolidated spot so that python knows where it can import from. This
> works, but might break the ability to run such tests as one-offs without
> executing the environment setup.
>
> Again, not work I care to do right now and so I won't. The benefit of
> these patches is to provide some minimum viable CI CQA for Python where
> we had none before, NOT fix every possible Python problem in one shot.
OK I guess we misunderstood each other :)
I didn't understood your comment as personal to you for this patch, but
generic. It makes sense it is not your priority and it is obvious this
task will take a single developer a lot of time resources. I am
certainly NOT asking you to do it.
My question was rather community-oriented.
(Cc'ing Eduardo because we talked about this after the last KVM forum).
>
> --js
>
>>>
>>> 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 60c4c7f736..214f59995e 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -30,6 +30,7 @@
>>> from collections import OrderedDict
>>> from typing import Collection
>>> +# pylint: disable=import-error, wrong-import-position
>>> sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
>>> 'python'))
>>> from qemu import qtest
>>>
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 9/9] iotests: add pylintrc file
2020-02-27 0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
2020-02-27 1:57 ` Eric Blake
2020-02-27 14:11 ` Philippe Mathieu-Daudé
@ 2020-03-04 7:22 ` Markus Armbruster
2020-03-04 19:17 ` John Snow
2 siblings, 1 reply; 34+ messages in thread
From: Markus Armbruster @ 2020-03-04 7:22 UTC (permalink / raw)
To: John Snow; +Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz
John Snow <jsnow@redhat.com> writes:
> Repeatable results. run `pylint iotests.py` and you should get a pass.
Start your sentences with a capital letter, please.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
> 1 file changed, 20 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..feed506f75
> --- /dev/null
> +++ b/tests/qemu-iotests/pylintrc
> @@ -0,0 +1,20 @@
> +[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,
> + missing-docstring,
> + line-too-long,
> + too-many-lines,
> + too-few-public-methods,
> + too-many-arguments,
> + too-many-locals,
> + too-many-branches,
> + too-many-public-methods,
> \ No newline at end of file
Add the newline, please.
German pejorative for the too-many- and too-few- warnings: "Müsli".
Implies it's for muesli-knitters / granola-crunchers indulging their
orthorexia.
missing-docstring is not advisable for libraries. Feels okay here.
line-too-long might be worth cleaning up. How many of them do we have
now?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 1/9] iotests: do a light delinting
2020-03-03 21:25 ` John Snow
@ 2020-03-04 11:12 ` Kevin Wolf
2020-03-04 18:35 ` John Snow
0 siblings, 1 reply; 34+ messages in thread
From: Kevin Wolf @ 2020-03-04 11:12 UTC (permalink / raw)
To: John Snow; +Cc: qemu-devel, qemu-block, Max Reitz
Am 03.03.2020 um 22:25 hat John Snow geschrieben:
>
>
> On 2/27/20 7:59 AM, Max Reitz wrote:
> > On 27.02.20 01:06, John Snow wrote:
> >> 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. This will be important if (when?) we
> >> begin adding type hints to our code base.
I'm not sure I understand this connection. mypy doesn't care about
style.
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >> tests/qemu-iotests/iotests.py | 88 ++++++++++++++++++-----------------
> >> 1 file changed, 45 insertions(+), 43 deletions(-)
> >
> > I feel like I’m the wrongest person there is for reviewing a Python
> > style-fixing patch, but here I am and so here I go:
>
> No, it's good.
Max can't be the wrongest person for it because that's already me.
> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >> index 8815052eb5..e8a0ea14fc 100644
> >> --- a/tests/qemu-iotests/iotests.py
> >> +++ b/tests/qemu-iotests/iotests.py
> >
> > [...]
> >
> >> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
> >> ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
> >> if exitcode == 0:
> >> return exitcode, ''
> >> - else:
> >> - return exitcode, subp.communicate()[0]
> >> + return exitcode, subp.communicate()[0]
> >
> > If we want to make such a change (which I don’t doubt we want), I think
> > it should be the other way around: Make the condition “exitcode != 0”,
> > so the final return is the return for the successful case. (Just
> > because I think that’s how we usually do it, at least in the qemu code?)
> >
> > [...]
> >
>
> Yes, makes sense. I was behaving a little more mechanically.
Here and...
> >> @@ -756,12 +750,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)
> >
> > I don’t mind the if alignment, but I do mind not aligning the third line
> > to the “edge” above it (i.e. the third line is part of the condition, so
> > I’d align it to the “if” condition).
> >
> > But then again it’s nothing new that I like to disagree with commonly
> > agreed-upon Python coding styles, so.
> >
> > [...]
> >
>
> OK, that can be addressed by highlighting the sub-expression with
> parentheses:
>
> node_id = next(edge['child'] for edge in graph['edges']
> if (edge['parent'] == node['id'] and
> edge['name'] == child_name))
...here I must say that while I think Max's suggestions feel like an
improvement to me over the patch, I actually like the current code best
in both cases.
In fact, after scanning your patch, I feel it's actually the majority of
changes that pylint wants that aren't an improvement... Maybe this just
underlines the fact that I am the wrongest person to review such patches
and not Max. Though I'm surprised that I'm generally not the person who
introduces the code violating the rules, and I don't have the impression
in this thread that anyone is eager to defend pylint's opinion.
Now I ran pylint myself and it prints some even more ridiculous warnings
like variable names being too short for its liking. I guess this means
that if we want to run it without warnings or errors, we need to use a
config file anyway to disable the worst parts.
And if we have a config file anyway, maybe we can more selectively
enable the checks that we actually want?
Kevin
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 1/9] iotests: do a light delinting
2020-03-04 11:12 ` Kevin Wolf
@ 2020-03-04 18:35 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-03-04 18:35 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, Max Reitz
On 3/4/20 6:12 AM, Kevin Wolf wrote:
> Am 03.03.2020 um 22:25 hat John Snow geschrieben:
>>
>>
>> On 2/27/20 7:59 AM, Max Reitz wrote:
>>> On 27.02.20 01:06, John Snow wrote:
>>>> 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. This will be important if (when?) we
>>>> begin adding type hints to our code base.
>
> I'm not sure I understand this connection. mypy doesn't care about
> style.
>
Each catches things the other doesn't -- and often getting mypy passing
involves new errors in non-type-checked contexts. I have personally
found it useful to *always use both tools*.
So having a pylint baseline will help ensure that -- while we transition
to mypy, and expect to see many errors during the transition -- we have
a solid baseline from the other tool to avoid regressions with.
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> tests/qemu-iotests/iotests.py | 88 ++++++++++++++++++-----------------
>>>> 1 file changed, 45 insertions(+), 43 deletions(-)
>>>
>>> I feel like I’m the wrongest person there is for reviewing a Python
>>> style-fixing patch, but here I am and so here I go:
>>
>> No, it's good.
>
> Max can't be the wrongest person for it because that's already me.
>
>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>> index 8815052eb5..e8a0ea14fc 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>
>>> [...]
>>>
>>>> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
>>>> ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
>>>> if exitcode == 0:
>>>> return exitcode, ''
>>>> - else:
>>>> - return exitcode, subp.communicate()[0]
>>>> + return exitcode, subp.communicate()[0]
>>>
>>> If we want to make such a change (which I don’t doubt we want), I think
>>> it should be the other way around: Make the condition “exitcode != 0”,
>>> so the final return is the return for the successful case. (Just
>>> because I think that’s how we usually do it, at least in the qemu code?)
>>>
>>> [...]
>>>
>>
>> Yes, makes sense. I was behaving a little more mechanically.
>
> Here and...
>
This check can be disabled I think legitimately.
>>>> @@ -756,12 +750,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)
>>>
>>> I don’t mind the if alignment, but I do mind not aligning the third line
>>> to the “edge” above it (i.e. the third line is part of the condition, so
>>> I’d align it to the “if” condition).
>>>
>>> But then again it’s nothing new that I like to disagree with commonly
>>> agreed-upon Python coding styles, so.
>>>
>>> [...]
>>>
>>
>> OK, that can be addressed by highlighting the sub-expression with
>> parentheses:
>>
>> node_id = next(edge['child'] for edge in graph['edges']
>> if (edge['parent'] == node['id'] and
>> edge['name'] == child_name))
>
> ...here I must say that while I think Max's suggestions feel like an
> improvement to me over the patch, I actually like the current code best
> in both cases.
>
This check I think *should* stay enabled to catch bad indenting, even if
it comes out looking subpar (subjective) in this one instance.
We might be able to fine-tune the indent checks, but I think for one
debated instance ... it's not important.
> In fact, after scanning your patch, I feel it's actually the majority of
> changes that pylint wants that aren't an improvement... Maybe this just
> underlines the fact that I am the wrongest person to review such patches
> and not Max. Though I'm surprised that I'm generally not the person who
> introduces the code violating the rules, and I don't have the impression
> in this thread that anyone is eager to defend pylint's opinion.
>
> Now I ran pylint myself and it prints some even more ridiculous warnings
> like variable names being too short for its liking. I guess this means
> that if we want to run it without warnings or errors, we need to use a
> config file anyway to disable the worst parts.
>
> And if we have a config file anyway, maybe we can more selectively
> enable the checks that we actually want?
>
> Kevin
>
See patch 9 for said conf file. I did disable most of the silly ones.
With this patch (and a few others later on) pylint is silent and can be
used for gating.
I do think that's worth a few indent styles that you feel are suboptimal.
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint
2020-03-04 0:02 ` Philippe Mathieu-Daudé
@ 2020-03-04 18:59 ` John Snow
0 siblings, 0 replies; 34+ messages in thread
From: John Snow @ 2020-03-04 18:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, Cleber Rosa, Eduardo Habkost, qemu-block, Max Reitz
On 3/3/20 7:02 PM, Philippe Mathieu-Daudé wrote:
> On 3/3/20 8:57 PM, John Snow wrote:
>> On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:
>>> On 2/27/20 1:06 AM, John Snow wrote:
>>>> 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.
>>>
>>> I'm tempted to NAck this and require an "installable python module"...
>>>
>>> Let's discuss why it is that hard!
>>>
>>
>> I've been tricked into this before. It's not work I am interested in
>> doing right now; it's WAY beyond the scope of what I am doing here.
>>
>> It involves properly factoring all of our python code, deciding which
>> portions are meant to be installed separately from QEMU itself, coming
>> up with a versioning scheme, packaging code, and moving code around in
>> many places.
>>
>> Then it involves coming up with tooling and infrastructure for creating
>> virtual environments, installing the right packages to it, and using it
>> to run our python tests.
>>
>> No, that's way too invasive. I'm not doing it and I will scream loudly
>> if you make me.
>>
>> A less invasive hack involves setting the python import path in a
>> consolidated spot so that python knows where it can import from. This
>> works, but might break the ability to run such tests as one-offs without
>> executing the environment setup.
>>
>> Again, not work I care to do right now and so I won't. The benefit of
>> these patches is to provide some minimum viable CI CQA for Python where
>> we had none before, NOT fix every possible Python problem in one shot.
>
> OK I guess we misunderstood each other :)
>
> I didn't understood your comment as personal to you for this patch, but
> generic. It makes sense it is not your priority and it is obvious this
> task will take a single developer a lot of time resources. I am
> certainly NOT asking you to do it.
>
> My question was rather community-oriented.
> (Cc'ing Eduardo because we talked about this after the last KVM forum).
>
OK, *that* is a fair question -- but you did threaten to reject the
patch, implying it was in-scope for this series. In no uncertain terms,
it is not.
HOWEVER ... since we're here, let's discuss python packaging.
This is the iotests code base. It is not intended to be packaged as
such, it's intended to be run in-tree or in the build folder. It will
rely on files being in specific paths and so on.
(I don't remember if iotests is designed to detect features at runtime
or if it still uses the build options to skip tests. Cleber would know
better, as he's battled this distinction in the past.)
Regardless, I think iotests depends on the qemu version AND the build
configuration, so iotests shouldn't be independently packaged or
packagable. It should remain "a collection of scripts" instead.
In this paradigm, python can only find descendant files. Importing from
subfolders using relative paths.
In this case, we're trying to climb *up* the path tree, which causes
grief. One often quoted solution is to use syspath hacking to
dynamically, at runtime, add parent folders to the PYTHONPATH.
Most static analysis tooling I have used to date is unable to cope with
this workaround -- hence pylint's failure to find the package being
imported.
(If we progress to using mypy, mypy will also be unable to cope with
this statement. It becomes clear we need to create a well defined
environment so tools know where they are allowed to look for sources.)
However, the thing we are trying to import is something that arguably
can be turned into an installable package as a light Python SDK for
interfacing with QEMU. That's worth doing. This code does not (AFAIK)
depend on any build configuration. That's good!
So there are a few approaches, and they're not mutually exclusive.
Option 1 *can* be a stopgap to option 2.
1. Use the iotest runners to set the PYTHONPATH to include the
python/qemu source tree directory. The imports will fail outside of the
test runner now, but we won't have to do any syspath hacking. This is
likely not important because the tests already require a good number of
environment variables set to function properly anyway.
This is a good workaround, but still outside of the scope for this series.
Notably, any pylint gating will need to occur under this specialized
environment (with PYTHONPATH set.) This might be more of a burden at times.
2. Convert the "python/qemu" folder into an installable "qemu" module.
The version of this package can track the `git describe` version well.
Once it is installable, there are several ways to use it:
A: Install it using pip/setuptools to the system. (pip3 install .)
B: Install it to the user's environment (pip3 install --user .)
C: Create a virtual environment (using whichever virtual environment
tool of your choice) and then having entered the venv, installing it:
(pip3 install .)
D: Any of the options above, but in editable/develop mode. In this mode,
the package is installed using symlinks that point back to the source
code files. This allows you to edit the package as you work without
having to reinstall after each change.
You'll use something like this:
pip3 install [--user] -e .
python3 setup.py develop [--user]
E: Continue just setting PYTHONPATH to point to the package when needed.
This is essentially editable/develop mode but without invoking setuptools.
At the moment, I don't believe we use any virtualenv configurations or
declare our dependencies natively to python tooling -- we rely on
configure to check for distribution packages.
We may want to get serious about python and begin using
virtualenvironments in places where it makes sense (instead of
PYTHONPATH hacking), but they are expensive to build so they should be
persistent so they can be re-engaged quickly to run tests and other scripts.
(A development venv might include our qemu package, the sphinx packages
locked to the correct versions, etc. We have not had major problems here
yet, but as our usage of python expands for document building and we
revamp QAPI, it may become more important to start fashioning these
bespoke environments to create more reproducible builds across different
environments.)
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 9/9] iotests: add pylintrc file
2020-03-04 7:22 ` Markus Armbruster
@ 2020-03-04 19:17 ` John Snow
2020-03-05 5:49 ` Markus Armbruster
0 siblings, 1 reply; 34+ messages in thread
From: John Snow @ 2020-03-04 19:17 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz
On 3/4/20 2:22 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>
> Start your sentences with a capital letter, please.
>
The German complains about the capitalization, but not the sentence
fragment.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>> 1 file changed, 20 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..feed506f75
>> --- /dev/null
>> +++ b/tests/qemu-iotests/pylintrc
>> @@ -0,0 +1,20 @@
>> +[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,
>> + missing-docstring,
>> + line-too-long,
>> + too-many-lines,
>> + too-few-public-methods,
>> + too-many-arguments,
>> + too-many-locals,
>> + too-many-branches,
>> + too-many-public-methods,
>> \ No newline at end of file
>
> Add the newline, please.
>
> German pejorative for the too-many- and too-few- warnings: "Müsli".
> Implies it's for muesli-knitters / granola-crunchers indulging their
> orthorexia.
>
They are useful at times as they can suggest when you are going a bit
overboard on "organically grown" design. For cleaning an existing
codebase, it's more of a hindrance to the immediate goal of establishing
a baseline.
(*cough* I try to adhere to them in my own freshly written code, and
disable per-line when I've decided to veto the suggestion. Not
appropriate for a codebase like ours. As Max reminds me, it's just tests
-- don't make them too clever or pretty.)
Regardless. It's not appropriate here and now.
> missing-docstring is not advisable for libraries. Feels okay here.
>
Ideally we do start using them, but it's out of scope here. Since I did
some cleanup, I wanted to establish the baseline of what I adhered to.
*not* suggest that it's the destination state.
Adding proper docstrings should be done during mypy conversion once the
types are determined, understood, and enforced. Not before then.
> line-too-long might be worth cleaning up. How many of them do we have
> now?
>
Five in iotests.py using the default length of 100. 15 if I limit to 80.
If we agree that 100 is okay, I can tackle this in an amendment patch.
If 80 is okay, I'm going to put it off as one coat of paint too many.
(I will try to clean up the 100+ lines for my next version. I am
hesitant to make a deeper cut because I have the feeling it's the type
of series that will incur a lot of nitpicks on indent style.)
--js
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6 9/9] iotests: add pylintrc file
2020-03-04 19:17 ` John Snow
@ 2020-03-05 5:49 ` Markus Armbruster
0 siblings, 0 replies; 34+ messages in thread
From: Markus Armbruster @ 2020-03-05 5:49 UTC (permalink / raw)
To: John Snow
Cc: Kevin Wolf, Max Reitz, Markus Armbruster, qemu-block, qemu-devel
John Snow <jsnow@redhat.com> writes:
> On 3/4/20 2:22 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Repeatable results. run `pylint iotests.py` and you should get a pass.
>>
>> Start your sentences with a capital letter, please.
>>
>
> The German complains about the capitalization, but not the sentence
> fragment.
Heh!
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++
>>> 1 file changed, 20 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..feed506f75
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/pylintrc
>>> @@ -0,0 +1,20 @@
>>> +[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,
>>> + missing-docstring,
>>> + line-too-long,
>>> + too-many-lines,
>>> + too-few-public-methods,
>>> + too-many-arguments,
>>> + too-many-locals,
>>> + too-many-branches,
>>> + too-many-public-methods,
>>> \ No newline at end of file
>>
>> Add the newline, please.
>>
>> German pejorative for the too-many- and too-few- warnings: "Müsli".
>> Implies it's for muesli-knitters / granola-crunchers indulging their
>> orthorexia.
>>
>
> They are useful at times as they can suggest when you are going a bit
> overboard on "organically grown" design. For cleaning an existing
> codebase, it's more of a hindrance to the immediate goal of establishing
> a baseline.
Yes, gentle nudges to reconsider your code organization can be useful.
But when we run pylint with the goal of getting no output, even warnings
are much more than gentle nudges.
> (*cough* I try to adhere to them in my own freshly written code, and
> disable per-line when I've decided to veto the suggestion. Not
> appropriate for a codebase like ours. As Max reminds me, it's just tests
> -- don't make them too clever or pretty.)
>
> Regardless. It's not appropriate here and now.
>
>> missing-docstring is not advisable for libraries. Feels okay here.
>>
>
> Ideally we do start using them, but it's out of scope here. Since I did
> some cleanup, I wanted to establish the baseline of what I adhered to.
>
> *not* suggest that it's the destination state.
>
> Adding proper docstrings should be done during mypy conversion once the
> types are determined, understood, and enforced. Not before then.
>
>> line-too-long might be worth cleaning up. How many of them do we have
>> now?
>>
>
> Five in iotests.py using the default length of 100. 15 if I limit to 80.
>
> If we agree that 100 is okay, I can tackle this in an amendment patch.
> If 80 is okay, I'm going to put it off as one coat of paint too many.
>
> (I will try to clean up the 100+ lines for my next version. I am
> hesitant to make a deeper cut because I have the feeling it's the type
> of series that will incur a lot of nitpicks on indent style.)
One step at a time.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2020-03-05 5:50 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-27 0:06 [PATCH v6 0/9] iotests: use python logging John Snow
2020-02-27 0:06 ` [PATCH v6 1/9] iotests: do a light delinting John Snow
2020-02-27 12:59 ` Max Reitz
2020-03-03 21:25 ` John Snow
2020-03-04 11:12 ` Kevin Wolf
2020-03-04 18:35 ` John Snow
2020-02-27 0:06 ` [PATCH v6 2/9] iotests: add script_initialize John Snow
2020-02-27 13:47 ` Max Reitz
2020-03-03 21:12 ` John Snow
2020-02-27 0:06 ` [PATCH v6 3/9] iotests: replace mutable list default args John Snow
2020-02-27 13:50 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 4/9] iotest 258: use script_main John Snow
2020-02-27 13:55 ` Max Reitz
2020-02-27 14:10 ` Philippe Mathieu-Daudé
2020-02-27 0:06 ` [PATCH v6 5/9] iotests: Mark verify functions as private John Snow
2020-02-27 13:59 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 6/9] iotests: use python logging for iotests.log() John Snow
2020-02-27 14:21 ` Max Reitz
2020-03-03 20:00 ` John Snow
2020-02-27 0:06 ` [PATCH v6 7/9] iotests: ignore import warnings from pylint John Snow
2020-02-27 14:14 ` Philippe Mathieu-Daudé
2020-03-03 19:57 ` John Snow
2020-03-04 0:02 ` Philippe Mathieu-Daudé
2020-03-04 18:59 ` John Snow
2020-02-27 0:06 ` [PATCH v6 8/9] iotests: don't use 'format' for drive_add John Snow
2020-02-27 14:12 ` Philippe Mathieu-Daudé
2020-02-27 14:26 ` Max Reitz
2020-02-27 0:06 ` [PATCH v6 9/9] iotests: add pylintrc file John Snow
2020-02-27 1:57 ` Eric Blake
2020-02-27 14:11 ` Philippe Mathieu-Daudé
2020-03-03 19:52 ` John Snow
2020-03-04 7:22 ` Markus Armbruster
2020-03-04 19:17 ` John Snow
2020-03-05 5:49 ` Markus Armbruster
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.