On 08/30/2017 02:33 PM, Eric Blake wrote: > 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 <jc...@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? >
It should probably either take an iterable, or an arbitrary number of arguments, or both, I dunno. I'm not a python. >> 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 <ebl...@redhat.com> > I'm a little iffy on this patch; I know that ./check can take care of our temp files for us now, but because each python test is itself a little mini-harness, I'm a little leery of moving the teardown to setup and trying to pre-clean the confetti before the test begins. What's the benefit? We still have to clean up these files per-test, but now it's slightly more error-prone and in a weird place. If we want to try to preserve the most-recent-failure-files, perhaps we can define a setting in the python test-runner that allows us to globally skip file cleanup. --js