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

[Kevin Wolf]

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