LGTM, thanks.
On Tue, Dec 10, 2013 at 2:45 PM, Santi Raffa <[email protected]> wrote: > I forgot to send this interdiff. :) > > diff --git a/test/py/ganeti.storage.filestorage_unittest.py > b/test/py/ganeti.storage.filestorage_unittest.py > index 918686d..5d66a5d 100755 > --- a/test/py/ganeti.storage.filestorage_unittest.py > +++ b/test/py/ganeti.storage.filestorage_unittest.py > @@ -236,17 +236,20 @@ class TestFileDeviceHelper(testutils.GanetiTestCase): > > _file_path_acceptance_fn=skip_checks) > class TempEnvironment(object): > + > def __init__(self, create_file=False, delete_file=True): > self.create_file = create_file > self.delete_file = delete_file > + > def __enter__(self): > self.directory = tempfile.mkdtemp() > self.subdirectory = io.PathJoin(self.directory, "pinky") > os.mkdir(self.subdirectory) > self.path = io.PathJoin(self.subdirectory, "bunny") > if self.create_file: > - open(self.path, mode="w") > + open(self.path, mode="w").close() > return self > + > def __exit__(self, *args): > if self.delete_file: > os.unlink(self.path) > > On Fri, Dec 6, 2013 at 1:09 PM, Thomas Thrainer <[email protected]> > wrote: > > On Fri, Dec 6, 2013 at 12:31 PM, Santi Raffa <[email protected]> wrote: > >> TestFileDeviceHelper._Make(path).Exists(assert_exists=False) > > > > Why don't you store the FileDeviceHelper in a local variable? That's > valid > > for all the tests here... > > The object doesn't actually have state (its single attribute is > write-once-read-many path), so it doesn't really make any difference; > I created it many times in order to test all branches of the > constructor, but all of that logic is now under CreateFile. > > This is what it'd look like: > > diff --git a/test/py/ganeti.storage.filestorage_unittest.py > b/test/py/ganeti.storage.filestorage_unittest.py > index 5d66a5d..d0f63a4 100755 > --- a/test/py/ganeti.storage.filestorage_unittest.py > +++ b/test/py/ganeti.storage.filestorage_unittest.py > @@ -246,6 +246,7 @@ class TestFileDeviceHelper(testutils.GanetiTestCase): > self.subdirectory = io.PathJoin(self.directory, "pinky") > os.mkdir(self.subdirectory) > self.path = io.PathJoin(self.subdirectory, "bunny") > + self.volume = TestFileDeviceHelper._Make(self.path) > if self.create_file: > open(self.path, mode="w").close() > return self > @@ -258,19 +259,20 @@ class TestFileDeviceHelper(testutils.GanetiTestCase): > return False #don't swallow exceptions > > def testOperationsOnNonExistingFiles(self): > - directory = subdirectory = path = "/e/no/ent" > + path = "/e/no/ent" > + volume = TestFileDeviceHelper._Make(path) > > # These should fail horribly. > - TestFileDeviceHelper._Make(path).Exists(assert_exists=False) > + volume.Exists(assert_exists=False) > self.assertRaises(errors.BlockDeviceError, lambda: \ > - TestFileDeviceHelper._Make(path).Exists(assert_exists=True)) > + volume.Exists(assert_exists=True)) > self.assertRaises(errors.BlockDeviceError, lambda: \ > - TestFileDeviceHelper._Make(path).Size()) > + volume.Size()) > self.assertRaises(errors.BlockDeviceError, lambda: \ > - TestFileDeviceHelper._Make(path).Grow(0.020, True, False, None)) > + volume.Grow(0.020, True, False, None)) > > # Removing however fails silently. > - TestFileDeviceHelper._Make(path).Remove() > + volume.Remove() > > # Make sure we don't create all directories for you unless we ask for > it > self.assertRaises(errors.BlockDeviceError, lambda: \ > @@ -280,10 +282,10 @@ class TestFileDeviceHelper(testutils.GanetiTestCase): > with TestFileDeviceHelper.TempEnvironment() as env: > TestFileDeviceHelper._Make(env.path, create_with_size=1) > > - self.assertTrue(TestFileDeviceHelper._Make(env.path).Exists()) > - TestFileDeviceHelper._Make(env.path).Exists(assert_exists=True) > + self.assertTrue(env.volume.Exists()) > + env.volume.Exists(assert_exists=True) > self.assertRaises(errors.BlockDeviceError, lambda: \ > - TestFileDeviceHelper._Make(env.path).Exists(assert_exists=False)) > + env.volume.Exists(assert_exists=False)) > > self.assertRaises(errors.BlockDeviceError, lambda: \ > TestFileDeviceHelper._Make("/enoent", create_with_size=0.042)) > @@ -297,17 +299,16 @@ class TestFileDeviceHelper(testutils.GanetiTestCase): > def testGrowFile(self): > with TestFileDeviceHelper.TempEnvironment(create_file=True) as env: > self.assertRaises(errors.BlockDeviceError, lambda: \ > - TestFileDeviceHelper._Make(env.path).Grow(-1, False, True, None)) > + env.volume.Grow(-1, False, True, None)) > > - TestFileDeviceHelper._Make(env.path).Grow(2, False, True, None) > - self.assertEqual(2.0, > - TestFileDeviceHelper._Make(env.path).Size() / > 1024.0**2) > + env.volume.Grow(2, False, True, None) > + self.assertEqual(2.0, env.volume.Size() / 1024.0**2) > > def testRemoveFile(self): > with TestFileDeviceHelper.TempEnvironment(create_file=True, > delete_file=False) as env: > - TestFileDeviceHelper._Make(env.path).Remove() > - TestFileDeviceHelper._Make(env.path).Exists(assert_exists=False) > + env.volume.Remove() > + env.volume.Exists(assert_exists=False) > > if __name__ == "__main__": > testutils.GanetiTestProgram() > > > -- > Raffa Santi > Google Germany GmbH > Dienerstr. 12 > 80331 München > > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
