| Message ID | 20181213015013.15350-7-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: remove x- prefix from QMP api | expand |
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>
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): >
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.
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): >> > >
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 --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):
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(-)