On Fri, Dec 6, 2013 at 12:31 PM, Santi Raffa <[email protected]> wrote:
> 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):
>
Please adhere to the coding guidelines in the tests as well (I know, they
are not linted, but that's no reason to not stick to the guidelines).
> + 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")
>
Doesn't that leave a file descriptor open unnecessarily?
> + 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)
>
Why don't you store the FileDeviceHelper in a local variable? That's valid
for all the tests here...
> - 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
>
--
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
