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

[John Snow]

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.)

> Max
>