Re: [Qemu-devel] [PATCH v3 4/5] qemu-iotests: make python tests attempt to leave intermediate files

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5466 bytes --]

On 08/30/2017 11:52 AM, Jeff Cody wrote:
> Now that 'check' will clean up after tests, try and make python
> tests leave intermediate files so that they might be inspectable
> on failure.
> 
> This isn't perfect; the python unittest framework runs multiple
> tests, even if previous tests failed.  So we need to make sure that
> each test still begins with a "clean" slate, to prevent false
> positives or tainted test runs.
> 
> Rather than delete images in the unittest tearDown, invert this
> and delete images to be used in that test at the beginning of the
> setUp.  This is to make sure that the test run is not inadvertently
> using file droppings from previous runs.  We must use 'blind_remove'
> then for these, as the files might not exist yet, but we don't want
> to throw an error for that.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---

> +++ b/tests/qemu-iotests/030
> @@ -21,7 +21,7 @@
>  import time
>  import os
>  import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_io, blind_remove
>  
>  backing_img = os.path.join(iotests.test_dir, 'backing.img')
>  mid_img = os.path.join(iotests.test_dir, 'mid.img')
> @@ -31,6 +31,9 @@ class TestSingleDrive(iotests.QMPTestCase):
>      image_len = 1 * 1024 * 1024 # MB
>  
>      def setUp(self):
> +        blind_remove(test_img)
> +        blind_remove(mid_img)
> +        blind_remove(backing_img)

Would it be any more pythonic to have support for:

blind_remove(test_img, mid_img, backing_img)

built into the previous patch?

>      def tearDown(self):
>          self.vm.shutdown()
> -        os.remove(self.test_img)
> -        os.remove(self.mid_img_abs)
> -        os.remove(self.backing_img_abs)
> -        try:
> -            os.rmdir(os.path.join(iotests.test_dir, self.dir1))
> -            os.rmdir(os.path.join(iotests.test_dir, self.dir3))
> -            os.rmdir(os.path.join(iotests.test_dir, self.dir2))
> -        except OSError as exception:
> -            if exception.errno != errno.EEXIST and exception.errno != errno.ENOTEMPTY:
> -                raise

The code removed here is using a syntax that differs from what you used
in 3/5 when defining blind_remove; does that matter for 3/5?

> +++ b/tests/qemu-iotests/041

> +        blind_remove(target_img)
>          iotests.create_image(backing_img, self.image_len)
>          qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
>          self.vm = iotests.VM().add_drive(test_img, "node-name=top,backing.node-name=base")
> @@ -49,12 +52,6 @@ class TestSingleDrive(iotests.QMPTestCase):
>  
>      def tearDown(self):
>          self.vm.shutdown()
> -        os.remove(test_img)
> -        os.remove(backing_img)
> -        try:
> -            os.remove(target_img)
> -        except OSError:
> -            pass

You're changing failures other than ENOENT from ignored to explicit -
nice little bug-fix along the way :)  I notice this pattern in multiple
tests; is it worth mentioning in the commit message as intentional?

> @@ -797,6 +788,9 @@ class TestRepairQuorum(iotests.QMPTestCase):
>      IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
>  
>      def setUp(self):
> +        for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
> +            blind_remove(i)

Again, would it be more pythonic if blind_remove() could take a list and
automatically work on each element of the list, rather than having to
make the caller iterate?

> +++ b/tests/qemu-iotests/057
> @@ -23,7 +23,7 @@
>  import time
>  import os
>  import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_io, blind_remove
>  
>  test_drv_base_name = 'drive'
>  
> @@ -36,6 +36,8 @@ class ImageSnapshotTestCase(iotests.QMPTestCase):
>  
>      def _setUp(self, test_img_base_name, image_num):
>          self.vm = iotests.VM()
> +        for dev_expect in self.expect:
> +            blind_remove(dev_expect['image'])

Another place where python magic could make the caller nicer?

> +++ b/tests/qemu-iotests/118

> @@ -411,16 +411,16 @@ class TestFloppyInitiallyEmpty(TestInitiallyEmpty):
>  
>  class TestChangeReadOnly(ChangeBaseClass):
>      def setUp(self):
> -        qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
> -        qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
> -        self.vm = iotests.VM()
> -
> -    def tearDown(self):
> -        self.vm.shutdown()
>          os.chmod(old_img, 0666)
>          os.chmod(new_img, 0666)
> -        os.remove(old_img)
> -        os.remove(new_img)
> +        blind_remove(old_img)
> +        blind_remove(new_img)
> +        qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
> +        qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
> +        self.vm = iotests.VM()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()

The script framework doesn't have any problem removing left-over
read-only files, correct?  (If it does, then earlier in the series you
may need to add 'chmod -R u+rwx scratch/$seq' prior to its removal?)

But overall, I didn't see any problems, so I'm okay with:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]