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.

Note that this changes the sort order for "command" and "arguments",
so I restrict this reordering to occur only when the indent is specified.
---
 tests/qemu-iotests/iotests.py | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9595429fea..ab5823c011 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
             result.append(filter_qmp_event(ev))
         return result
 
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
-        logmsg = '{"execute": "%s", "arguments": %s}' % \
-            (cmd, json.dumps(kwargs, sort_keys=True))
+    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
+        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
+        separators = (',', ': ') if indent is not None else (',', ': ')
+        if indent is not None:
+            fullcmd = { "execute": cmd,
+                        "arguments": kwargs }
+            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,
+                                sort_keys=True)
+        else:
+            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, separators=separators,
+                       sort_keys=True), filters)
         return result
 
     def run_job(self, job, auto_finalize=True, auto_dismiss=False):
-- 
2.17.2

15.12.2018 2:15, 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.
> 
> Note that this changes the sort order for "command" and "arguments",
> so I restrict this reordering to occur only when the indent is specified.
> ---
>   tests/qemu-iotests/iotests.py | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 9595429fea..ab5823c011 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
>               result.append(filter_qmp_event(ev))
>           return result
>   
> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
> -            (cmd, json.dumps(kwargs, sort_keys=True))
> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
> +        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
> +        separators = (',', ': ') if indent is not None else (',', ': ')
> +        if indent is not None:
> +            fullcmd = { "execute": cmd,
> +                        "arguments": kwargs }
> +            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,
> +                                sort_keys=True)
> +        else:
> +            logmsg = '{"execute": "%s", "arguments": %s}' % \
> +                (cmd, json.dumps(kwargs, sort_keys=True))

definitely overlogic on None/is not None, but anyway, ',' separator leads to less
beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)"
on final message, here or in log(), or in both.

>           log(logmsg, filters)
>           result = self.qmp(cmd, **kwargs)
> -        log(json.dumps(result, sort_keys=True), filters)
> +        log(json.dumps(result, indent=indent, separators=separators,
> +                       sort_keys=True), filters)
>           return result
>   
>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
> 


-- 
Best regards,
Vladimir

17.12.2018 17:26, Vladimir Sementsov-Ogievskiy wrote:
> 15.12.2018 2:15, 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.
>>
>> Note that this changes the sort order for "command" and "arguments",
>> so I restrict this reordering to occur only when the indent is specified.
>> ---
>>   tests/qemu-iotests/iotests.py | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 9595429fea..ab5823c011 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
>>               result.append(filter_qmp_event(ev))
>>           return result
>> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
>> -            (cmd, json.dumps(kwargs, sort_keys=True))
>> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
>> +        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
>> +        separators = (',', ': ') if indent is not None else (',', ': ')
>> +        if indent is not None:
>> +            fullcmd = { "execute": cmd,
>> +                        "arguments": kwargs }
>> +            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,
>> +                                sort_keys=True)
>> +        else:
>> +            logmsg = '{"execute": "%s", "arguments": %s}' % \
>> +                (cmd, json.dumps(kwargs, sort_keys=True))
> 
> definitely overlogic on None/is not None, but anyway, ',' separator leads to less
> beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)"
> on final message, here or in log(), or in both.

find myself composing counter-proposal patch, will send soon...

> 
>>           log(logmsg, filters)
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(json.dumps(result, indent=indent, separators=separators,
>> +                       sort_keys=True), filters)
>>           return result
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>
> 
> 


-- 
Best regards,
Vladimir

