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..e2ef3bb812 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):
+            super().on_pause(event)
+            result = self._vm.qmp('block-job-resume', device=self._id)
+            self.test.assert_qmp(result, 'return', {})
+
+        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(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', 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 {
-- 
2.21.1

Am 14.05.2020 um 04:25 hat John Snow geschrieben:
> Instead of having somewhat reproduced it for itself.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

I think you should pass auto_dismiss=True to the JobRunner, or (probably
preferable) change prepare_and_start_job() to start the job with
auto_dismiss=False.

Kevin

On 5/14/20 11:53 AM, Kevin Wolf wrote:
> Am 14.05.2020 um 04:25 hat John Snow geschrieben:
>> Instead of having somewhat reproduced it for itself.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> I think you should pass auto_dismiss=True to the JobRunner, or (probably
> preferable) change prepare_and_start_job() to start the job with
> auto_dismiss=False.
> 
> Kevin
> 

okay, I'll try that out and see if I like it.


Wild tangents, as is my normal:

I also think it would be neat, in some sense, to provide a job creation
abstraction where creating the QMP command in python also creates the
runner with the right parameters based on how you initialized it.

I've not given these even a proper three minutes think, but some
generalized interface for managing the creation of jobs to use in
concert with the job runner would be slick.

(What reminds me of this is needing to remember and understand if I
started something with auto_dismiss or not, which jobs it defaults to
which for, etc. Streamlining the creation and runner could be slick for
faster test-writing in normative cases.)

Am 14.05.2020 um 21:37 hat John Snow geschrieben:
> 
> 
> On 5/14/20 11:53 AM, Kevin Wolf wrote:
> > Am 14.05.2020 um 04:25 hat John Snow geschrieben:
> >> Instead of having somewhat reproduced it for itself.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> > 
> > I think you should pass auto_dismiss=True to the JobRunner, or (probably
> > preferable) change prepare_and_start_job() to start the job with
> > auto_dismiss=False.
> > 
> > Kevin
> > 
> 
> okay, I'll try that out and see if I like it.
> 
> 
> Wild tangents, as is my normal:
> 
> I also think it would be neat, in some sense, to provide a job creation
> abstraction where creating the QMP command in python also creates the
> runner with the right parameters based on how you initialized it.
> 
> I've not given these even a proper three minutes think, but some
> generalized interface for managing the creation of jobs to use in
> concert with the job runner would be slick.

JobRunner could have a static method that starts a job (it would take
the same options as qmp() and forward everything to qmp(), except that
it parses auto_* first) and returns a JobRunner object that you can run
later.

> (What reminds me of this is needing to remember and understand if I
> started something with auto_dismiss or not, which jobs it defaults to
> which for, etc. Streamlining the creation and runner could be slick for
> faster test-writing in normative cases.)

Yes.

In general, I think we should keep tests simple (even if that means some
duplication) and avoid overengineering testing infrastructure because we
could well sink more time there than we'll ever get back, but I guess
small and simple wrappers like in this case can't hurt.

Kevin