[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v10 10/14] iotests: add hmp helper with logging
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v10 10/14] iotests: add hmp helper with logging |
Date: |
Tue, 31 Mar 2020 19:39:45 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
Am 31.03.2020 um 19:23 hat John Snow geschrieben:
>
>
> On 3/31/20 6:21 AM, Max Reitz wrote:
> > On 31.03.20 02:00, John Snow wrote:
> >> Minor cleanup for HMP functions; helps with line length and consolidates
> >> HMP helpers through one implementation function.
> >>
> >> Although we are adding a universal toggle to turn QMP logging on or off,
> >> many existing callers to hmp functions don't expect that output to be
> >> logged, which causes quite a few changes in the test output.
> >>
> >> For now, offer a use_log parameter.
> >>
> >>
> >> Typing notes:
> >>
> >> QMPResponse is just an alias for Dict[str, Any]. It holds no special
> >> meanings and it is not a formal subtype of Dict[str, Any]. It is best
> >> thought of as a lexical synonym.
> >>
> >> We may well wish to add stricter subtypes in the future for certain
> >> shapes of data that are not formalized as Python objects, at which point
> >> we can simply retire the alias and allow mypy to more strictly check
> >> usages of the name.
> >>
> >> Signed-off-by: John Snow <address@hidden>
> >> ---
> >> tests/qemu-iotests/iotests.py | 35 ++++++++++++++++++++++-------------
> >> 1 file changed, 22 insertions(+), 13 deletions(-)
> >
> > Reviewed-by: Max Reitz <address@hidden>
> >
> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >> index b08bcb87e1..dfc753c319 100644
> >> --- a/tests/qemu-iotests/iotests.py
> >> +++ b/tests/qemu-iotests/iotests.py
> >> @@ -37,6 +37,10 @@
> >>
> >> assert sys.version_info >= (3, 6)
> >>
> >> +# Type Aliases
> >> +QMPResponse = Dict[str, Any]
> >> +
> >> +
> >> faulthandler.enable()
> >>
> >> # This will not work if arguments contain spaces but is necessary if we
> >> @@ -540,25 +544,30 @@ def add_incoming(self, addr):
> >> self._args.append(addr)
> >> return self
> >>
> >> - def pause_drive(self, drive, event=None):
> >> - '''Pause drive r/w operations'''
> >> + def hmp(self, command_line: str, use_log: bool = False) ->
> >> QMPResponse:
> >> + cmd = 'human-monitor-command'
> >> + kwargs = {'command-line': command_line}
> >> + if use_log:
> >> + return self.qmp_log(cmd, **kwargs)
> >> + else:
> >> + return self.qmp(cmd, **kwargs)
> >
> > Hm. I suppose I should take this chance to understand something about
> > mypy. QEMUMachine.qmp() isn’t typed, so mypy can’t check that this
> > really returns QMPResponse. Is there some flag to make it? Like
> > --actually-check-types?
> >
>
> One of --strict's implied options, I'm not sure which. Otherwise, mypy
> is geared towards a 'gradual typing' discipline.
>
> In truth, I'm a little thankful for that because it helps avoid yak
> shaving marathons.
>
> It does mean that sometimes the annotations don't "do anything" yet,
> apart from offering hints and documentation in e.g. pycharm. Which does
> mean that sometimes they can be completely wrong...
>
> The more we add, the more we'll catch problems.
>
> Once this series is dusted I'll try to tackle more conversions for
> iotests, qmp, etc. I've got a few WIP patches to tackle conversions for
> tests/qemu-iotests/*.py but I am trying to shepherd this one in first
> before I go bananas.
>
> > (--strict seems, well, overly strict? Like not allowing generics, I
> > don’t see why. Or I suppose for the time being we want to allow untyped
> > definitions, as long as they don’t break type assertions such as it kind
> > of does here...?)
> >
>
> "disallow-any-generics" means disallowing `Any` generics, not
> disallowing generics ... in general. (I think? I've been using mypy in
> strict mode for a personal project a lot lately and I use generics in a
> few places, it seems OK.)
--disallow-any-generics
disallow usage of generic types that do not specify explicit type
parameters
So it will complain if you say just List, and you need to be explicit if
you really want List[Any]. Which I think is a reasonable thing to
require.
Kevin
- [PATCH v10 01/14] iotests: do a light delinting, (continued)
- [PATCH v10 01/14] iotests: do a light delinting, John Snow, 2020/03/30
- [PATCH v10 03/14] iotests: ignore import warnings from pylint, John Snow, 2020/03/30
- [PATCH v10 04/14] iotests: replace mutable list default args, John Snow, 2020/03/30
- [PATCH v10 07/14] iotests: drop pre-Python 3.4 compatibility code, John Snow, 2020/03/30
- [PATCH v10 05/14] iotests: add pylintrc file, John Snow, 2020/03/30
- [PATCH v10 08/14] iotests: touch up log function signature, John Snow, 2020/03/30
- [PATCH v10 10/14] iotests: add hmp helper with logging, John Snow, 2020/03/30
- [PATCH v10 06/14] iotests: alphabetize standard imports, John Snow, 2020/03/30
- [PATCH v10 09/14] iotests: limit line length to 79 chars, John Snow, 2020/03/30
- [PATCH v10 12/14] iotest 258: use script_main, John Snow, 2020/03/30
- [PATCH v10 13/14] iotests: Mark verify functions as private, John Snow, 2020/03/30
- [PATCH v10 11/14] iotests: add script_initialize, John Snow, 2020/03/30
- [PATCH v10 14/14] iotests: use python logging for iotests.log(), John Snow, 2020/03/30
- Re: [PATCH v10 00/14] iotests: use python logging, Kevin Wolf, 2020/03/31