Re: [PATCH v10 10/14] iotests: add hmp helper with logging

[Kevin Wolf]

Am 01.04.2020 um 14:28 hat Max Reitz geschrieben:
> On 31.03.20 16:00, Kevin Wolf wrote:
> > Am 31.03.2020 um 12:21 hat Max Reitz geschrieben:
> >> 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?
> > 
> > There is --check-untyped-defs, but I'm not sure if that actually changes
> > the return type of untyped functions from Any to an inferred type. I
> > kind of doubt it.
> 
> Well, but Any doesn’t fit into QMPResponse, so there should be an error
> reported somewhere.

Any is the void* of Python typing. It's compatible with everything else,
in both directions.

> >> (--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...?)
> > 
> > At least, --strict does actually complain about this one because Any
> > isn't good enough any more (it includes --warn-return-any):
> 
> Hm, yes, but we’re not at a point where it’s really feasible to enable
> --strict...

We're not yet, but I think it's a reasonable goal.

Kevin

signature.asc
Description: PGP signature