[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/5] python/utils: add VerboseProcessError
From: |
John Snow |
Subject: |
Re: [PATCH v3 2/5] python/utils: add VerboseProcessError |
Date: |
Thu, 17 Mar 2022 12:31:52 -0400 |
On Thu, Mar 17, 2022 at 11:56 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
> On 17.03.22 16:13, John Snow wrote:
> > On Thu, Mar 17, 2022 at 5:23 AM Hanna Reitz <hreitz@redhat.com> wrote:
> >> On 08.03.22 02:57, John Snow wrote:
> >>> This adds an Exception that extends the Python stdlib
> >>> subprocess.CalledProcessError.
> >>>
> >>> The difference is that the str() method of this exception also adds the
> >>> stdout/stderr logs. In effect, if this exception goes unhandled, Python
> >>> will print the output in a visually distinct wrapper to the terminal so
> >>> that it's easy to spot in a sea of traceback information.
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>> ---
> >>> python/qemu/utils/__init__.py | 36 +++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 36 insertions(+)
> >>>
> >>> diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
> >>> index 5babf40df2..355ac550bc 100644
> >>> --- a/python/qemu/utils/__init__.py
> >>> +++ b/python/qemu/utils/__init__.py
> >>> @@ -18,6 +18,7 @@
> >>> import os
> >>> import re
> >>> import shutil
> >>> +from subprocess import CalledProcessError
> >>> import textwrap
> >>> from typing import Optional
> >>>
> >>> @@ -26,6 +27,7 @@
> >>>
> >>>
> >>> __all__ = (
> >>> + 'VerboseProcessError',
> >>> 'add_visual_margin',
> >>> 'get_info_usernet_hostfwd_port',
> >>> 'kvm_available',
> >>> @@ -121,3 +123,37 @@ def _wrap(line: str) -> str:
> >>> os.linesep.join(_wrap(line) for line in content.splitlines()),
> >>> _bar(None, top=False),
> >>> ))
> >>> +
> >>> +
> >>> +class VerboseProcessError(CalledProcessError):
> >>> + """
> >>> + The same as CalledProcessError, but more verbose.
> >>> +
> >>> + This is useful for debugging failed calls during test executions.
> >>> + The return code, signal (if any), and terminal output will be
> >>> displayed
> >>> + on unhandled exceptions.
> >>> + """
> >>> + def summary(self) -> str:
> >>> + """Return the normal CalledProcessError str() output."""
> >>> + return super().__str__()
> >>> +
> >>> + def __str__(self) -> str:
> >>> + lmargin = ' '
> >>> + width = -len(lmargin)
> >>> + sections = []
> >>> +
> >>> + name = 'output' if self.stderr is None else 'stdout'
> >>> + if self.stdout:
> >>> + sections.append(add_visual_margin(self.stdout, width, name))
> >>> + else:
> >>> + sections.append(f"{name}: N/A")
> >>> +
> >>> + if self.stderr:
> >>> + sections.append(add_visual_margin(self.stderr, width,
> >>> 'stderr'))
> >>> + elif self.stderr is not None:
> >> What exactly is this condition for? I would’ve understood if it was
> >> `self.stdout` (because the stdout section then is called just “output”,
> >> so it would make sense to omit the stderr block), but this way it looks
> >> like we’ll only go here if `self.stderr` is an empty string (which
> >> doesn’t make much sense to me, and I don’t understand why we wouldn’t
> >> have the same in the `self.stdout` part above).
> >>
> >> (tl;dr: should this be `self.stdout`?)
> >>
> >> Hanna
> >>
> > if self.stderr is None, it means that the IO streams were combined. If
> > it is merely empty, it means there wasn't any stderr output.
>
> Might warrant a comment? :)
How 'bout:
has_combined_output = self.stderr is None
>
> > so:
> >
> > if self.stderr: There's content here, so put it in a lil' box
> > else: could be either None or the empty string. If it's None, we
> > didn't *have* a stderr, so don't print anything at all, let the
> > "output" section above handle it. If we did have stderr and it just
> > happened to be empty, write N/A.
> >
> > I wanted that "N/A" to provide active feedback to show the human
> > operator that we're not just failing to show them what the stderr
> > output was: there genuinely wasn't any.
>
> Thanks, that makes sense.
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
- [PATCH v3 1/5] python/utils: add add_visual_margin() text decoration utility, (continued)
[PATCH v3 4/5] iotests: make qemu_img raise on non-zero rc by default, John Snow, 2022/03/07
Re: [PATCH v3 4/5] iotests: make qemu_img raise on non-zero rc by default, Hanna Reitz, 2022/03/17
[PATCH v3 5/5] iotests: fortify compare_images() against crashes, John Snow, 2022/03/07