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. >> >> 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]
