Message ID | 20200226004425.1303-3-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
Series | iotests: complicate run_job with this one weird trick? | expand |
On 26.02.20 01:44, John Snow wrote: > Instead of having somewhat reproduced it for itself. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/040 | 51 +++++++++++++++++++++--------------------- > 1 file changed, 25 insertions(+), 26 deletions(-) > > diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 > index 90b59081ff..579dafc797 100755 > --- a/tests/qemu-iotests/040 > +++ b/tests/qemu-iotests/040 > @@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase): > file=('top-dbg' if top_debug else 'top-file'), > backing='mid-fmt') > > + > + class TestJobRunner(iotests.JobRunner): > + expected_events = ('BLOCK_JOB_COMPLETED', > + 'BLOCK_JOB_ERROR', > + 'BLOCK_JOB_READY') > + > + def __init__(self, *args, test, **kwargs): > + super().__init__(*args, **kwargs) > + self.log = [] > + self.test = test > + > + def on_pause(self, event): > + result = self._vm.qmp('block-job-resume', device=self._id) > + self.test.assert_qmp(result, 'return', {}) > + super().on_pause(event) Not that it functionally matters, but I suppose I’d call super().on_pause() before resuming (because the job isn’t exactly paused afterwards). > + > + def on_block_job_event(self, event): > + if event['event'] not in self.expected_events: > + self.test.fail("Unexpected event: %s" % event) > + super().on_block_job_event(event) > + self.log.append(iotests.filter_qmp_event(event)) Hasn’t the event been through filter_qmp_event() already? Max > + > def run_job(self, expected_events, error_pauses_job=False): > - match_device = {'data': {'device': 'job0'}} > - events = { > - 'BLOCK_JOB_COMPLETED': match_device, > - 'BLOCK_JOB_CANCELLED': match_device, > - 'BLOCK_JOB_ERROR': match_device, > - 'BLOCK_JOB_READY': match_device, > - } > - > - completed = False > - log = [] > - while not completed: > - ev = self.vm.events_wait(events, timeout=5.0) > - if ev['event'] == 'BLOCK_JOB_COMPLETED': > - completed = True > - elif ev['event'] == 'BLOCK_JOB_ERROR': > - if error_pauses_job: > - result = self.vm.qmp('block-job-resume', device='job0') > - self.assert_qmp(result, 'return', {}) > - elif ev['event'] == 'BLOCK_JOB_READY': > - result = self.vm.qmp('block-job-complete', device='job0') > - self.assert_qmp(result, 'return', {}) > - else: > - self.fail("Unexpected event: %s" % ev) > - log.append(iotests.filter_qmp_event(ev)) > - > + job = self.TestJobRunner(self.vm, 'job0', use_log=False, test=self) > + job.run() > self.maxDiff = None > - self.assertEqual(expected_events, log) > + self.assertEqual(expected_events, job.log) > > def event_error(self, op, action): > return { >
On 2/26/20 6:31 AM, Max Reitz wrote: > On 26.02.20 01:44, John Snow wrote: >> Instead of having somewhat reproduced it for itself. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> tests/qemu-iotests/040 | 51 +++++++++++++++++++++--------------------- >> 1 file changed, 25 insertions(+), 26 deletions(-) >> >> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 >> index 90b59081ff..579dafc797 100755 >> --- a/tests/qemu-iotests/040 >> +++ b/tests/qemu-iotests/040 >> @@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase): >> file=('top-dbg' if top_debug else 'top-file'), >> backing='mid-fmt') >> >> + >> + class TestJobRunner(iotests.JobRunner): >> + expected_events = ('BLOCK_JOB_COMPLETED', >> + 'BLOCK_JOB_ERROR', >> + 'BLOCK_JOB_READY') >> + >> + def __init__(self, *args, test, **kwargs): >> + super().__init__(*args, **kwargs) >> + self.log = [] >> + self.test = test >> + >> + def on_pause(self, event): >> + result = self._vm.qmp('block-job-resume', device=self._id) >> + self.test.assert_qmp(result, 'return', {}) >> + super().on_pause(event) > > Not that it functionally matters, but I suppose I’d call > super().on_pause() before resuming (because the job isn’t exactly paused > afterwards). > Reasonable detail to consider. It's also likely valid to just *omit* calling the base class pause event when overriding behavior -- If we decide to send resume commands in the future, we'll want to avoid sending conflicting/duplicate events. In this case, the base event is just a NOP so it doesn't matter, but it probably is good hygiene to avoid changing the state *and then* calling the base class. So I think this is a valid observation that should be worked into the docstring for the JobRunner class on how best to make use of it. >> + >> + def on_block_job_event(self, event): >> + if event['event'] not in self.expected_events: >> + self.test.fail("Unexpected event: %s" % event) >> + super().on_block_job_event(event) >> + self.log.append(iotests.filter_qmp_event(event)) > > Hasn’t the event been through filter_qmp_event() already? > Oh, yeah. When I converted 040 here I just kind of shoehorned it onto the new API in a somewhat mechanical fashion, but you're right.
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index 90b59081ff..579dafc797 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase): file=('top-dbg' if top_debug else 'top-file'), backing='mid-fmt') + + class TestJobRunner(iotests.JobRunner): + expected_events = ('BLOCK_JOB_COMPLETED', + 'BLOCK_JOB_ERROR', + 'BLOCK_JOB_READY') + + def __init__(self, *args, test, **kwargs): + super().__init__(*args, **kwargs) + self.log = [] + self.test = test + + def on_pause(self, event): + result = self._vm.qmp('block-job-resume', device=self._id) + self.test.assert_qmp(result, 'return', {}) + super().on_pause(event) + + def on_block_job_event(self, event): + if event['event'] not in self.expected_events: + self.test.fail("Unexpected event: %s" % event) + super().on_block_job_event(event) + self.log.append(iotests.filter_qmp_event(event)) + def run_job(self, expected_events, error_pauses_job=False): - match_device = {'data': {'device': 'job0'}} - events = { - 'BLOCK_JOB_COMPLETED': match_device, - 'BLOCK_JOB_CANCELLED': match_device, - 'BLOCK_JOB_ERROR': match_device, - 'BLOCK_JOB_READY': match_device, - } - - completed = False - log = [] - while not completed: - ev = self.vm.events_wait(events, timeout=5.0) - if ev['event'] == 'BLOCK_JOB_COMPLETED': - completed = True - elif ev['event'] == 'BLOCK_JOB_ERROR': - if error_pauses_job: - result = self.vm.qmp('block-job-resume', device='job0') - self.assert_qmp(result, 'return', {}) - elif ev['event'] == 'BLOCK_JOB_READY': - result = self.vm.qmp('block-job-complete', device='job0') - self.assert_qmp(result, 'return', {}) - else: - self.fail("Unexpected event: %s" % ev) - log.append(iotests.filter_qmp_event(ev)) - + job = self.TestJobRunner(self.vm, 'job0', use_log=False, test=self) + job.run() self.maxDiff = None - self.assertEqual(expected_events, log) + self.assertEqual(expected_events, job.log) def event_error(self, op, action): return {
Instead of having somewhat reproduced it for itself. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qemu-iotests/040 | 51 +++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 26 deletions(-)