| Message ID | 20200227000639.9644-2-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: use python logging | expand |
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
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
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
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
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):
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(-)