Re: [Qemu-devel] [PATCH 06/14] qemu-iotests: Add VM.qmp_log()

[Max Reitz]

On 2018-05-29 14:39, Kevin Wolf wrote:
> Am 29.05.2018 um 14:15 hat Max Reitz geschrieben:
>> On 2018-05-29 14:12, Kevin Wolf wrote:
>>> Am 29.05.2018 um 13:48 hat Max Reitz geschrieben:
>>>> On 2018-05-25 18:33, Kevin Wolf wrote:
>>>>> This adds a helper function that logs both the QMP request and the
>>>>> received response before returning it.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <address@hidden>
>>>>> ---
>>>>>  tests/qemu-iotests/iotests.py | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>>> index 17aa7c88dc..319d898172 100644
>>>>> --- a/tests/qemu-iotests/iotests.py
>>>>> +++ b/tests/qemu-iotests/iotests.py
>>>>> @@ -206,6 +206,10 @@ def filter_qmp_event(event):
>>>>>          event['timestamp']['microseconds'] = 'USECS'
>>>>>      return event
>>>>>  
>>>>> +def filter_testfiles(msg):
>>>>> +    prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>>>> +    return msg.replace(prefix, 'TEST_DIR/')
>>>>
>>>> I'd prefer 'TEST_DIR/PID-' (just because).
>>>>
>>>> But if you really like just 'TEST_DIR/'...  Then OK.
>>>
>>> I preferred that because it leaves the output unchanged from the old
>>> bash tests, which made reviewing the results easier. Maybe that's a too
>>> temporary advantage to be of any use in the future, though, so we could
>>> change it afterwards...
>>
>> It doesn't really make reviewing the patches easier, though, because the
>> hardest part of course is the change of the test itself and not the
>> change of the result. :-)
>>
>> (And some file name changes really are on the easy side.)
> 
> If you think so... For me the main reason to convert the test files was
> to see whether the job actually does the same thing as the old
> synchronous command did.

Sure, but kompare is rather good at highlighting changes in a single
line, so I can quickly verify them as benign. :-)

>>>>> +
>>>>>  def log(msg, filters=[]):
>>>>>      for flt in filters:
>>>>>          msg = flt(msg)
>>>>> @@ -389,6 +393,13 @@ class VM(qtest.QEMUQtestMachine):
>>>>>              result.append(filter_qmp_event(ev))
>>>>>          return result
>>>>>  
>>>>> +    def qmp_log(self, cmd, **kwargs):
>>>>> +        logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
>>>>> +        log(filter_testfiles(logmsg))
>>>>> +        result = self.qmp(cmd, **kwargs)
>>>>> +        log(result)
>>>>
>>>> I think we should apply the testfiles filter here, too (error messages
>>>> may contain file names, after all).
>>>
>>> Didn't happen in the test outputs of this series, and filter_testfiles()
>>> processes strings whereas result is a dict, so it would be more
>>> complicated than just adding a function call.
>>
>> You mean it would be "log(filter_testfiles('%s' % result)))"?
> 
> Ah, you mean just for logging? Yes, we can do that. I thought you meant
> returning a filtered result as well.

Oh, no.  At least I can't think of a reason why we'd want that.

Max

signature.asc
Description: OpenPGP digital signature