Martin Mucha has posted comments on this change. Change subject: core: refactor&test for HostNetworkAttachmentsPersister ......................................................................
Patch Set 37: (7 comments) https://gerrit.ovirt.org/#/c/36067/37/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersister.java: Line 97: return containsNetwork(existingNetworkAttachments, network); Line 98: } Line 99: Line 100: private boolean containsNetwork(List<NetworkAttachment> networkAttachments, Network network) { Line 101: for (NetworkAttachment networkAttachment : networkAttachments) { > You can use -Linq.firstOrNull to find out whether the list contains network used. (I understand that people get excited about lambdas, but until we move to JDK8, this Linq stuff does not make code better. Only equally verbose with more complex syntax. Especially true in this method). But I've changed it upon request. Done. Line 102: if (network.getId().equals(networkAttachment.getNetworkId())) { Line 103: return true; Line 104: } Line 105: } Line 109: Line 110: /** Line 111: * Saves or updates network attachments which were provided by the user and were reported by the host Line 112: * @return newly create network attachments. Line 113: * @param validNetworkAttachments > Shouldn't the order be opposite? First the param and then return value. empty param removed.Done. Line 114: */ Line 115: private List<NetworkAttachment> saveUserNetworkAttachments(List<NetworkAttachment> validNetworkAttachments) { Line 116: List<NetworkAttachment> newlyCreatedNetworkAttachments = new ArrayList<>(); Line 117: Line 136: Line 137: return newlyCreatedNetworkAttachments; Line 138: } Line 139: Line 140: private void verifyUserAttachmentsDoesNotReferenceSameNetworkDuplicately(List<NetworkAttachment> userNetworkAttachments) { > This should have been verified in the candoaction of the relevant commands. Done Line 141: Set<Guid> networkIds = new HashSet<>(userNetworkAttachments.size()); Line 142: for (NetworkAttachment networkAttachment : userNetworkAttachments) { Line 143: networkIds.add(networkAttachment.getNetworkId()); Line 144: } https://gerrit.ovirt.org/#/c/36067/37/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersisterTest.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HostNetworkAttachmentsPersisterTest.java: Line 149: verifyNoMoreInteractions(networkAttachmentDao); Line 150: } Line 151: Line 152: @Test Line 153: public void testPersistNetworkAttachmentsWhenCalledWithNewUserAttachments() throws Exception { > The method looks very similar to the previous one. Please create a common m I don't know how to do it without creating very hard to read method with gazillion parameters. Line 154: Mockito.when(networkAttachmentDao.getAllForHost(eq(hostId))).thenReturn(new ArrayList<NetworkAttachment>()); Line 155: Line 156: NetworkAttachment userNetworkAttachment = createNetworkAttachment(clusterNetworkA); Line 157: Line 201: assertThat(networkAttachmentCaptor.getValue().getNicId(), is(interfaceWithAttachedClusterNetworkA.getId())); Line 202: //new id will be generated for persisted record Line 203: assertThat(networkAttachmentCaptor.getValue().getId(), equalTo(useProvidedNetworkAttachmentId)); Line 204: assertThat(networkAttachmentCaptor.getValue().getIpConfiguration(), is(userNetworkAttachment.getIpConfiguration())); Line 205: assertThat(networkAttachmentCaptor.getValue().getNetworkId(), is(userNetworkAttachment.getNetworkId())); > What about properties? Done Line 206: Line 207: Line 208: //verify that nothing else happens, no removals, no creations. Line 209: verifyNoMoreInteractions(networkAttachmentDao); Line 258: Line 259: ArgumentCaptor<NetworkAttachment> networkAttachmentCaptor = ArgumentCaptor.forClass(NetworkAttachment.class); Line 260: verify(networkAttachmentDao).save(networkAttachmentCaptor.capture()); Line 261: Line 262: assertThat(networkAttachmentCaptor.getValue().getNetworkId(), is(clusterNetworkA.getId())); > Please move the assertions comparison to the captor to a separate method. I I don't know how to do it without creating very hard to read method with gazillion parameters. There three usages at first seems very similar, but captor is created in different places, and assertions are not that equal as well. This has some duplicates, but some complexness-free readability as well. Line 263: assertThat(networkAttachmentCaptor.getValue().getNicId(), is(interfaceWithAttachedClusterNetworkA.getId())); Line 264: assertThat(networkAttachmentCaptor.getValue().getId(), notNullValue()); Line 265: //TODO MM: verify also properties? do we want to be so specific? Line 266: Line 261: Line 262: assertThat(networkAttachmentCaptor.getValue().getNetworkId(), is(clusterNetworkA.getId())); Line 263: assertThat(networkAttachmentCaptor.getValue().getNicId(), is(interfaceWithAttachedClusterNetworkA.getId())); Line 264: assertThat(networkAttachmentCaptor.getValue().getId(), notNullValue()); Line 265: //TODO MM: verify also properties? do we want to be so specific? > what about properties and ipConfiguration? Done Line 266: Line 267: //verify that nothing else happens, namely, interfaceWithoutAttachedNetwork will not trigger persisting any data. Line 268: verifyNoMoreInteractions(networkAttachmentDao); Line 269: } -- To view, visit https://gerrit.ovirt.org/36067 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib39c3678a29a7bd8728d32b55490f8500a77d9d6 Gerrit-PatchSet: 37 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
