On Sat, Dec 13, 2014 at 04:40:02PM +0200, Alex Pyrgiotis wrote:
> On 11/10/2014 06:20 PM, Aaron Karper wrote:
> > On Thu, Nov 06, 2014 at 12:30:50AM +0200, Alex Pyrgiotis wrote:
> >> Test if the AttachInstanceDisk and DetachInstanceDisk wrappers update
> >> the configuration as expected.
> >>
> >> Signed-off-by: Alex Pyrgiotis <[email protected] <mailto:[email protected]>>
> >>
> >> diff --git a/test/py/ganeti.config___unittest.py
> > <http://ganeti.config_unittest.py> b/test/py/ganeti.config___unittest.py
> > <http://ganeti.config_unittest.py>
> >> index 22f34dd..0af8c2f 100755
> >> --- a/test/py/ganeti.config___unittest.py
> > <http://ganeti.config_unittest.py>
> >> +++ b/test/py/ganeti.config___unittest.py
> > <http://ganeti.config_unittest.py>
> >> @@ -661,6 +661,60 @@ class TestConfigRunner(unittest.__TestCase):
> >> cfg.__RemoveNodeFromCandidateCerts(__node_uuid, warn_fn=None)
> >> self.assertEqual(0, len(cfg.GetCandidateCerts()))
> >>
> >> + def testAttachDetachDisks(self):
> >> + """Test if the attach/detach wrappers work properly."""
> >> + # construct instance
> >> + cfg = self._get_object_mock()
> >> + inst = self._create_instance(cfg)
> >> + disk = objects.Disk(dev_type=__constants.DT_PLAIN, size=128,
> >> + logical_id=("myxenvg", "disk25494"),
> > uuid="disk0")
> >> + fake_disk = objects.Disk(dev_type=__constants.DT_PLAIN, size=128,
> >> + logical_id=("myxenvg", "disk25494"),
> > uuid="disk1")
> >> + cfg.AddInstance(inst, "my-job")
> >> + cfg.AddInstanceDisk(inst.uuid, disk)
> >> +
> >> + # Test 1a - Detach disk from non-existent instance
> >> + with self.assertRaises(errors.__ConfigurationError) as cm:
> >> + cfg.DetachInstanceDisk("1134", "disk0")
> >
> > Python 2.7 feature: assertRaises as a context manager.
> >
> >> + self.assertEqual(cm.exception.__message, "Instance 1134 doesn't
> > exist")
> >> +
> >> + # Test 1b - Detach non-existent disk
> >> + with self.assertRaises(errors.__ConfigurationError) as cm:
> >> + cfg.DetachInstanceDisk("test-__uuid", "disk1")
> >> + self.assertEqual(cm.exception.__message, "Disk disk1 doesn't exist")
> >> +
> >> + # Test 1c - Detach disk
> >> + cfg.DetachInstanceDisk("test-__uuid", "disk0")
> >> + instance_disks = cfg.GetInstanceDisks("test-__uuid")
> >> + self.assertEqual(instance___disks, [])
> >> +
> >> + # Test 1d - Detach disk again
> >> + with self.assertRaises(errors.__ProgrammerError) as cm:
> >> + cfg.DetachInstanceDisk("test-__uuid", "disk0")
> >> + self.assertEqual(cm.exception.__message, "Disk disk0 is not
> > attached to an"
> >> + " instance")
> >> +
> >> + # Test 2a - Attach disk to non-existent instance
> >> + with self.assertRaises(errors.__ConfigurationError) as cm:
> >> + cfg.AttachInstanceDisk("1134", disk)
> >> + self.assertEqual(cm.exception.__message, "Instance 1134 doesn't
> > exist")
> >> +
> >> + # Test 2b - Attach non-existent disk
> >> + with self.assertRaises(errors.__ConfigurationError) as cm:
> >> + cfg.AttachInstanceDisk("test-__uuid", fake_disk)
> >> + self.assertEqual(cm.exception.__message, "Disk disk1 doesn't exist")
> >> +
> >> + # Test 2c - Attach disk
> >> + cfg.AttachInstanceDisk("test-__uuid", disk)
> >> + instance_disks = cfg.GetInstanceDisks("test-__uuid")
> >> + self.assertEqual(instance___disks, [disk])
> >> +
> >> + # Test 2d - Attach disk again
> >> + with self.assertRaises(errors.__ReservationError) as cm:
> >> + cfg.AttachInstanceDisk("test-__uuid", disk)
> >> + self.assertEqual(cm.exception.__message, "Disk disk0 already
> > attached to"
> >> + " instance test.example.com
> > <http://test.example.com>")
> >> +
> >
> > Please split into setUp and multiple test methods.
> >
>
> I'd argue that this is not needed in this test case. This test checks if
> the configuration remains in a consistent state after a series of
> detach/attach ops. Thus, the assertions must all be in one test case.
> Also, if you see one test above, ``testAddAndRemoveCerts`` does that too.
>
> I believe that the issue with this test is that I have marked parts of
> the code as "Tests" in my comments. These should be called just "steps",
> or nothing at all. If it's ok with you, I'll fix that.
>
Yes, that makes sense. Calling them steps is a good idea, maybe add the
explanation that you gave me ("This test checks if the configuration
remains in a consistent state after a series of detach/attach ops") as a
doc string or comment.
> >>
> >> def _IsErrorInList(err_str, err_list):
> >> return any(map(lambda e: err_str in e, err_list))
> >> --
> >> 1.7.10.4
> >>
> >
> > --
> > 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
>
>
> --
> Alex | [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
