Allon Mureinik has uploaded a new change for review. Change subject: core: ImportVm test cleanup: Don't mock DAOs ......................................................................
core: ImportVm test cleanup: Don't mock DAOs Cleaned up the TestHelperImprotVmCommand to stop mocking DAOs and instead override methods that represent a higher business logic, in order to make the test more readable and understandable. In order to achieve this refactoring, a few blocks of ImportVmCommand.canDoAction() were extracted to methods, which could be overridden. Note that this is an interim patch, performed in order to make the real goal, removing TestHelperImportVmCommand altogether. Change-Id: I19d711cd8df8c0f8eccfe6b7838c16605829b2f6 Signed-off-by: Allon Mureinik <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/TestHelperImportVmCommand.java 2 files changed, 94 insertions(+), 143 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/26/9626/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java index b4d9ef8..13f0ced 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java @@ -197,18 +197,11 @@ return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_ILLEGAL); } - // Load images from Import/Export domain - GetAllFromExportDomainQueryParameters tempVar = - new GetAllFromExportDomainQueryParameters - (getParameters().getStoragePoolId(), getParameters().getSourceDomainId()); - VdcQueryReturnValue qretVal = - getBackend().runInternalQuery(VdcQueryType.GetVmsFromExportDomain, tempVar); - - if (!qretVal.getSucceeded()) { + List<VM> vms = getVmsFromExportDomain(); + if (vms == null) { return false; } - List<VM> vms = (List<VM>) qretVal.getReturnValue(); VM vm = LinqUtils.firstOrNull(vms, new Predicate<VM>() { @Override public boolean eval(VM evalVm) { @@ -268,6 +261,23 @@ return true; } + /** + * Load images from Import/Export domain. + * @return A {@link List} of {@link VM}s, or <code>null</code> if the query to the export domain failed. + */ + protected List<VM> getVmsFromExportDomain() { + GetAllFromExportDomainQueryParameters p = + new GetAllFromExportDomainQueryParameters + (getParameters().getStoragePoolId(), getParameters().getSourceDomainId()); + VdcQueryReturnValue qretVal = getBackend().runInternalQuery(VdcQueryType.GetVmsFromExportDomain, p); + + if (!qretVal.getSucceeded()) { + return null; + } + + return (List<VM>) qretVal.getReturnValue(); + } + private boolean validateImageConfig(List<String> canDoActionMessages, Map<Guid, storage_domains> domainsMap, DiskImage image) { @@ -281,10 +291,7 @@ VM vm = getParameters().getVm(); // check that the imported vm guid is not in engine - VmStatic duplicateVm = getVmStaticDAO().get(getVm().getId()); - if (duplicateVm != null) { - addCanDoActionMessage(VdcBllMessages.VM_CANNOT_IMPORT_VM_EXISTS); - addCanDoActionMessage(String.format("$VmName %1$s", duplicateVm.getvm_name())); + if (!validateNoDuplicateVm()) { return false; } @@ -325,17 +332,8 @@ return false; } - boolean inCluster = false; - List<VDSGroup> groups = getVdsGroupDAO().getAllForStoragePool(getParameters().getStoragePoolId()); - for (VDSGroup group : groups) { - if (group.getId().equals(getParameters().getVdsGroupId())) { - inCluster = true; - break; - } - } - - if (!inCluster) { - return failCanDoAction(VdcBllMessages.VDS_CLUSTER_IS_NOT_VALID); + if (!validateVdsCluster()) { + return false; } Map<storage_domains, Integer> domainMap = getSpaceRequirementsForStorageDomains(imageList); @@ -346,21 +344,11 @@ } } - if (Config.<Boolean> GetValue(ConfigValues.LimitNumberOfNetworkInterfaces, - getVdsGroup().getcompatibility_version().toString())) { - // check that we have no more then 8 interfaces (kvm limitation in - // version 2.x) - if (!VmCommand.validateNumberOfNics(vm.getInterfaces(), null)) { - return failCanDoAction(VdcBllMessages.NETWORK_INTERFACE_EXITED_MAX_INTERFACES); - } + if (!validateNumberOfNics()) { + return false; } - // Check that the USB policy is legal - VmHandler.updateImportedVmUsbPolicy(vm.getStaticData()); - if (!VmHandler.isUsbPolicyLegal(vm.getusb_policy(), - vm.getos(), - getVdsGroup(), - getReturnValue().getCanDoActionMessages())) { + if (!validateUsbPolicy()) { return false; } @@ -369,6 +357,51 @@ } return true; + } + + /** Validate that there is no duplicate VM. */ + protected boolean validateNoDuplicateVm() { + VmStatic duplicateVm = getVmStaticDAO().get(getVm().getId()); + if (duplicateVm != null) { + addCanDoActionMessage(VdcBllMessages.VM_CANNOT_IMPORT_VM_EXISTS); + addCanDoActionMessage(String.format("$VmName %1$s", duplicateVm.getvm_name())); + return false; + } + return true; + } + + /** Validates that that the required cluster exists. */ + protected boolean validateVdsCluster() { + List<VDSGroup> groups = getVdsGroupDAO().getAllForStoragePool(getParameters().getStoragePoolId()); + for (VDSGroup group : groups) { + if (group.getId().equals(getParameters().getVdsGroupId())) { + return true; + } + } + return failCanDoAction(VdcBllMessages.VDS_CLUSTER_IS_NOT_VALID); + } + + /** Validates that there aren't too many NICs. */ + protected boolean validateNumberOfNics() { + if (Config.<Boolean> GetValue(ConfigValues.LimitNumberOfNetworkInterfaces, + getVdsGroup().getcompatibility_version().toString())) { + // check that we have no more then 8 interfaces (kvm limitation in + // version 2.x) + if (!VmCommand.validateNumberOfNics(getParameters().getVm().getInterfaces(), null)) { + return failCanDoAction(VdcBllMessages.NETWORK_INTERFACE_EXITED_MAX_INTERFACES); + } + } + return true; + } + + /** Validates the USB policy. */ + protected boolean validateUsbPolicy() { + VM vm = getParameters().getVm(); + VmHandler.updateImportedVmUsbPolicy(vm.getStaticData()); + return VmHandler.isUsbPolicyLegal(vm.getusb_policy(), + vm.getos(), + getVdsGroup(), + getReturnValue().getCanDoActionMessages()); } private boolean templateExistsOnExportDomain() { @@ -451,7 +484,7 @@ return true; } - private boolean canAddVm() { + protected boolean canAddVm() { // Checking if a desktop with same name already exists boolean exists = (Boolean) getBackend() .runInternalQuery(VdcQueryType.IsVmWithSameNameExist, diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/TestHelperImportVmCommand.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/TestHelperImportVmCommand.java index e297398..b36b4d2 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/TestHelperImportVmCommand.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/TestHelperImportVmCommand.java @@ -1,35 +1,15 @@ package org.ovirt.engine.core.bll; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import java.util.ArrayList; +import java.util.Collections; import java.util.List; -import org.ovirt.engine.core.bll.interfaces.BackendInternal; import org.ovirt.engine.core.common.action.ImportVmParameters; import org.ovirt.engine.core.common.businessentities.StorageDomainStatus; import org.ovirt.engine.core.common.businessentities.StorageDomainType; -import org.ovirt.engine.core.common.businessentities.VDSGroup; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VmTemplate; import org.ovirt.engine.core.common.businessentities.storage_domains; import org.ovirt.engine.core.common.businessentities.storage_pool; -import org.ovirt.engine.core.common.queries.VdcQueryParametersBase; -import org.ovirt.engine.core.common.queries.VdcQueryReturnValue; -import org.ovirt.engine.core.common.queries.VdcQueryType; -import org.ovirt.engine.core.compat.Guid; -import org.ovirt.engine.core.compat.Version; -import org.ovirt.engine.core.dao.BusinessEntitySnapshotDAO; -import org.ovirt.engine.core.dao.StorageDomainDAO; -import org.ovirt.engine.core.dao.StorageDomainStaticDAO; -import org.ovirt.engine.core.dao.VdsGroupDAO; -import org.ovirt.engine.core.dao.VmDAO; -import org.ovirt.engine.core.dao.VmStaticDAO; -import org.ovirt.engine.core.dao.VmStatisticsDAO; -import org.ovirt.engine.core.dao.VmTemplateDAO; public class TestHelperImportVmCommand extends ImportVmCommand { @@ -40,39 +20,8 @@ } @Override - protected boolean checkStoragePool() { + protected boolean validateNoDuplicateVm() { return true; - } - - @Override - protected BusinessEntitySnapshotDAO getBusinessEntitySnapshotDAO() { - return null; - } - - @Override - protected boolean checkTemplateInStorageDomain() { - return true; - } - - @Override - protected VmDAO getVmDAO() { - final VmDAO d = mock(VmDAO.class); - when(d.get(any(Guid.class))).thenReturn(null); - return d; - } - - @Override - public VmStaticDAO getVmStaticDAO() { - final VmStaticDAO d = mock(VmStaticDAO.class); - when(d.get(any(Guid.class))).thenReturn(null); - return d; - } - - @Override - protected VmStatisticsDAO getVmStatisticsDAO() { - final VmStatisticsDAO d = mock(VmStatisticsDAO.class); - when(d.get(any(Guid.class))).thenReturn(null); - return d; } @Override @@ -81,72 +30,41 @@ } @Override - public VdsGroupDAO getVdsGroupDAO() { - VdsGroupDAO d = mock(VdsGroupDAO.class); - List<VDSGroup> list = new ArrayList<VDSGroup>(); - VDSGroup g = new VDSGroup(); - g.setId(getParameters().getVdsGroupId()); - Version v = new Version("2.2"); - g.setcompatibility_version(v); - list.add(g); - when(d.getAllForStoragePool(any(Guid.class))).thenReturn(list); - when(d.get(any(Guid.class))).thenReturn(g); - return d; + protected boolean validateVdsCluster() { + return true; } @Override - public StorageDomainDAO getStorageDomainDAO() { - final storage_domains sd = new storage_domains(); + protected boolean validateNumberOfNics() { + return true; + } + + @Override + protected boolean validateUsbPolicy() { + return true; + } + + @Override + protected storage_domains getSourceDomain() { + storage_domains sd = new storage_domains(); sd.setstorage_domain_type(StorageDomainType.ImportExport); sd.setstatus(StorageDomainStatus.Active); - final StorageDomainDAO d = mock(StorageDomainDAO.class); - when(d.getForStoragePool(any(Guid.class), any(Guid.class))).thenReturn(sd); - return d; - } - - @Override - protected StorageDomainStaticDAO getStorageDomainStaticDAO() { - StorageDomainStaticDAO d = mock(StorageDomainStaticDAO.class); - return d; - } - - @Override - public storage_domains getStorageDomain() { - storage_domains sd = new storage_domains(); - sd.setstatus(StorageDomainStatus.Active); - sd.setavailable_disk_size(10); return sd; } @Override - public BackendInternal getBackend() { - BackendInternal backend = mock(BackendInternal.class); - when(backend.runInternalQuery(eq(VdcQueryType.GetVmsFromExportDomain), any(VdcQueryParametersBase.class))).thenReturn(createVmQueryResult()); - when(backend.runInternalQuery(eq(VdcQueryType.IsVmWithSameNameExist), any(VdcQueryParametersBase.class))).thenReturn(createDuplicateResult()); - return backend; + public VmTemplate getVmTemplate() { + return new VmTemplate(); } @Override - public VmTemplateDAO getVmTemplateDAO() { - VmTemplateDAO d = mock(VmTemplateDAO.class); - when(d.get(any(Guid.class))).thenReturn(new VmTemplate()); - return d; + protected boolean canAddVm() { + return true; } - private VdcQueryReturnValue createVmQueryResult() { - final VdcQueryReturnValue v = new VdcQueryReturnValue(); - List<VM> list = new ArrayList<VM>(); - list.add(createVM()); - v.setReturnValue(list); - v.setSucceeded(true); - return v; - } - - private VdcQueryReturnValue createDuplicateResult() { - final VdcQueryReturnValue v = new VdcQueryReturnValue(); - v.setReturnValue(Boolean.FALSE); - v.setSucceeded(true); - return v; + @Override + protected List<VM> getVmsFromExportDomain() { + return Collections.singletonList(createVM()); } protected VM createVM() { -- To view, visit http://gerrit.ovirt.org/9626 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I19d711cd8df8c0f8eccfe6b7838c16605829b2f6 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
