diff mbox series

[v2,6/7] iotests: allow pretty-print for qmp_log

Message ID 20181213015013.15350-7-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series bitmaps: remove x- prefix from QMP api | expand

Commit Message

John Snow Dec. 13, 2018, 1:50 a.m. UTC
If iotests have lines exceeding >998 characters long, git doesn't
want to send it plaintext to the list. We can solve this by allowing
the iotests to use pretty printed QMP output that we can match against
instead.

As a bonus, it's much nicer for human eyes, too.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Blake Dec. 13, 2018, 2:20 a.m. UTC | #1
On 12/12/18 7:50 PM, John Snow wrote:
> If iotests have lines exceeding >998 characters long, git doesn't
> want to send it plaintext to the list. We can solve this by allowing
> the iotests to use pretty printed QMP output that we can match against
> instead.
> 
> As a bonus, it's much nicer for human eyes, too.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 9595429fea..dbbef4bad3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine):
>               result.append(filter_qmp_event(ev))
>           return result
>   
> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):

Why None instead of False?

>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>               (cmd, json.dumps(kwargs, sort_keys=True))
>           log(logmsg, filters)
>           result = self.qmp(cmd, **kwargs)
> -        log(json.dumps(result, sort_keys=True), filters)
> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)

But I'd actually have to read the contract to json.dumps() to learn what 
is expected.

/me goes and does that...
https://docs.python.org/2/library/json.html

If indent is a non-negative integer, then JSON array elements and object 
members will be pretty-printed with that indent level. An indent level 
of 0, or negative, will only insert newlines. None (the default) selects 
the most compact representation.

Okay, makes sense.
Reviewed-by: Eric Blake <eblake@redhat.com>
Vladimir Sementsov-Ogievskiy Dec. 13, 2018, 1:09 p.m. UTC | #2
13.12.2018 4:50, John Snow wrote:
> If iotests have lines exceeding >998 characters long, git doesn't
> want to send it plaintext to the list. We can solve this by allowing
> the iotests to use pretty printed QMP output that we can match against
> instead.
> 
> As a bonus, it's much nicer for human eyes, too.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 9595429fea..dbbef4bad3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine):
>               result.append(filter_qmp_event(ev))
>           return result
>   
> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>               (cmd, json.dumps(kwargs, sort_keys=True))
>           log(logmsg, filters)

why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it.
(hmm, unfortunately, "execute" < "arguments", and they will be rearranged)

with or without (as the patch don't aim to prettify everything):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>           result = self.qmp(cmd, **kwargs)
> -        log(json.dumps(result, sort_keys=True), filters)
> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
>           return result

hmm, doing the same thing as Eric (check specs), I see the following note:

 > Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.

And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago.
It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least
in common helpers. May be best place to fix is iotests.log() function

>   
>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>
John Snow Dec. 13, 2018, 6:26 p.m. UTC | #3
On 12/12/18 9:20 PM, Eric Blake wrote:
> On 12/12/18 7:50 PM, John Snow wrote:
>> If iotests have lines exceeding >998 characters long, git doesn't
>> want to send it plaintext to the list. We can solve this by allowing
>> the iotests to use pretty printed QMP output that we can match against
>> instead.
>>
>> As a bonus, it's much nicer for human eyes, too.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 9595429fea..dbbef4bad3 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine):
>>               result.append(filter_qmp_event(ev))
>>           return result
>>   -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles],
>> **kwargs):
> 
> Why None instead of False?
> 
>>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>>               (cmd, json.dumps(kwargs, sort_keys=True))
>>           log(logmsg, filters)
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
> 
> But I'd actually have to read the contract to json.dumps() to learn what
> is expected.
> 
> /me goes and does that...
> https://docs.python.org/2/library/json.html
> 
> If indent is a non-negative integer, then JSON array elements and object
> members will be pretty-printed with that indent level. An indent level
> of 0, or negative, will only insert newlines. None (the default) selects
> the most compact representation.
> 
> Okay, makes sense.
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Yeah, it's a bubbling up of API in this case and a little messy, but it
was the quickest route to making the output look pretty.
John Snow Dec. 14, 2018, 8:51 p.m. UTC | #4
On 12/13/18 8:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.12.2018 4:50, John Snow wrote:
>> If iotests have lines exceeding >998 characters long, git doesn't
>> want to send it plaintext to the list. We can solve this by allowing
>> the iotests to use pretty printed QMP output that we can match against
>> instead.
>>
>> As a bonus, it's much nicer for human eyes, too.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 9595429fea..dbbef4bad3 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine):
>>               result.append(filter_qmp_event(ev))
>>           return result
>>   
>> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
>>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>>               (cmd, json.dumps(kwargs, sort_keys=True))
>>           log(logmsg, filters)
> 
> why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it.
> (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
> 

It wasn't long enough to irritate me :)
but we're here, so I'll do that too.

> with or without (as the patch don't aim to prettify everything):
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
>>           return result
> 
> hmm, doing the same thing as Eric (check specs), I see the following note:
> 
>  > Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
> 
> And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago.
> It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least
> in common helpers. May be best place to fix is iotests.log() function
> 

Oh, good spot. I actually did run into this and wasn't aware of what
caused it! I will change the default separator.

>>   
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>
> 
>
Vladimir Sementsov-Ogievskiy Dec. 17, 2018, 9:15 a.m. UTC | #5
14.12.2018 23:51, John Snow wrote:
> 
> 
> On 12/13/18 8:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.12.2018 4:50, John Snow wrote:
>>> If iotests have lines exceeding >998 characters long, git doesn't
>>> want to send it plaintext to the list. We can solve this by allowing
>>> the iotests to use pretty printed QMP output that we can match against
>>> instead.
>>>
>>> As a bonus, it's much nicer for human eyes, too.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    tests/qemu-iotests/iotests.py | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 9595429fea..dbbef4bad3 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine):
>>>                result.append(filter_qmp_event(ev))
>>>            return result
>>>    
>>> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>>> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
>>>            logmsg = '{"execute": "%s", "arguments": %s}' % \
>>>                (cmd, json.dumps(kwargs, sort_keys=True))
>>>            log(logmsg, filters)
>>
>> why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it.
>> (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
>>
> 
> It wasn't long enough to irritate me :)
> but we're here, so I'll do that too.
> 
>> with or without (as the patch don't aim to prettify everything):
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>>            result = self.qmp(cmd, **kwargs)
>>> -        log(json.dumps(result, sort_keys=True), filters)
>>> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
>>>            return result
>>
>> hmm, doing the same thing as Eric (check specs), I see the following note:
>>
>>   > Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
>>
>> And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago.
>> It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least
>> in common helpers. May be best place to fix is iotests.log() function
>>
> 
> Oh, good spot. I actually did run into this and wasn't aware of what
> caused it! I will change the default separator.

In this case output would be less pretty. I'd prefer just remove trailing whitespace as part of filtering process in log().

> 
>>>    
>>>        def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>>
>>
>>
diff mbox series

Patch

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9595429fea..dbbef4bad3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -447,12 +447,12 @@  class VM(qtest.QEMUQtestMachine):
             result.append(filter_qmp_event(ev))
         return result
 
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
+    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
         logmsg = '{"execute": "%s", "arguments": %s}' % \
             (cmd, json.dumps(kwargs, sort_keys=True))
         log(logmsg, filters)
         result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters)
+        log(json.dumps(result, indent=indent, sort_keys=True), filters)
         return result
 
     def run_job(self, job, auto_finalize=True, auto_dismiss=False):