Re: [PATCH master 05/14] FileStorage: extract file logic to a FileDeviceHelper object

[Santi Raffa]

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 <thoma...@google.com> wrote:
> On Fri, Dec 6, 2013 at 12:31 PM, Santi Raffa <rsa...@google.com> 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