| Message ID | 20200317004105.27059-8-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: use python logging | expand |
On 3/17/20 1:41 AM, John Snow wrote: > 79 is the PEP8 recommendation. This recommendation works well for > reading patch diffs in TUI email clients. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ > tests/qemu-iotests/pylintrc | 6 +++- > 2 files changed, 47 insertions(+), 23 deletions(-) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 3d90fb157d..75fd697d77 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -77,9 +77,11 @@ > def qemu_img(*args): > '''Run qemu-img and return the exit code''' > devnull = open('/dev/null', 'r+') > - exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, stdout=devnull) > + exitcode = subprocess.call(qemu_img_args + list(args), > + stdin=devnull, stdout=devnull) > if exitcode < 0: > - sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args)))) > + sys.stderr.write('qemu-img received signal %i: %s\n' > + % (-exitcode, ' '.join(qemu_img_args + list(args)))) > return exitcode > > def ordered_qmp(qmsg, conv_keys=True): > @@ -118,7 +120,8 @@ def qemu_img_verbose(*args): > '''Run qemu-img without suppressing its output and return the exit code''' > exitcode = subprocess.call(qemu_img_args + list(args)) > if exitcode < 0: > - sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args)))) > + sys.stderr.write('qemu-img received signal %i: %s\n' > + % (-exitcode, ' '.join(qemu_img_args + list(args)))) > return exitcode > > def qemu_img_pipe(*args): > @@ -129,7 +132,8 @@ def qemu_img_pipe(*args): > universal_newlines=True) > exitcode = subp.wait() > if exitcode < 0: > - sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args)))) > + sys.stderr.write('qemu-img received signal %i: %s\n' > + % (-exitcode, ' '.join(qemu_img_args + list(args)))) > return subp.communicate()[0] > > def qemu_img_log(*args): > @@ -159,7 +163,8 @@ def qemu_io(*args): > universal_newlines=True) > exitcode = subp.wait() > if exitcode < 0: > - sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args))) > + sys.stderr.write('qemu-io received signal %i: %s\n' > + % (-exitcode, ' '.join(args))) > return subp.communicate()[0] > > def qemu_io_log(*args): > @@ -281,10 +286,13 @@ def filter_test_dir(msg): > def filter_win32(msg): > return win32_re.sub("", msg) > > -qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)") > +qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* " > + r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec " > + r"and [0-9\/.inf]* ops\/sec\)") > def filter_qemu_io(msg): > msg = filter_win32(msg) > - return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", msg) > + return qemu_io_re.sub("X ops; XX:XX:XX.X " > + "(XXX YYY/sec and XXX ops/sec)", msg) > > chown_re = re.compile(r"chown [0-9]+:[0-9]+") > def filter_chown(msg): > @@ -336,7 +344,9 @@ def filter_img_info(output, filename): > line = line.replace(filename, 'TEST_IMG') \ > .replace(imgfmt, 'IMGFMT') > line = re.sub('iters: [0-9]+', 'iters: XXX', line) > - line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line) > + line = re.sub('uuid: [-a-f0-9]+', > + 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', > + line) > line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line) > lines.append(line) > return '\n'.join(lines) > @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): > self.pause_drive(drive, "write_aio") > return > self.qmp('human-monitor-command', > - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) > + command_line='qemu-io %s "break %s bp_%s"' > + % (drive, event, drive)) > > def resume_drive(self, drive): > self.qmp('human-monitor-command', > - command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive)) > + command_line='qemu-io %s "remove_break bp_%s"' > + % (drive, drive)) > > def hmp_qemu_io(self, drive, cmd): > '''Write to a given drive using an HMP command''' > @@ -793,16 +805,18 @@ def dictpath(self, d, path): > idx = int(idx) > > if not isinstance(d, dict) or component not in d: > - self.fail('failed path traversal for "%s" in "%s"' % (path, str(d))) > + self.fail(f'failed path traversal for "{path}" in "{d}"') > d = d[component] > > if m: > if not isinstance(d, list): > - self.fail('path component "%s" in "%s" is not a list in "%s"' % (component, path, str(d))) > + self.fail(f'path component "{component}" in "{path}" ' > + f'is not a list in "{d}"') > try: > d = d[idx] > except IndexError: > - self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d))) > + self.fail(f'invalid index "{idx}" in path "{path}" ' > + f'in "{d}"') > return d > > def assert_qmp_absent(self, d, path): > @@ -853,10 +867,13 @@ def assert_json_filename_equal(self, json_filename, reference): > '''Asserts that the given filename is a json: filename and that its > content is equal to the given reference object''' > self.assertEqual(json_filename[:5], 'json:') > - self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])), > - self.vm.flatten_qmp_object(reference)) > + self.assertEqual( > + self.vm.flatten_qmp_object(json.loads(json_filename[5:])), > + self.vm.flatten_qmp_object(reference) > + ) > > - def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0): > + def cancel_and_wait(self, drive='drive0', force=False, > + resume=False, wait=60.0): > '''Cancel a block job and wait for it to finish, returning the event''' > result = self.vm.qmp('block-job-cancel', device=drive, force=force) > self.assert_qmp(result, 'return', {}) > @@ -880,8 +897,8 @@ def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0): > self.assert_no_active_block_jobs() > return result > > - def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0, > - error=None): > + def wait_until_completed(self, drive='drive0', check_offset=True, > + wait=60.0, error=None): > '''Wait for a block job to finish, returning the event''' > while True: > for event in self.vm.get_qmp_events(wait=wait): > @@ -1020,8 +1037,11 @@ def verify_quorum(): > notrun('quorum support missing') > > def qemu_pipe(*args): > - '''Run qemu with an option to print something and exit (e.g. a help option), > - and return its output''' > + """ > + Run qemu with an option to print something and exit (e.g. a help option). > + > + :return: QEMU's stdout output. > + """ > args = [qemu_prog] + qemu_opts + list(args) > subp = subprocess.Popen(args, stdout=subprocess.PIPE, > stderr=subprocess.STDOUT, > @@ -1059,8 +1079,8 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs): > > usf_list = list(set(fmts) - set(supported_formats(read_only))) > if usf_list: > - test_case.case_skip('{}: formats {} are not whitelisted'.format( > - test_case, usf_list)) > + msg = f'{test_case}: formats {usf_list} are not whitelisted' > + test_case.case_skip(msg) > return None > else: > return func(test_case, *args, **kwargs) > diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc > index 8720b6a0de..8d02f00607 100644 > --- a/tests/qemu-iotests/pylintrc > +++ b/tests/qemu-iotests/pylintrc > @@ -19,4 +19,8 @@ disable=invalid-name, > too-many-public-methods, > # These are temporary, and should be removed: > missing-docstring, > - line-too-long, > + > +[FORMAT] > + > +# Maximum number of characters on a single line. > +max-line-length=79 > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 3/17/20 6:36 AM, Philippe Mathieu-Daudé wrote: > On 3/17/20 1:41 AM, John Snow wrote: >> 79 is the PEP8 recommendation. This recommendation works well for >> reading patch diffs in TUI email clients. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ >> tests/qemu-iotests/pylintrc | 6 +++- >> 2 files changed, 47 insertions(+), 23 deletions(-) >> >> diff --git a/tests/qemu-iotests/iotests.py >> b/tests/qemu-iotests/iotests.py >> index 3d90fb157d..75fd697d77 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py >> @@ -77,9 +77,11 @@ >> def qemu_img(*args): >> '''Run qemu-img and return the exit code''' >> devnull = open('/dev/null', 'r+') >> - exitcode = subprocess.call(qemu_img_args + list(args), >> stdin=devnull, stdout=devnull) >> + exitcode = subprocess.call(qemu_img_args + list(args), >> + stdin=devnull, stdout=devnull) >> if exitcode < 0: >> - sys.stderr.write('qemu-img received signal %i: %s\n' % >> (-exitcode, ' '.join(qemu_img_args + list(args)))) >> + sys.stderr.write('qemu-img received signal %i: %s\n' >> + % (-exitcode, ' '.join(qemu_img_args + >> list(args)))) >> return exitcode >> def ordered_qmp(qmsg, conv_keys=True): >> @@ -118,7 +120,8 @@ def qemu_img_verbose(*args): >> '''Run qemu-img without suppressing its output and return the >> exit code''' >> exitcode = subprocess.call(qemu_img_args + list(args)) >> if exitcode < 0: >> - sys.stderr.write('qemu-img received signal %i: %s\n' % >> (-exitcode, ' '.join(qemu_img_args + list(args)))) >> + sys.stderr.write('qemu-img received signal %i: %s\n' >> + % (-exitcode, ' '.join(qemu_img_args + >> list(args)))) >> return exitcode >> def qemu_img_pipe(*args): >> @@ -129,7 +132,8 @@ def qemu_img_pipe(*args): >> universal_newlines=True) >> exitcode = subp.wait() >> if exitcode < 0: >> - sys.stderr.write('qemu-img received signal %i: %s\n' % >> (-exitcode, ' '.join(qemu_img_args + list(args)))) >> + sys.stderr.write('qemu-img received signal %i: %s\n' >> + % (-exitcode, ' '.join(qemu_img_args + >> list(args)))) >> return subp.communicate()[0] >> def qemu_img_log(*args): >> @@ -159,7 +163,8 @@ def qemu_io(*args): >> universal_newlines=True) >> exitcode = subp.wait() >> if exitcode < 0: >> - sys.stderr.write('qemu-io received signal %i: %s\n' % >> (-exitcode, ' '.join(args))) >> + sys.stderr.write('qemu-io received signal %i: %s\n' >> + % (-exitcode, ' '.join(args))) >> return subp.communicate()[0] >> def qemu_io_log(*args): >> @@ -281,10 +286,13 @@ def filter_test_dir(msg): >> def filter_win32(msg): >> return win32_re.sub("", msg) >> -qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* >> [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)") >> +qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* " >> + r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec " >> + r"and [0-9\/.inf]* ops\/sec\)") >> def filter_qemu_io(msg): >> msg = filter_win32(msg) >> - return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX >> ops/sec)", msg) >> + return qemu_io_re.sub("X ops; XX:XX:XX.X " >> + "(XXX YYY/sec and XXX ops/sec)", msg) >> chown_re = re.compile(r"chown [0-9]+:[0-9]+") >> def filter_chown(msg): >> @@ -336,7 +344,9 @@ def filter_img_info(output, filename): >> line = line.replace(filename, 'TEST_IMG') \ >> .replace(imgfmt, 'IMGFMT') >> line = re.sub('iters: [0-9]+', 'iters: XXX', line) >> - line = re.sub('uuid: [-a-f0-9]+', 'uuid: >> XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line) >> + line = re.sub('uuid: [-a-f0-9]+', >> + 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', >> + line) >> line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line) >> lines.append(line) >> return '\n'.join(lines) >> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): >> self.pause_drive(drive, "write_aio") >> return >> self.qmp('human-monitor-command', >> - command_line='qemu-io %s "break %s bp_%s"' % (drive, >> event, drive)) >> + command_line='qemu-io %s "break %s bp_%s"' >> + % (drive, event, drive)) >> def resume_drive(self, drive): >> self.qmp('human-monitor-command', >> - command_line='qemu-io %s "remove_break bp_%s"' % >> (drive, drive)) >> + command_line='qemu-io %s "remove_break bp_%s"' >> + % (drive, drive)) >> def hmp_qemu_io(self, drive, cmd): >> '''Write to a given drive using an HMP command''' >> @@ -793,16 +805,18 @@ def dictpath(self, d, path): >> idx = int(idx) >> if not isinstance(d, dict) or component not in d: >> - self.fail('failed path traversal for "%s" in "%s"' % >> (path, str(d))) >> + self.fail(f'failed path traversal for "{path}" in >> "{d}"') >> d = d[component] >> if m: >> if not isinstance(d, list): >> - self.fail('path component "%s" in "%s" is not a >> list in "%s"' % (component, path, str(d))) >> + self.fail(f'path component "{component}" in >> "{path}" ' >> + f'is not a list in "{d}"') >> try: >> d = d[idx] >> except IndexError: >> - self.fail('invalid index "%s" in path "%s" in >> "%s"' % (idx, path, str(d))) >> + self.fail(f'invalid index "{idx}" in path "{path}" ' >> + f'in "{d}"') >> return d >> def assert_qmp_absent(self, d, path): >> @@ -853,10 +867,13 @@ def assert_json_filename_equal(self, >> json_filename, reference): >> '''Asserts that the given filename is a json: filename and >> that its >> content is equal to the given reference object''' >> self.assertEqual(json_filename[:5], 'json:') >> - >> self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])), >> >> - self.vm.flatten_qmp_object(reference)) >> + self.assertEqual( >> + self.vm.flatten_qmp_object(json.loads(json_filename[5:])), >> + self.vm.flatten_qmp_object(reference) >> + ) >> - def cancel_and_wait(self, drive='drive0', force=False, >> resume=False, wait=60.0): >> + def cancel_and_wait(self, drive='drive0', force=False, >> + resume=False, wait=60.0): >> '''Cancel a block job and wait for it to finish, returning >> the event''' >> result = self.vm.qmp('block-job-cancel', device=drive, >> force=force) >> self.assert_qmp(result, 'return', {}) >> @@ -880,8 +897,8 @@ def cancel_and_wait(self, drive='drive0', >> force=False, resume=False, wait=60.0): >> self.assert_no_active_block_jobs() >> return result >> - def wait_until_completed(self, drive='drive0', >> check_offset=True, wait=60.0, >> - error=None): >> + def wait_until_completed(self, drive='drive0', check_offset=True, >> + wait=60.0, error=None): >> '''Wait for a block job to finish, returning the event''' >> while True: >> for event in self.vm.get_qmp_events(wait=wait): >> @@ -1020,8 +1037,11 @@ def verify_quorum(): >> notrun('quorum support missing') >> def qemu_pipe(*args): >> - '''Run qemu with an option to print something and exit (e.g. a >> help option), >> - and return its output''' >> + """ >> + Run qemu with an option to print something and exit (e.g. a help >> option). >> + >> + :return: QEMU's stdout output. >> + """ >> args = [qemu_prog] + qemu_opts + list(args) >> subp = subprocess.Popen(args, stdout=subprocess.PIPE, >> stderr=subprocess.STDOUT, >> @@ -1059,8 +1079,8 @@ def func_wrapper(test_case: QMPTestCase, *args, >> **kwargs): >> usf_list = list(set(fmts) - >> set(supported_formats(read_only))) >> if usf_list: >> - test_case.case_skip('{}: formats {} are not >> whitelisted'.format( >> - test_case, usf_list)) >> + msg = f'{test_case}: formats {usf_list} are not >> whitelisted' >> + test_case.case_skip(msg) >> return None >> else: >> return func(test_case, *args, **kwargs) >> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc >> index 8720b6a0de..8d02f00607 100644 >> --- a/tests/qemu-iotests/pylintrc >> +++ b/tests/qemu-iotests/pylintrc >> @@ -19,4 +19,8 @@ disable=invalid-name, >> too-many-public-methods, >> # These are temporary, and should be removed: >> missing-docstring, >> - line-too-long, >> + >> +[FORMAT] >> + >> +# Maximum number of characters on a single line. >> +max-line-length=79 >> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Thanks :)
On 17.03.20 01:41, John Snow wrote: > 79 is the PEP8 recommendation. This recommendation works well for > reading patch diffs in TUI email clients. Also for my very GUI-y diff program (kompare). > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ > tests/qemu-iotests/pylintrc | 6 +++- > 2 files changed, 47 insertions(+), 23 deletions(-) > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 3d90fb157d..75fd697d77 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py [...] > @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): > self.pause_drive(drive, "write_aio") > return > self.qmp('human-monitor-command', > - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) > + command_line='qemu-io %s "break %s bp_%s"' > + % (drive, event, drive)) Can we put this value in a variable instead? I don’t like the % aligning with the parameter name instead of the string value. (I also don’t like how there are no spaces around the assignment =, but around the %, even though the % binds more strongly.) > > 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''' > @@ -793,16 +805,18 @@ def dictpath(self, d, path): > idx = int(idx) > > if not isinstance(d, dict) or component not in d: > - self.fail('failed path traversal for "%s" in "%s"' % (path, str(d))) > + self.fail(f'failed path traversal for "{path}" in "{d}"') Do we require 3.6 so that f-strings are guaranteed to work? (I thought we didn’t. I’d be happy to use them.) Max
On 24.03.20 16:02, Max Reitz wrote: > On 17.03.20 01:41, John Snow wrote: >> 79 is the PEP8 recommendation. This recommendation works well for >> reading patch diffs in TUI email clients. > > Also for my very GUI-y diff program (kompare). > >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ >> tests/qemu-iotests/pylintrc | 6 +++- >> 2 files changed, 47 insertions(+), 23 deletions(-) >> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >> index 3d90fb157d..75fd697d77 100644 >> --- a/tests/qemu-iotests/iotests.py >> +++ b/tests/qemu-iotests/iotests.py > > [...] > >> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): >> self.pause_drive(drive, "write_aio") >> return >> self.qmp('human-monitor-command', >> - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) >> + command_line='qemu-io %s "break %s bp_%s"' >> + % (drive, event, drive)) > > Can we put this value in a variable instead? I don’t like the % > aligning with the parameter name instead of the string value. (I also > don’t like how there are no spaces around the assignment =, but around > the %, even though the % binds more strongly.) > >> >> 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''' >> @@ -793,16 +805,18 @@ def dictpath(self, d, path): >> idx = int(idx) >> >> if not isinstance(d, dict) or component not in d: >> - self.fail('failed path traversal for "%s" in "%s"' % (path, str(d))) >> + self.fail(f'failed path traversal for "{path}" in "{d}"') > > Do we require 3.6 so that f-strings are guaranteed to work? (I thought > we didn’t. I’d be happy to use them.) Oh. Of course we do. It says so in this file that this whole series is about. Max
On 3/24/20 11:12 AM, Max Reitz wrote: > On 24.03.20 16:02, Max Reitz wrote: >> On 17.03.20 01:41, John Snow wrote: >>> 79 is the PEP8 recommendation. This recommendation works well for >>> reading patch diffs in TUI email clients. >> >> Also for my very GUI-y diff program (kompare). >> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ >>> tests/qemu-iotests/pylintrc | 6 +++- >>> 2 files changed, 47 insertions(+), 23 deletions(-) >>> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>> index 3d90fb157d..75fd697d77 100644 >>> --- a/tests/qemu-iotests/iotests.py >>> +++ b/tests/qemu-iotests/iotests.py >> >> [...] >> >>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): >>> self.pause_drive(drive, "write_aio") >>> return >>> self.qmp('human-monitor-command', >>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) >>> + command_line='qemu-io %s "break %s bp_%s"' >>> + % (drive, event, drive)) >> >> Can we put this value in a variable instead? I don’t like the % >> aligning with the parameter name instead of the string value. (I also >> don’t like how there are no spaces around the assignment =, but around >> the %, even though the % binds more strongly.) >> >>> I think I had another patch somewhere that added an HMP helper that fixes this issue for this particular instance. I can send that separately as a follow-up. I think otherwise, unfortunately, "we" "decided" that this indent style is "best". So I think that this patch is "correct". (All of the other options for indent styles seemed to be worse visually, or actively go against PEP8. While PEP8 is not a bible, every conscious choice to disregard it generally means you will be fighting a CQA tool at some other point in time. I have therefore adopted a "When in Rome" policy to reduce friction wherever possible with pylint, flake8, mypy, pycharm, and so on.) ((I would prefer we begin to deprecate uses of % and begin using .format() and f-strings wherever possible to help alleviate this kind of syntax, too -- but I must insist that's for another series.)) >>> 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''' >>> @@ -793,16 +805,18 @@ def dictpath(self, d, path): >>> idx = int(idx) >>> >>> if not isinstance(d, dict) or component not in d: >>> - self.fail('failed path traversal for "%s" in "%s"' % (path, str(d))) >>> + self.fail(f'failed path traversal for "{path}" in "{d}"') >> >> Do we require 3.6 so that f-strings are guaranteed to work? (I thought >> we didn’t. I’d be happy to use them.) > > Oh. Of course we do. It says so in this file that this whole series is > about. > Yup. I didn't like the idea of iotests using a newer version of python than our build system, but my concern was not shared, so we get to use f-strings for non-buildtime tools. End of support for Python 3.5 will be 2020-09-13; so we'll get to use f-strings everywhere else soon, too. --js
On 24.03.20 18:09, John Snow wrote: > > > On 3/24/20 11:12 AM, Max Reitz wrote: >> On 24.03.20 16:02, Max Reitz wrote: >>> On 17.03.20 01:41, John Snow wrote: >>>> 79 is the PEP8 recommendation. This recommendation works well for >>>> reading patch diffs in TUI email clients. >>> >>> Also for my very GUI-y diff program (kompare). >>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ >>>> tests/qemu-iotests/pylintrc | 6 +++- >>>> 2 files changed, 47 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py >>>> index 3d90fb157d..75fd697d77 100644 >>>> --- a/tests/qemu-iotests/iotests.py >>>> +++ b/tests/qemu-iotests/iotests.py >>> >>> [...] >>> >>>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): >>>> self.pause_drive(drive, "write_aio") >>>> return >>>> self.qmp('human-monitor-command', >>>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) >>>> + command_line='qemu-io %s "break %s bp_%s"' >>>> + % (drive, event, drive)) >>> >>> Can we put this value in a variable instead? I don’t like the % >>> aligning with the parameter name instead of the string value. (I also >>> don’t like how there are no spaces around the assignment =, but around >>> the %, even though the % binds more strongly.) >>> >>>> > > I think I had another patch somewhere that added an HMP helper that > fixes this issue for this particular instance. > > I can send that separately as a follow-up. I think otherwise, > unfortunately, "we" "decided" that this indent style is "best". > > So I think that this patch is "correct". Perhaps it’s the best if we assume that we had to do it this way, but can’t we just do a qemu_io_cmd = f'qemu-io {drive} "break {event} bp_{drive}"' self.qmp('human-monitor-command', command_line=qemu_io_cmd) ? (Or maybe an f-string inline. I was thinking of a separate variable because I wasn’t aware that f-strings would be an option until I got to later hunks of this patch...) > (All of the other options for indent styles seemed to be worse visually, > or actively go against PEP8. While PEP8 is not a bible, every conscious > choice to disregard it generally means you will be fighting a CQA tool > at some other point in time. I have therefore adopted a "When in Rome" > policy to reduce friction wherever possible with pylint, flake8, mypy, > pycharm, and so on.) > > > ((I would prefer we begin to deprecate uses of % and begin using > .format() and f-strings wherever possible to help alleviate this kind of > syntax, too -- but I must insist that's for another series.)) Hm. But you do change something to f-strings below, why not here? Max
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 3d90fb157d..75fd697d77 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -77,9 +77,11 @@ def qemu_img(*args): '''Run qemu-img and return the exit code''' devnull = open('/dev/null', 'r+') - exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, stdout=devnull) + exitcode = subprocess.call(qemu_img_args + list(args), + stdin=devnull, stdout=devnull) if exitcode < 0: - sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args)))) + sys.stderr.write('qemu-img received signal %i: %s\n' + % (-exitcode, ' '.join(qemu_img_args + list(args)))) return exitcode def ordered_qmp(qmsg, conv_keys=True): @@ -118,7 +120,8 @@ def qemu_img_verbose(*args): '''Run qemu-img without suppressing its output and return the exit code''' exitcode = subprocess.call(qemu_img_args + list(args)) if exitcode < 0: - sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args)))) + sys.stderr.write('qemu-img received signal %i: %s\n' + % (-exitcode, ' '.join(qemu_img_args + list(args)))) return exitcode def qemu_img_pipe(*args): @@ -129,7 +132,8 @@ def qemu_img_pipe(*args): universal_newlines=True) exitcode = subp.wait() if exitcode < 0: - sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args)))) + sys.stderr.write('qemu-img received signal %i: %s\n' + % (-exitcode, ' '.join(qemu_img_args + list(args)))) return subp.communicate()[0] def qemu_img_log(*args): @@ -159,7 +163,8 @@ def qemu_io(*args): universal_newlines=True) exitcode = subp.wait() if exitcode < 0: - sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args))) + sys.stderr.write('qemu-io received signal %i: %s\n' + % (-exitcode, ' '.join(args))) return subp.communicate()[0] def qemu_io_log(*args): @@ -281,10 +286,13 @@ def filter_test_dir(msg): def filter_win32(msg): return win32_re.sub("", msg) -qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)") +qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* " + r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec " + r"and [0-9\/.inf]* ops\/sec\)") def filter_qemu_io(msg): msg = filter_win32(msg) - return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", msg) + return qemu_io_re.sub("X ops; XX:XX:XX.X " + "(XXX YYY/sec and XXX ops/sec)", msg) chown_re = re.compile(r"chown [0-9]+:[0-9]+") def filter_chown(msg): @@ -336,7 +344,9 @@ def filter_img_info(output, filename): line = line.replace(filename, 'TEST_IMG') \ .replace(imgfmt, 'IMGFMT') line = re.sub('iters: [0-9]+', 'iters: XXX', line) - line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line) + line = re.sub('uuid: [-a-f0-9]+', + 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', + line) line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line) lines.append(line) return '\n'.join(lines) @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None): self.pause_drive(drive, "write_aio") return self.qmp('human-monitor-command', - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) + command_line='qemu-io %s "break %s bp_%s"' + % (drive, event, drive)) def resume_drive(self, drive): self.qmp('human-monitor-command', - command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive)) + command_line='qemu-io %s "remove_break bp_%s"' + % (drive, drive)) def hmp_qemu_io(self, drive, cmd): '''Write to a given drive using an HMP command''' @@ -793,16 +805,18 @@ def dictpath(self, d, path): idx = int(idx) if not isinstance(d, dict) or component not in d: - self.fail('failed path traversal for "%s" in "%s"' % (path, str(d))) + self.fail(f'failed path traversal for "{path}" in "{d}"') d = d[component] if m: if not isinstance(d, list): - self.fail('path component "%s" in "%s" is not a list in "%s"' % (component, path, str(d))) + self.fail(f'path component "{component}" in "{path}" ' + f'is not a list in "{d}"') try: d = d[idx] except IndexError: - self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d))) + self.fail(f'invalid index "{idx}" in path "{path}" ' + f'in "{d}"') return d def assert_qmp_absent(self, d, path): @@ -853,10 +867,13 @@ def assert_json_filename_equal(self, json_filename, reference): '''Asserts that the given filename is a json: filename and that its content is equal to the given reference object''' self.assertEqual(json_filename[:5], 'json:') - self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])), - self.vm.flatten_qmp_object(reference)) + self.assertEqual( + self.vm.flatten_qmp_object(json.loads(json_filename[5:])), + self.vm.flatten_qmp_object(reference) + ) - def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0): + def cancel_and_wait(self, drive='drive0', force=False, + resume=False, wait=60.0): '''Cancel a block job and wait for it to finish, returning the event''' result = self.vm.qmp('block-job-cancel', device=drive, force=force) self.assert_qmp(result, 'return', {}) @@ -880,8 +897,8 @@ def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0): self.assert_no_active_block_jobs() return result - def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0, - error=None): + def wait_until_completed(self, drive='drive0', check_offset=True, + wait=60.0, error=None): '''Wait for a block job to finish, returning the event''' while True: for event in self.vm.get_qmp_events(wait=wait): @@ -1020,8 +1037,11 @@ def verify_quorum(): notrun('quorum support missing') def qemu_pipe(*args): - '''Run qemu with an option to print something and exit (e.g. a help option), - and return its output''' + """ + Run qemu with an option to print something and exit (e.g. a help option). + + :return: QEMU's stdout output. + """ args = [qemu_prog] + qemu_opts + list(args) subp = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, @@ -1059,8 +1079,8 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs): usf_list = list(set(fmts) - set(supported_formats(read_only))) if usf_list: - test_case.case_skip('{}: formats {} are not whitelisted'.format( - test_case, usf_list)) + msg = f'{test_case}: formats {usf_list} are not whitelisted' + test_case.case_skip(msg) return None else: return func(test_case, *args, **kwargs) diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc index 8720b6a0de..8d02f00607 100644 --- a/tests/qemu-iotests/pylintrc +++ b/tests/qemu-iotests/pylintrc @@ -19,4 +19,8 @@ disable=invalid-name, too-many-public-methods, # These are temporary, and should be removed: missing-docstring, - line-too-long, + +[FORMAT] + +# Maximum number of characters on a single line. +max-line-length=79
79 is the PEP8 recommendation. This recommendation works well for reading patch diffs in TUI email clients. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qemu-iotests/iotests.py | 64 +++++++++++++++++++++++------------ tests/qemu-iotests/pylintrc | 6 +++- 2 files changed, 47 insertions(+), 23 deletions(-)