On 12/17/18 11:37 AM, Vladimir Sementsov-Ogievskiy wrote:
> 17.12.2018 17:26, Vladimir Sementsov-Ogievskiy wrote:
>> 15.12.2018 2:15, 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.
>>>
>>> Note that this changes the sort order for "command" and "arguments",
>>> so I restrict this reordering to occur only when the indent is specified.
>>> ---
>>>   tests/qemu-iotests/iotests.py | 17 +++++++++++++----
>>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 9595429fea..ab5823c011 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
>>>               result.append(filter_qmp_event(ev))
>>>           return result
>>> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>>> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
>>> -            (cmd, json.dumps(kwargs, sort_keys=True))
>>> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
>>> +        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
>>> +        separators = (',', ': ') if indent is not None else (',', ': ')
>>> +        if indent is not None:
>>> +            fullcmd = { "execute": cmd,
>>> +                        "arguments": kwargs }
>>> +            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,
>>> +                                sort_keys=True)
>>> +        else:
>>> +            logmsg = '{"execute": "%s", "arguments": %s}' % \
>>> +                (cmd, json.dumps(kwargs, sort_keys=True))
>>
>> definitely overlogic on None/is not None, but anyway, ',' separator leads to less
>> beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)"
>> on final message, here or in log(), or in both.

I can't use "if indent" because 0 is a valid indentation, and I decided
to keep the "correct" order for execute/arguments if no indentation is
requested.

How is ',' less beautiful? When pretty-printing output ',' only occurs
at the end of a line anyway. I don't understand the critique.

> 
> find myself composing counter-proposal patch, will send soon...
> 
>>
>>>           log(logmsg, filters)
>>>           result = self.qmp(cmd, **kwargs)
>>> -        log(json.dumps(result, sort_keys=True), filters)
>>> +        log(json.dumps(result, indent=indent, separators=separators,
>>> +                       sort_keys=True), filters)
>>>           return result
>>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>>
>>
>>
> 
> 

17.12.2018 22:21, John Snow wrote:
> 
> 
> On 12/17/18 11:37 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 17.12.2018 17:26, Vladimir Sementsov-Ogievskiy wrote:
>>> 15.12.2018 2:15, 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.
>>>>
>>>> Note that this changes the sort order for "command" and "arguments",
>>>> so I restrict this reordering to occur only when the indent is specified.
>>>> ---
>>>>    tests/qemu-iotests/iotests.py | 17 +++++++++++++----
>>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>>> index 9595429fea..ab5823c011 100644
>>>> --- a/tests/qemu-iotests/iotests.py
>>>> +++ b/tests/qemu-iotests/iotests.py
>>>> @@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
>>>>                result.append(filter_qmp_event(ev))
>>>>            return result
>>>> -    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>>>> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
>>>> -            (cmd, json.dumps(kwargs, sort_keys=True))
>>>> +    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
>>>> +        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
>>>> +        separators = (',', ': ') if indent is not None else (',', ': ')
>>>> +        if indent is not None:
>>>> +            fullcmd = { "execute": cmd,
>>>> +                        "arguments": kwargs }
>>>> +            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,
>>>> +                                sort_keys=True)
>>>> +        else:
>>>> +            logmsg = '{"execute": "%s", "arguments": %s}' % \
>>>> +                (cmd, json.dumps(kwargs, sort_keys=True))
>>>
>>> definitely overlogic on None/is not None, but anyway, ',' separator leads to less
>>> beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)"
>>> on final message, here or in log(), or in both.
> 
> I can't use "if indent" because 0 is a valid indentation, and I decided
> to keep the "correct" order for execute/arguments if no indentation is
> requested.
> 
> How is ',' less beautiful? When pretty-printing output ',' only occurs
> at the end of a line anyway. I don't understand the critique.

Oops, you are right, I thought about a,b,c,d all this time :(.

Anyway, I'd prefer to make indent=2 by default, like in my sent patch. But now, your
way looks ok for me too, at least as a first step.

so, with fixed s/else (',', ': ')/else (', ', ': ')/:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

however, I'd prefer positive conditions: if indent is None, instead of if indent is not None, as we have
else branch, and positive way is always shorter in this case.


> 
>>
>> find myself composing counter-proposal patch, will send soon...
>>
>>>
>>>>            log(logmsg, filters)
>>>>            result = self.qmp(cmd, **kwargs)
>>>> -        log(json.dumps(result, sort_keys=True), filters)
>>>> +        log(json.dumps(result, indent=indent, separators=separators,
>>>> +                       sort_keys=True), filters)
>>>>            return result
>>>>        def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>>>
>>>
>>>
>>
>>


-- 
Best regards,
Vladimir