Re: [PATCH v7 06/10] iotests: limit line length to 79 chars

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/4/20 10:38 PM, 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 | 69 ++++++++++++++++++++++-------------
>   tests/qemu-iotests/pylintrc   |  6 ++-
>   2 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 54d68774e1..1be11f491f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -76,9 +76,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))))

Do we want to indent Python like C and align argument below opening 
parenthesis? Except when using sys.stderr.write() you seem to do it.

>       return exitcode
>   
>   def ordered_qmp(qmsg, conv_keys=True):
> @@ -117,7 +119,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):
> @@ -128,7 +131,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):
> @@ -158,7 +162,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):
> @@ -280,10 +285,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):
> @@ -335,7 +343,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)
> @@ -356,12 +366,9 @@ def log(msg, filters=(), indent=None):
>       for flt in filters:
>           msg = flt(msg)
>       if isinstance(msg, (dict, list)):
> -        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
> -        separators = (', ', ': ') if indent is None else (',', ': ')
>           # Don't sort if it's already sorted
>           do_sort = not isinstance(msg, OrderedDict)
> -        print(json.dumps(msg, sort_keys=do_sort,
> -                         indent=indent, separators=separators))
> +        print(json.dumps(msg, sort_keys=do_sort, indent=indent))

Unrelated change. Maybe worth a separate patch?

>       else:
>           print(msg)
>   
> @@ -529,11 +536,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))

Can this work?

                     command_line='qemu-io %s "remove_break bp_%s"'
                                  % (drive, drive))

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   
>       def hmp_qemu_io(self, drive, cmd):
>           '''Write to a given drive using an HMP command'''
> @@ -790,16 +799,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):
> @@ -850,10 +861,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', {})
> @@ -877,8 +891,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):
> @@ -1017,8 +1031,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,
> @@ -1056,8 +1073,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))
> +                test_case.case_skip(f'{test_case}: formats {usf_list} '
> +                                    'are not whitelisted')
>                   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
>