As promised the tests refactoring interdiff:
diff --git a/lib/storage/filestorage.py b/lib/storage/filestorage.py
index cdfa0de..c980d17 100644
--- a/lib/storage/filestorage.py
+++ b/lib/storage/filestorage.py
@@ -132,7 +132,7 @@ class FileDeviceHelper(object):
except OSError as err:
base.ThrowError("%s: can't stat: %s", self.path, err)
- def Grow(self, amount, dryrun, backingstore, excl_stor):
+ def Grow(self, amount, dryrun, backingstore, _excl_stor):
"""Grow the file
@param amount: the amount (in mebibytes) to grow by.
@@ -241,7 +241,7 @@ class FileStorage(base.BlockDev):
return
if dryrun:
return
- self.file.Grow(amount, amount, dryrun, backingstore, excl_stor)
+ self.file.Grow(amount, dryrun, backingstore, excl_stor)
def Attach(self):
"""Attach to an existing file.
diff --git a/test/py/ganeti.storage.filestorage_unittest.py
b/test/py/ganeti.storage.filestorage_unittest.py
index 921bb28..918686d 100755
--- a/test/py/ganeti.storage.filestorage_unittest.py
+++ b/test/py/ganeti.storage.filestorage_unittest.py
@@ -235,61 +235,76 @@ class TestFileDeviceHelper(testutils.GanetiTestCase):
return filestorage.FileDeviceHelper(path,
_file_path_acceptance_fn=skip_checks)
-
- def test(self):
- # Get temp directory
- directory = tempfile.mkdtemp()
- subdirectory = io.PathJoin(directory, "pinky")
- path = io.PathJoin(subdirectory, "bunny")
-
- should_fail = lambda fn: self.assertRaises(errors.BlockDeviceError, fn)
-
- # Make sure it doesn't exist, and methods check for it
+ 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")
+ return self
+ def __exit__(self, *args):
+ if self.delete_file:
+ os.unlink(self.path)
+ os.rmdir(self.subdirectory)
+ os.rmdir(self.directory)
+ return False #don't swallow exceptions
+
+ def testOperationsOnNonExistingFiles(self):
+ directory = subdirectory = path = "/e/no/ent"
+
+ # These should fail horribly.
TestFileDeviceHelper._Make(path).Exists(assert_exists=False)
- should_fail( lambda: \
+ self.assertRaises(errors.BlockDeviceError, lambda: \
TestFileDeviceHelper._Make(path).Exists(assert_exists=True))
- should_fail( lambda: \
+ self.assertRaises(errors.BlockDeviceError, lambda: \
TestFileDeviceHelper._Make(path).Size())
- should_fail( lambda: \
+ self.assertRaises(errors.BlockDeviceError, lambda: \
TestFileDeviceHelper._Make(path).Grow(0.020, True, False, None))
# Removing however fails silently.
TestFileDeviceHelper._Make(path).Remove()
# Make sure we don't create all directories for you unless we ask for it
- should_fail( lambda: \
+ self.assertRaises(errors.BlockDeviceError, lambda: \
TestFileDeviceHelper._Make(path, create_with_size=1))
- # Create the file.
- TestFileDeviceHelper._Make(path, create_with_size=1, create_folders=True)
-
- # This should still fail.
- should_fail( lambda: \
- TestFileDeviceHelper._Make(subdirectory).Size())
-
-
- self.assertTrue(TestFileDeviceHelper._Make(path).Exists())
-
- should_fail( lambda: \
- TestFileDeviceHelper._Make(path, create_with_size=0.042))
-
- TestFileDeviceHelper._Make(path).Exists(assert_exists=True)
- should_fail( lambda: \
- TestFileDeviceHelper._Make(path).Exists(assert_exists=False))
-
- should_fail( lambda: \
- TestFileDeviceHelper._Make(path).Grow(-1, False, True, None))
-
- TestFileDeviceHelper._Make(path).Grow(2, False, True, None)
- self.assertEqual(3.0,
- TestFileDeviceHelper._Make(path).Size() / 1024.0**2)
-
- TestFileDeviceHelper._Make(path).Remove()
- TestFileDeviceHelper._Make(path).Exists(assert_exists=False)
-
- os.rmdir(subdirectory)
- os.rmdir(directory)
-
+ def testFileCreation(self):
+ 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.assertRaises(errors.BlockDeviceError, lambda: \
+ TestFileDeviceHelper._Make(env.path).Exists(assert_exists=False))
+
+ self.assertRaises(errors.BlockDeviceError, lambda: \
+ TestFileDeviceHelper._Make("/enoent", create_with_size=0.042))
+
+ def testFailSizeDirectory(self):
+ # This should still fail.
+ with TestFileDeviceHelper.TempEnvironment(delete_file=False) as env:
+ self.assertRaises(errors.BlockDeviceError, lambda: \
+ TestFileDeviceHelper._Make(env.subdirectory).Size())
+
+ 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))
+
+ TestFileDeviceHelper._Make(env.path).Grow(2, False, True, None)
+ self.assertEqual(2.0,
+ TestFileDeviceHelper._Make(env.path).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)
if __name__ == "__main__":
testutils.GanetiTestProgram()
On Thu, Dec 5, 2013 at 3:42 PM, Santi Raffa <[email protected]> wrote:
> On Thu, Dec 5, 2013 at 1:22 PM, Thomas Thrainer <[email protected]> wrote:
>> On Thu, Dec 5, 2013 at 10:19 AM, Santi Raffa <[email protected]> wrote:
>>> + if not _file_path_acceptance_fn:
>>> + _file_path_acceptance_fn = CheckFileStoragePathAcceptance
>>> + _file_path_acceptance_fn(path)
>>> +
>>
>>
>> I think it'd be cleaner to put this as the default value in the parameter
>> list.
>
> I tried that, but it's not possible because this is a top-level
> definition that comes before CheckFileStoragePathAcceptance's; trying
> to do that gets you:
>
> NameError: name 'CheckFileStoragePathAcceptance' is not defined
>
>>> - def Grow(self, amount):
>>> + def Grow(self, amount, dryrun, backingstore, excl_stor):
>>
>>
>> Why excl_stor?
>
> If we're going to modify the implementation of FileDeviceHelper.Grow
> to handle the disk templates' parameters uniformly, I figured we
> might've as well passed all of the parameters to it.
>
> OTOH, changing the signature of Grow and the name of Create requires
> changing the tests to match.
>
> --- a/test/py/ganeti.storage.filestorage_unittest.py
> +++ b/test/py/ganeti.storage.filestorage_unittest.py
> @@ -227,7 +227,7 @@ class TestFileDeviceHelper(testutils.GanetiTestCase):
> def _Make(path, create_with_size=None, create_folders=False):
> skip_checks = lambda path: None
> if create_with_size:
> - return filestorage.FileDeviceHelper.Create(
> + return filestorage.FileDeviceHelper.CreateFile(
> path, create_with_size, create_folders=create_folders,
> _file_path_acceptance_fn=skip_checks
> )
> @@ -251,7 +251,7 @@ class TestFileDeviceHelper(testutils.GanetiTestCase):
> should_fail( lambda: \
> TestFileDeviceHelper._Make(path).Size())
> should_fail( lambda: \
> - TestFileDeviceHelper._Make(path).Grow(20))
> + TestFileDeviceHelper._Make(path).Grow(20, True, False, None))
>
> # Removing however fails silently.
> TestFileDeviceHelper._Make(path).Remove()
> @@ -278,9 +278,9 @@ class TestFileDeviceHelper(testutils.GanetiTestCase):
> TestFileDeviceHelper._Make(path).Exists(assert_exists=False))
>
> should_fail( lambda: \
> - TestFileDeviceHelper._Make(path).Grow(-30))
> + TestFileDeviceHelper._Make(path).Grow(-30, True, False, None))
>
> - TestFileDeviceHelper._Make(path).Grow(58)
> + TestFileDeviceHelper._Make(path).Grow(58, True, False, None)
> self.assertEqual(100 * 1024 * 1024,
> TestFileDeviceHelper._Make(path).Size())
>
> --
> 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
--
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
