| Message ID | 20180525163327.23097-7-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Make blockdev-create a job and stable API | expand |
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 <kwolf@redhat.com> > --- > 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. > + > 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). Max > + return result > + > > index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') > >
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 <kwolf@redhat.com> > > --- > > 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... > > + > > 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. Kevin
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 <kwolf@redhat.com> >>> --- >>> 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.) >>> + >>> 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)))"? Max
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 <kwolf@redhat.com> > >>> --- > >>> 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. > >>> + > >>> 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. Kevin
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 <kwolf@redhat.com> >>>>> --- >>>>> 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
On Fri, May 25, 2018 at 06:33:19PM +0200, 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 <kwolf@redhat.com> > --- > 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/') > + Either as-is, or with the suggestion by Max to add the PID to the output: Reviewed-by: Jeff Cody <jcody@redhat.com> > 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) > + return result > + > > index_re = re.compile(r'([^\[]+)\[([^\]]+)\]') > > -- > 2.13.6 > >
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/') + 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) + return result + index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
This adds a helper function that logs both the QMP request and the received response before returning it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- tests/qemu-iotests/iotests.py | 11 +++++++++++ 1 file changed, 11 insertions(+)