[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v10 10/14] iotests: add hmp helper with logging
From: |
Max Reitz |
Subject: |
Re: [PATCH v10 10/14] iotests: add hmp helper with logging |
Date: |
Wed, 1 Apr 2020 14:42:05 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 |
On 01.04.20 14:28, Max Reitz wrote:
> 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.
>
>>> (--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...
>
>> iotests.py:560: warning: Returning Any from function declared to return
>> "Dict[str, Any]"
>> iotests.py:560: error: Call to untyped function "qmp_log" in typed context
>> iotests.py:562: warning: Returning Any from function declared to return
>> "Dict[str, Any]"
>>
>> Not sure why you think it doesn't allow generics? I never had problems
>> with that, even in my Python museum. :-)
>
> I thought --disallow-any-generics would mean that. But I suppose mypy
> understands a “generic” to be something else than I do, as John
> described... *shrug*
Oh. John didn’t describe that. I just read the “Any” thing wrong,
again. (On my first read, I thought he just used the back ticks to
stress the word “any”, not to refer to the type “Any”.)
Max
signature.asc
Description: OpenPGP digital signature