Alona Kaplan has posted comments on this change. Change subject: core: tests for NetworkAttachmentValidator ......................................................................
Patch Set 40: (26 comments) https://gerrit.ovirt.org/#/c/36570/40/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidator.java: Line 49: return ValidationResult.failWith(VdcBllMessages.EXTERNAL_NETWORK_CANNOT_BE_PROVISIONED) Line 50: .when(getNetwork().isExternal()); Line 51: } Line 52: Line 53: public ValidationResult notRemovingManagementNetwork() { This method check whether the attachment related to management network. I doesn't check whether yo remove it. I would keep the previous name. Line 54: return createNetworkValidator().notManagementNetwork(); Line 55: } Line 56: Line 57: public ValidationResult networkAttachedToCluster() { https://gerrit.ovirt.org/#/c/36570/40/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/NetworkAttachmentValidatorTest.java: Line 49: private ManagementNetworkUtil managementNetworkUtil; Line 50: Line 51: @Before Line 52: public void setUp() throws Exception { Line 53: networkDaoMock = mock(NetworkDao.class); Please add @RunWith(MockitoJUnitRunner.class) to the class, so you can use @Mock. Line 54: networkClusterDaoMock = mock(NetworkClusterDao.class); Line 55: vdsDaoMock = mock(VdsDAO.class); Line 56: managementNetworkUtil = mock(ManagementNetworkUtil.class); Line 57: Line 54: networkClusterDaoMock = mock(NetworkClusterDao.class); Line 55: vdsDaoMock = mock(VdsDAO.class); Line 56: managementNetworkUtil = mock(ManagementNetworkUtil.class); Line 57: Line 58: when(DbFacade.getInstance().getNetworkDao()).thenReturn(networkDaoMock); This is a general comment- daos should be injected, DbFacade.getInstance() should be deprecated. You can pass the daos to NetworkAttachmentValidator via the ctor (they can be injected in the command). Same as you pass the managementNetworkUtil. Line 59: when(DbFacade.getInstance().getNetworkClusterDao()).thenReturn(networkClusterDaoMock); Line 60: when(DbFacade.getInstance().getVdsDao()).thenReturn(vdsDaoMock); Line 61: } Line 62: Line 77: implementation which returns failing ValidationResult on NetworkValidator#networkIsSet. Finally it's tested, whether Line 78: this ValidationResult was propagated correctly. Line 79: */ Line 80: @Test Line 81: public void testNetworkExists() throws Exception { Please add test for the valid use case. Line 82: NetworkAttachmentValidator networkAttachmentValidatorSpy = Mockito.spy( Line 83: new NetworkAttachmentValidator(new NetworkAttachment(), new VDS(), managementNetworkUtil)); Line 84: Line 85: NetworkValidator networkValidatorSpy = Mockito.spy(new NetworkValidator(new Network())); Line 81: public void testNetworkExists() throws Exception { Line 82: NetworkAttachmentValidator networkAttachmentValidatorSpy = Mockito.spy( Line 83: new NetworkAttachmentValidator(new NetworkAttachment(), new VDS(), managementNetworkUtil)); Line 84: Line 85: NetworkValidator networkValidatorSpy = Mockito.spy(new NetworkValidator(new Network())); The NetworkValidator can be a mock. Line 86: doReturn(networkValidatorSpy).when(networkAttachmentValidatorSpy).createNetworkValidator(); Line 87: Line 88: ValidationResult propagatedResult = new ValidationResult(VdcBllMessages.NETWORK_NOT_EXISTS, "a"); Line 89: doReturn(propagatedResult).when(networkValidatorSpy).networkIsSet(); Line 97: public void testNotExternalNetworkWhenExternalNetworkIsProvided() throws Exception { Line 98: Network externalNetwork = new Network(); Line 99: externalNetwork.setProvidedBy(new ProviderNetwork(Guid.newGuid(), "")); Line 100: Line 101: when(networkDaoMock.get(any(Guid.class))).thenReturn(externalNetwork); Please be more explicit- setId on the network, pass to the get this id, set the id on the networkId property of the NetworkAttachment. Line 102: Line 103: assertThat(new NetworkAttachmentValidator(new NetworkAttachment(), new VDS(), managementNetworkUtil).notExternalNetwork(), Line 104: failsWith(VdcBllMessages.EXTERNAL_NETWORK_CANNOT_BE_PROVISIONED)); Line 105: } Line 104: failsWith(VdcBllMessages.EXTERNAL_NETWORK_CANNOT_BE_PROVISIONED)); Line 105: } Line 106: Line 107: @Test Line 108: public void testNotExternalNetwork() throws Exception { same comment as the previous. Line 109: Network notExternalNetwork = new Network(); Line 110: notExternalNetwork.setProvidedBy(null); Line 111: Line 112: when(networkDaoMock.get(any(Guid.class))).thenReturn(notExternalNetwork); Line 111: Line 112: when(networkDaoMock.get(any(Guid.class))).thenReturn(notExternalNetwork); Line 113: Line 114: assertThat(new NetworkAttachmentValidator(new NetworkAttachment(), new VDS(), managementNetworkUtil).notExternalNetwork(), isValid()); Line 115: Redundant empty line. Line 116: } Line 117: Line 118: @Test Line 119: public void testNotRemovingManagementNetwork() throws Exception { Line 119: public void testNotRemovingManagementNetwork() throws Exception { Line 120: NetworkAttachmentValidator networkAttachmentValidatorSpy = Mockito.spy( Line 121: new NetworkAttachmentValidator(new NetworkAttachment(), new VDS(), managementNetworkUtil)); Line 122: Line 123: NetworkValidator networkValidatorSpy = Mockito.spy(new NetworkValidator(new Network())); the networkValidator can be a mock. Line 124: doReturn(networkValidatorSpy).when(networkAttachmentValidatorSpy).createNetworkValidator(); Line 125: Line 126: ValidationResult propagatedResult = Line 127: new ValidationResult(VdcBllMessages.NETWORK_CANNOT_REMOVE_MANAGEMENT_NETWORK, "a"); Line 143: NetworkAttachment attachment = new NetworkAttachment(); Line 144: attachment.setNetworkId(network.getId()); Line 145: Line 146: NetworkClusterId networkClusterId = new NetworkClusterId(host.getVdsGroupId(), attachment.getNetworkId()); Line 147: when(networkClusterDaoMock.get(eq(networkClusterId))).thenReturn(new NetworkCluster()); The comment should have been placed in NetworkAttachmentValidator.getNetworkCluster but just here I noticed it- why do you query the networkDao? You need just the networkId, you have it on the attachment. Line 148: when(networkDaoMock.get(Mockito.eq(network.getId()))).thenReturn(network); Line 149: Line 150: assertThat(new NetworkAttachmentValidator(attachment, host, managementNetworkUtil).networkAttachedToCluster(), isValid()); Line 151: } Line 144: attachment.setNetworkId(network.getId()); Line 145: Line 146: NetworkClusterId networkClusterId = new NetworkClusterId(host.getVdsGroupId(), attachment.getNetworkId()); Line 147: when(networkClusterDaoMock.get(eq(networkClusterId))).thenReturn(new NetworkCluster()); Line 148: when(networkDaoMock.get(Mockito.eq(network.getId()))).thenReturn(network); Why do you need the Mockito prefix here? Line 149: Line 150: assertThat(new NetworkAttachmentValidator(attachment, host, managementNetworkUtil).networkAttachedToCluster(), isValid()); Line 151: } Line 152: Line 177: assertThat(validator.ipConfiguredForStaticBootProtocol(), isValid()); Line 178: } Line 179: Line 180: @Test Line 181: public void testIpConfiguredForStaticBootProtocolWhenIpConfigurationIsNotNullAndBootProtocolIsNotStatic() throws Exception { Please separate to two tests. Line 182: Line 183: assertThat( Line 184: new NetworkAttachmentValidator( Line 185: createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.DHCP, null, null), Line 197: Line 198: @Test Line 199: public void testIpConfiguredForStaticBootProtocolWhenIpConfigurationIsNotNullAndBootProtocolIsStaticAndAddressIsNull() throws Exception { Line 200: NetworkAttachment attachment = Line 201: createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.STATIC_IP, null, ""); null and "" are considered as empty strings. I would except the following tests- 1. createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.STATIC_IP, null, "255.255.255.0"); 2. createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.STATIC_IP, "", "255.255.255.0"); Line 202: Line 203: assertThat(new NetworkAttachmentValidator(attachment, new VDS(), managementNetworkUtil).ipConfiguredForStaticBootProtocol(), Line 204: failsWith(VdcBllMessages.NETWORK_ADDR_MANDATORY_IN_STATIC_IP)); Line 205: } Line 205: } Line 206: Line 207: @Test Line 208: public void testIpConfiguredForStaticBootProtocolWhenIpConfigurationIsNotNullAndBootProtocolIsStaticAndNetmaskIsNull() throws Exception { Line 209: NetworkAttachment attachment = same- null and "" are considered as empty strings. I would except the following tests- 1. createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.STATIC_IP, 192.168.1.1, null); 2.createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.STATIC_IP, 192.168.1.1, ""); Line 210: createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.STATIC_IP, "", null); Line 211: Line 212: assertThat(new NetworkAttachmentValidator(attachment, new VDS(), managementNetworkUtil).ipConfiguredForStaticBootProtocol(), Line 213: failsWith(VdcBllMessages.NETWORK_ADDR_MANDATORY_IN_STATIC_IP)); Line 223: assertThat(new NetworkAttachmentValidator(attachment, new VDS(), managementNetworkUtil).ipConfiguredForStaticBootProtocol(), Line 224: isValid()); Line 225: } Line 226: Line 227: private NetworkAttachment createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol staticIp, s/staticIp/bootProtocol Line 228: String address, Line 229: String netmask) { Line 230: Line 231: IpConfiguration ipConfiguration = new IpConfiguration(); Line 241: return attachment; Line 242: } Line 243: Line 244: @Test Line 245: public void testBootProtocolSetForDisplayNetworkWhenIpConfigurationWhenNetworkClusterDisplayIsFalse() throws Exception { When(Null)IpConfiguration s/testBootProtocolSetForDisplayNetworkWhenIpConfigurationWhenNetworkClusterDisplayIsFalse/testBootProtocolSetForDisplayNetworkNullIpConfigurationFalseDisplay Line 246: Network network = new Network(); Line 247: network.setId(Guid.newGuid()); Line 248: Line 249: NetworkAttachment attachment = new NetworkAttachment(); Line 259: NetworkClusterId networkClusterId = new NetworkClusterId(host.getVdsGroupId(), attachment.getNetworkId()); Line 260: when(networkClusterDaoMock.get(eq(networkClusterId))).thenReturn(networkCluster); Line 261: when(networkDaoMock.get(Mockito.eq(network.getId()))).thenReturn(network); Line 262: Line 263: assertThat(new NetworkAttachmentValidator(attachment, host, managementNetworkUtil).bootProtocolSetForDisplayNetwork(), isValid()); The comment is relevant to the validator itself and not the tests- shouldn't you have a validation to make sure the ipConfiguration is set? Is it valid to have attachment with no IpConfiguration at all? Same question for bootProtocol. Is it valid to have null boot protocol? Line 264: } Line 265: Line 266: @Test Line 267: public void testBootProtocolSetForDisplayNetworkWhenIpConfigurationIsNull() throws Exception { Line 263: assertThat(new NetworkAttachmentValidator(attachment, host, managementNetworkUtil).bootProtocolSetForDisplayNetwork(), isValid()); Line 264: } Line 265: Line 266: @Test Line 267: public void testBootProtocolSetForDisplayNetworkWhenIpConfigurationIsNull() throws Exception { has a lot of common with the previous method, please extract. Line 268: Network network = new Network(); Line 269: network.setId(Guid.newGuid()); Line 270: Line 271: NetworkAttachment attachment = new NetworkAttachment(); Line 306: when(networkDaoMock.get(Mockito.eq(network.getId()))).thenReturn(network); Line 307: Line 308: assertThat(new NetworkAttachmentValidator(attachment, host, managementNetworkUtil).bootProtocolSetForDisplayNetwork(), Line 309: failsWith(VdcBllMessages.ACTION_TYPE_FAILED_DISPLAY_NETWORK_HAS_NO_BOOT_PROTOCOL)); Line 310: } Please add a valid test for display network (there is a valid test just for non-display network). Line 311: Line 312: @Test Line 313: public void testNicExistsWhenNicNameIsNull() throws Exception { Line 314: NetworkAttachment attachment = new NetworkAttachment(); Line 413: assertThat(new NetworkAttachmentValidator(attachment, null, managementNetworkUtil).networkNotChanged(oldAttachment), isValid()); Line 414: } Line 415: Line 416: @Test Line 417: public void testValidateGateway() throws Exception { Please add more tests for this validation (valid, mutipleGateway enabled, no ip config, no primary address set, etc.). Line 418: NetworkAttachment attachment = Line 419: createNetworkAttachmentWithIpConfiguration(NetworkBootProtocol.NONE, null, null); Line 420: Line 421: attachment.getIpConfiguration().getPrimaryAddress().setGateway("someGateway"); Line 423: Network network = new Network(); Line 424: network.setId(Guid.newGuid()); Line 425: network.setName("networkName"); Line 426: Line 427: IConfigUtilsInterface iConfigUtilsInterfaceMock = mock(IConfigUtilsInterface.class); Please use MockConfigRule- example of using the rule- @ClassRule public static final MockConfigRule mcr = new MockConfigRule( mockConfig(ConfigValues.HotPlugEnabled, Version.v3_1.getValue(), true), mockConfig(ConfigValues.HotPlugDiskSnapshotSupported, Version.v3_1.getValue(), true)); Line 428: Config.setConfigUtils(iConfigUtilsInterfaceMock); Line 429: Line 430: when(iConfigUtilsInterfaceMock.getValue(eq(ConfigValues.MultipleGatewaysSupported), anyString())).thenReturn(false); Line 431: when(networkDaoMock.get(any(Guid.class))).thenReturn(network); Line 427: IConfigUtilsInterface iConfigUtilsInterfaceMock = mock(IConfigUtilsInterface.class); Line 428: Config.setConfigUtils(iConfigUtilsInterfaceMock); Line 429: Line 430: when(iConfigUtilsInterfaceMock.getValue(eq(ConfigValues.MultipleGatewaysSupported), anyString())).thenReturn(false); Line 431: when(networkDaoMock.get(any(Guid.class))).thenReturn(network); Please be explicit, pass the actual id. Line 432: when(managementNetworkUtil.isManagementNetwork(network.getId())).thenReturn(false); Line 433: Line 434: VDS host = new VDS(); Line 435: host.setVdsGroupCompatibilityVersion(Version.getLast()); Line 439: } Line 440: Line 441: @Test Line 442: public void testNetworkNotAttachedToHost() throws Exception { Line 443: //no vds for network id. space after // Line 444: when(vdsDaoMock.getAllForNetwork(any(Guid.class))).thenReturn(Collections.<VDS>emptyList()); Line 445: Line 446: assertThat(new NetworkAttachmentValidator(new NetworkAttachment(), null, managementNetworkUtil).networkNotAttachedToHost(), isValid()); Line 447: } Line 440: Line 441: @Test Line 442: public void testNetworkNotAttachedToHost() throws Exception { Line 443: //no vds for network id. Line 444: when(vdsDaoMock.getAllForNetwork(any(Guid.class))).thenReturn(Collections.<VDS>emptyList()); Please be explicit, set the networkId. Line 445: Line 446: assertThat(new NetworkAttachmentValidator(new NetworkAttachment(), null, managementNetworkUtil).networkNotAttachedToHost(), isValid()); Line 447: } Line 448: Line 450: public void testNetworkNotAttachedToHostWhenAttached() throws Exception { Line 451: VDS host = new VDS(); Line 452: host.setId(Guid.newGuid()); Line 453: Line 454: //no vds for network id. The comment is wrong, remove it. Line 455: when(vdsDaoMock.getAllForNetwork(any(Guid.class))).thenReturn(Collections.singletonList(host)); Line 456: Line 457: assertThat(new NetworkAttachmentValidator(new NetworkAttachment(), host, managementNetworkUtil).networkNotAttachedToHost(), Line 458: failsWith(VdcBllMessages.NETWORK_ALREADY_ATTACHED_TO_HOST)); Line 451: VDS host = new VDS(); Line 452: host.setId(Guid.newGuid()); Line 453: Line 454: //no vds for network id. Line 455: when(vdsDaoMock.getAllForNetwork(any(Guid.class))).thenReturn(Collections.singletonList(host)); Please be explicit, set the networkId. Line 456: Line 457: assertThat(new NetworkAttachmentValidator(new NetworkAttachment(), host, managementNetworkUtil).networkNotAttachedToHost(), Line 458: failsWith(VdcBllMessages.NETWORK_ALREADY_ATTACHED_TO_HOST)); Line 459: } -- To view, visit https://gerrit.ovirt.org/36570 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4ba4327b93f8628e234e0e2b23234d8b89902b5 Gerrit-PatchSet: 40 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
