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

[Kevin Wolf]

Am 02.04.2020 um 20:27 hat John Snow geschrieben:
> On 4/1/20 8:40 AM, Max Reitz wrote:
> > On 31.03.20 19:39, Kevin Wolf wrote:
> >> 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.
> > 
> > Sure.  I was just looking into the different options.  I was interested
> > in whether I could come up with a mode that leaves wholly untyped code
> > alone, but warns for code that mixes it.  Or something.
> > 
> >>> 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.
> > 
> > Sure, sure.
> > 
> >>>> (--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.
> > 
> > OK.  So it’s “disallow ‘any’ generics”, not “disallow any ‘generic’s”.
> > Not easy to parse.  (Yes, yes, I should’ve actually read the man page...)
> > 
> > Good to know that mypy and me actually do seem to loosely agree on what
> > a generic is. :)
> > 
> > Max
> > 
> 
> Are we squared up for this series? I am actually not sure.

I had a comment in patch 14 which you may or may not want to address (my
R-b was unconditional). I think everything else was just tangential
discussion.

Kevin