On Tue, Oct 15, 2013 at 1:53 PM, Thomas Thrainer <[email protected]>wrote:
> > > > On Tue, Oct 15, 2013 at 10:06 AM, Helga Velroyen <[email protected]>wrote: > >> >> >> >> On Tue, Oct 15, 2013 at 12:01 AM, Thomas Thrainer <[email protected]>wrote: >> >>> This patch adds unit tests for LUBackupExport. >>> >>> Signed-off-by: Thomas Thrainer <[email protected]> >>> --- >>> lib/cmdlib/backup.py | 2 +- >>> test/py/cmdlib/backup_unittest.py | 114 >>> +++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 113 insertions(+), 3 deletions(-) >>> >>> diff --git a/lib/cmdlib/backup.py b/lib/cmdlib/backup.py >>> index 06920f5..79e7d27 100644 >>> --- a/lib/cmdlib/backup.py >>> +++ b/lib/cmdlib/backup.py >>> @@ -396,7 +396,7 @@ class LUBackupExport(LogicalUnit): >>> activate_disks = not self.instance.disks_active >>> >>> if activate_disks: >>> - # Activate the instance disks if we'exporting a stopped instance >>> + # Activate the instance disks if we're exporting a stopped >>> instance >>> feedback_fn("Activating disks for %s" % self.instance.name) >>> StartInstanceDisks(self, self.instance, None) >>> >>> diff --git a/test/py/cmdlib/backup_unittest.py >>> b/test/py/cmdlib/backup_unittest.py >>> index 3843071..db938f2 100644 >>> --- a/test/py/cmdlib/backup_unittest.py >>> +++ b/test/py/cmdlib/backup_unittest.py >>> @@ -21,9 +21,8 @@ >>> >>> """Tests for LUBackup*""" >>> >>> -import mock >>> - >>> from ganeti import constants >>> +from ganeti import objects >>> from ganeti import opcodes >>> from ganeti import query >>> >>> @@ -110,5 +109,116 @@ qRNQR5h9LJfZv7zZZSI= >>> self.ExecOpCode(op) >>> >>> https://code.google.com/a/google.com/p/ganeti-eng/wiki/Scripts#/google/data/rw/teams/ganeti/eng/scripts/helgav/prepare_bugrepor >>> >>> +class TestLUBackupExportBase(CmdlibTestCase): >>> + def setUp(self): >>> + super(TestLUBackupExportBase, self).setUp() >>> + >>> + self.rpc.call_instance_start.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, True) >>> + >>> + self.rpc.call_blockdev_assemble.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, None) >>> + >>> + self.rpc.call_blockdev_shutdown.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, None) >>> + >>> + self.rpc.call_blockdev_snapshot.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, ("mock_vg", "mock_id")) >>> + >>> + self.rpc.call_blockdev_remove.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, None) >>> + >>> + self.rpc.call_export_start.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, "export_daemon") >>> + >>> + def ImpExpStatus(node_uuid, name): >>> + return self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(node_uuid, >>> + [objects.ImportExportStatus( >>> + exit_status=0 >>> + )]) >>> + self.rpc.call_impexp_status.side_effect = ImpExpStatus >>> + >>> + def ImpExpCleanup(node_uuid, name): >>> + return self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(node_uuid) >>> + self.rpc.call_impexp_cleanup.side_effect = ImpExpCleanup >>> + >>> + self.rpc.call_finalize_export.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.master, None) >>> + >>> + def testRemoveRunningInstanceWithoutShutdown(self): >>> + inst = self.cfg.AddNewInstance(admin_state=constants.ADMINST_UP) >>> + op = opcodes.OpBackupExport(instance_name=inst.name, >>> + target_node=self.master.name, >>> + shutdown=False, >>> + remove_instance=True) >>> + self.ExecOpCodeExpectOpPrereqError( >>> + op, "Can not remove instance without shutting it down before") >>> + >>> + def testUnsupportedDiskTemplate(self): >>> + inst = self.cfg.AddNewInstance(disk_template=constants.DT_FILE) >>> + op = opcodes.OpBackupExport(instance_name=inst.name, >>> + target_node=self.master.name) >>> + self.ExecOpCodeExpectOpPrereqError( >>> + op, "Export not supported for instances with file-based disks") >>> >> >> I find it a bit odd to include a unittest that does not test the >> requirements, but tests for an error message which is only a (poor) >> workaround of an issue that needs to be fixed. Fixing the issue would >> actually break the test then. >> > > I agree with you, but given that Python is a dynamic language, where a > simple typo might break this code path, I wanted to go for as much coverage > as possible in order to exercise the code at least during the tests once. > If the actual code is fixed, then this test should be removed (instead of > testing for no error) IMHO, as this test is mainly for exercising all code > paths... > I see your point, and as I LGTM'ed already, I think it is okay to add this test for now. > > >> >> >>> + >>> + >>> +class TestLUBackupExportLocalExport(TestLUBackupExportBase): >>> + def setUp(self): >>> + super(TestLUBackupExportLocalExport, self).setUp() >>> + >>> + self.inst = self.cfg.AddNewInstance() >>> + self.target_node = self.cfg.AddNewNode() >>> + self.op = opcodes.OpBackupExport(mode=constants.EXPORT_MODE_LOCAL, >>> + instance_name=self.inst.name, >>> + target_node=self.target_node.name) >>> + >>> + self.rpc.call_import_start.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .CreateSuccessfulNodeResult(self.target_node, "import_daemon") >>> + >>> + def testExportWithShutdown(self): >>> + inst = self.cfg.AddNewInstance(admin_state=constants.ADMINST_UP) >>> + op = self.CopyOpCode(self.op, instance_name=inst.name, >>> shutdown=True) >>> + self.ExecOpCode(op) >>> + >>> + def testExportDeactivatedDisks(self): >>> + self.ExecOpCode(self.op) >>> + >>> + def testExportRemoveInstance(self): >>> + op = self.CopyOpCode(self.op, remove_instance=True) >>> + self.ExecOpCode(op) >>> + >>> + >>> +class TestLUBackupExportRemoteExport(TestLUBackupExportBase): >>> + def setUp(self): >>> + super(TestLUBackupExportRemoteExport, self).setUp() >>> + >>> + self.inst = self.cfg.AddNewInstance() >>> + self.op = opcodes.OpBackupExport(mode=constants.EXPORT_MODE_REMOTE, >>> + instance_name=self.inst.name, >>> + target_node=[], >>> + x509_key_name=["mock_key_name"], >>> + destination_x509_ca="mock_dest_ca") >>> + >>> + def testRemoteExportWithoutX509KeyName(self): >>> + op = self.CopyOpCode(self.op, x509_key_name=self.REMOVE) >>> + self.ExecOpCodeExpectOpPrereqError(op, >>> + "Missing X509 key name for >>> encryption") >>> + >>> + def testRemoteExportWithoutX509DestCa(self): >>> + op = self.CopyOpCode(self.op, destination_x509_ca=self.REMOVE) >>> + self.ExecOpCodeExpectOpPrereqError(op, >>> + "Missing destination X509 CA") >>> + >>> + >>> if __name__ == "__main__": >>> testutils.GanetiTestProgram() >>> -- >>> 1.8.4 >>> >>> >> LGTM, thanks >> >> >> -- >> -- >> Helga Velroyen | 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 >> > > > > -- > 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 > -- -- Helga Velroyen | 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
