Martin Mucha has posted comments on this change. Change subject: core: tests for NetworkAttachmentValidator ......................................................................
Patch Set 40: (9 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 d however if we look at generated error message — "…~cannot remove~…", this context cannot be changed. I.e. former method name allowed this method to be mistakenly used in broader context. You actually cannot use this method to test if network is management and if is complaint about it removal. That's why I added 'Removing' into method name. Ok, so lets agree, that both names are wrong. Proposals: • networkBeingRemovedIsNotManagementNetwork • removedNetworkIsNotManagementNetwork 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 Done 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, Done Line 59: when(DbFacade.getInstance().getNetworkClusterDao()).thenReturn(networkClusterDaoMock); Line 60: when(DbFacade.getInstance().getVdsDao()).thenReturn(vdsDaoMock); Line 61: } Line 62: 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. Done Line 116: } Line 117: Line 118: @Test Line 119: public void testNotRemovingManagementNetwork() throws Exception { 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? Done Line 149: Line 150: assertThat(new NetworkAttachmentValidator(attachment, host, managementNetworkUtil).networkAttachedToCluster(), isValid()); Line 151: } Line 152: 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 Done 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 Done Line 246: Network network = new Network(); Line 247: network.setId(Guid.newGuid()); Line 248: Line 249: NetworkAttachment attachment = new NetworkAttachment(); Line 439: } Line 440: Line 441: @Test Line 442: public void testNetworkNotAttachedToHost() throws Exception { Line 443: //no vds for network id. > space after // Done 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 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. Done 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)); -- 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
