Re: [Qemu-block] [PATCH 3/4] qemu-iotests: Make block job methods common

[Fam Zheng]

On Tue, 05/05 18:17, John Snow wrote:
> 
> 
> On 05/05/2015 08:46 AM, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <address@hidden>
> > ---
> >  tests/qemu-iotests/041        | 66 
> > ++++++++++---------------------------------
> >  tests/qemu-iotests/iotests.py | 28 ++++++++++++++++++
> >  2 files changed, 43 insertions(+), 51 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> > index 59a8f73..3d46ed7 100755
> > --- a/tests/qemu-iotests/041
> > +++ b/tests/qemu-iotests/041
> > @@ -34,38 +34,8 @@ quorum_img3 = os.path.join(iotests.test_dir, 
> > 'quorum3.img')
> >  quorum_repair_img = os.path.join(iotests.test_dir, 'quorum_repair.img')
> >  quorum_snapshot_file = os.path.join(iotests.test_dir, 
> > 'quorum_snapshot.img')
> >  
> > -class ImageMirroringTestCase(iotests.QMPTestCase):
> > -    '''Abstract base class for image mirroring test cases'''
> >  
> > -    def wait_ready(self, drive='drive0'):
> > -        '''Wait until a block job BLOCK_JOB_READY event'''
> > -        ready = False
> > -        while not ready:
> > -            for event in self.vm.get_qmp_events(wait=True):
> > -                if event['event'] == 'BLOCK_JOB_READY':
> > -                    self.assert_qmp(event, 'data/type', 'mirror')
> > -                    self.assert_qmp(event, 'data/device', drive)
> > -                    ready = True
> > -
> > -    def wait_ready_and_cancel(self, drive='drive0'):
> > -        self.wait_ready(drive=drive)
> > -        event = self.cancel_and_wait(drive=drive)
> > -        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> > -        self.assert_qmp(event, 'data/type', 'mirror')
> > -        self.assert_qmp(event, 'data/offset', event['data']['len'])
> > -
> > -    def complete_and_wait(self, drive='drive0', wait_ready=True):
> > -        '''Complete a block job and wait for it to finish'''
> > -        if wait_ready:
> > -            self.wait_ready(drive=drive)
> > -
> > -        result = self.vm.qmp('block-job-complete', device=drive)
> > -        self.assert_qmp(result, 'return', {})
> > -
> > -        event = self.wait_until_completed(drive=drive)
> > -        self.assert_qmp(event, 'data/type', 'mirror')
> > -
> > -class TestSingleDrive(ImageMirroringTestCase):
> > +class TestSingleDrive(iotests.QMPTestCase):
> >      image_len = 1 * 1024 * 1024 # MB
> >  
> >      def setUp(self):
> > @@ -221,17 +191,9 @@ class TestSingleDriveUnalignedLength(TestSingleDrive):
> >      test_small_buffer2 = None
> >      test_large_cluster = None
> >  
> > -class TestMirrorNoBacking(ImageMirroringTestCase):
> > +class TestMirrorNoBacking(iotests.QMPTestCase):
> >      image_len = 2 * 1024 * 1024 # MB
> >  
> > -    def complete_and_wait(self, drive='drive0', wait_ready=True):
> > -        iotests.create_image(target_backing_img, 
> > TestMirrorNoBacking.image_len)
> > -        return ImageMirroringTestCase.complete_and_wait(self, drive, 
> > wait_ready)
> > -
> > -    def compare_images(self, img1, img2):
> > -        iotests.create_image(target_backing_img, 
> > TestMirrorNoBacking.image_len)
> > -        return iotests.compare_images(img1, img2)
> > -
> >      def setUp(self):
> >          iotests.create_image(backing_img, TestMirrorNoBacking.image_len)
> >          qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
> > backing_img, test_img)
> > @@ -242,7 +204,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
> >          self.vm.shutdown()
> >          os.remove(test_img)
> >          os.remove(backing_img)
> > -        os.remove(target_backing_img)
> > +        try:
> > +            os.remove(target_backing_img)
> > +        except:
> > +            pass
> >          os.remove(target_img)
> >  
> >      def test_complete(self):
> > @@ -257,7 +222,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
> >          result = self.vm.qmp('query-block')
> >          self.assert_qmp(result, 'return[0]/inserted/file', target_img)
> >          self.vm.shutdown()
> > -        self.assertTrue(self.compare_images(test_img, target_img),
> > +        self.assertTrue(iotests.compare_images(test_img, target_img),
> >                          'target image does not match source after 
> > mirroring')
> >  
> >      def test_cancel(self):
> > @@ -272,7 +237,7 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
> >          result = self.vm.qmp('query-block')
> >          self.assert_qmp(result, 'return[0]/inserted/file', test_img)
> >          self.vm.shutdown()
> > -        self.assertTrue(self.compare_images(test_img, target_img),
> > +        self.assertTrue(iotests.compare_images(test_img, target_img),
> >                          'target image does not match source after 
> > mirroring')
> >  
> >      def test_large_cluster(self):
> > @@ -283,7 +248,6 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
> >                          %(TestMirrorNoBacking.image_len), 
> > target_backing_img)
> >          qemu_img('create', '-f', iotests.imgfmt, '-o', 
> > 'cluster_size=%d,backing_file=%s'
> >                          % (TestMirrorNoBacking.image_len, 
> > target_backing_img), target_img)
> > -        os.remove(target_backing_img)
> >  
> >          result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
> >                               mode='existing', target=target_img)
> > @@ -293,10 +257,10 @@ class TestMirrorNoBacking(ImageMirroringTestCase):
> >          result = self.vm.qmp('query-block')
> >          self.assert_qmp(result, 'return[0]/inserted/file', target_img)
> >          self.vm.shutdown()
> > -        self.assertTrue(self.compare_images(test_img, target_img),
> > +        self.assertTrue(iotests.compare_images(test_img, target_img),
> >                          'target image does not match source after 
> > mirroring')
> >  
> > -class TestMirrorResized(ImageMirroringTestCase):
> > +class TestMirrorResized(iotests.QMPTestCase):
> >      backing_len = 1 * 1024 * 1024 # MB
> >      image_len = 2 * 1024 * 1024 # MB
> >  
> > @@ -344,7 +308,7 @@ class TestMirrorResized(ImageMirroringTestCase):
> >          self.assertTrue(iotests.compare_images(test_img, target_img),
> >                          'target image does not match source after 
> > mirroring')
> >  
> > -class TestReadErrors(ImageMirroringTestCase):
> > +class TestReadErrors(iotests.QMPTestCase):
> >      image_len = 2 * 1024 * 1024 # MB
> >  
> >      # this should be a multiple of twice the default granularity
> > @@ -498,7 +462,7 @@ new_state = "1"
> >          self.assert_no_active_block_jobs()
> >          self.vm.shutdown()
> >  
> > -class TestWriteErrors(ImageMirroringTestCase):
> > +class TestWriteErrors(iotests.QMPTestCase):
> >      image_len = 2 * 1024 * 1024 # MB
> >  
> >      # this should be a multiple of twice the default granularity
> > @@ -624,7 +588,7 @@ new_state = "1"
> >          self.assert_no_active_block_jobs()
> >          self.vm.shutdown()
> >  
> > -class TestSetSpeed(ImageMirroringTestCase):
> > +class TestSetSpeed(iotests.QMPTestCase):
> >      image_len = 80 * 1024 * 1024 # MB
> >  
> >      def setUp(self):
> > @@ -690,7 +654,7 @@ class TestSetSpeed(ImageMirroringTestCase):
> >  
> >          self.wait_ready_and_cancel()
> >  
> > -class TestUnbackedSource(ImageMirroringTestCase):
> > +class TestUnbackedSource(iotests.QMPTestCase):
> >      image_len = 2 * 1024 * 1024 # MB
> >  
> >      def setUp(self):
> > @@ -731,7 +695,7 @@ class TestUnbackedSource(ImageMirroringTestCase):
> >          self.complete_and_wait()
> >          self.assert_no_active_block_jobs()
> >  
> > -class TestRepairQuorum(ImageMirroringTestCase):
> > +class TestRepairQuorum(iotests.QMPTestCase):
> >      """ This class test quorum file repair using drive-mirror.
> >          It's mostly a fork of TestSingleDrive """
> >      image_len = 1 * 1024 * 1024 # MB
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index e93e623..2e07cc4 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -326,6 +326,34 @@ class QMPTestCase(unittest.TestCase):
> >          self.assert_no_active_block_jobs()
> >          return event
> >  
> > +    def wait_ready(self, drive='drive0'):
> > +        '''Wait until a block job BLOCK_JOB_READY event'''
> > +        ready = False
> > +        while not ready:
> > +            for event in self.vm.get_qmp_events(wait=True):
> > +                if event['event'] == 'BLOCK_JOB_READY':
> > +                    self.assert_qmp(event, 'data/type', 'mirror')
> > +                    self.assert_qmp(event, 'data/device', drive)
> > +                    ready = True
> > +
> 
> I know you only moved the code, but since it's going into the general
> purpose iotests instead of inside of a specific test case...
> 
> get_qmp_events will completely empty the queue, but we're only
> interested in one specific event. This may make writing tests harder
> depending on what order events arrive in.
> 
> You may want to rely upon self.event_wait() instead, which allows you to
> specify an event to match, and any events it finds that don't match this
> are left alone to be pulled by future calls.
> 
> Something like this: (not tested...)
> 
> def wait_ready(self, drive='drive0'):
>   filter = {'data': {'type': 'mirror', 'device': drive } }
>   event = self.event_wait(BLOCK_JOB_READY, match=filter)
> 
> This way we can use wait_ready to test in asynchronous cases with other
> stuff flying around.
> 
> (Hmm, looks like I never converted the existing users of get_qmp_events,
> which can/would cause similar difficulties...)

Indeed it's nicer. I'll add a patch to improve that (not to mess up this code
moving patch).

Fam

> 
> > +    def wait_ready_and_cancel(self, drive='drive0'):
> > +        self.wait_ready(drive=drive)
> > +        event = self.cancel_and_wait(drive=drive)
> > +        self.assertEquals(event['event'], 'BLOCK_JOB_COMPLETED')
> > +        self.assert_qmp(event, 'data/type', 'mirror')
> > +        self.assert_qmp(event, 'data/offset', event['data']['len'])
> > +
> > +    def complete_and_wait(self, drive='drive0', wait_ready=True):
> > +        '''Complete a block job and wait for it to finish'''
> > +        if wait_ready:
> > +            self.wait_ready(drive=drive)
> > +
> > +        result = self.vm.qmp('block-job-complete', device=drive)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        event = self.wait_until_completed(drive=drive)
> > +        self.assert_qmp(event, 'data/type', 'mirror')
> > +
> >  def notrun(reason):
> >      '''Skip this test suite'''
> >      # Each test in qemu-iotests has a number ("seq")
> >