Alona Kaplan has posted comments on this change. Change subject: core: refactor&test for HostNetworkAttachmentsPersister ......................................................................
Patch Set 44: (4 comments) https://gerrit.ovirt.org/#/c/36067/44/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 221: NetworkAttachment attachmentBeingPersisted = networkAttachmentCaptor.getValue(); Line 222: assertThat(attachmentBeingPersisted.getNicId(), is(interfaceWithAttachedClusterNetworkA.getId())); Line 223: // new id will be generated for persisted record Line 224: assertThat(attachmentBeingPersisted.getId(), equalTo(userNetworkAttachment.getId())); Line 225: assertThat(attachmentBeingPersisted.getIpConfiguration(), is(userNetworkAttachment.getIpConfiguration())); Now you have lone 234- assertIpConfiguration, this line is redundant. Line 226: assertThat(attachmentBeingPersisted.getNetworkId(), is(userNetworkAttachment.getNetworkId())); Line 227: Line 228: Line 229: Map<String, String> propertiesBeingPersisted = attachmentBeingPersisted.getProperties(); Line 230: Map<String, String> interfaceCustomProperties = interfaceWithAttachedClusterNetworkA.getCustomProperties(); Line 231: Line 232: assertCustomProperties(propertiesBeingPersisted, createCustomProperties()); Line 233: Line 234: assertIpConfiguration(attachmentBeingPersisted.getIpConfiguration(), createIpConfiguration()); shouldn't it be? assertIpConfiguration(attachmentBeingPersisted.getIpConfiguration(), userNetworkAttachment.getIpConfiguration()); Line 235: Line 236: Line 237: // verify that nothing else happens, no removals, no creations. Line 238: verifyNoMoreInteractions(networkAttachmentDao); Line 239: } Line 240: Line 241: private IpConfiguration createIpConfiguration() { Line 242: IPv4Address address = new IPv4Address(); Line 243: address.setAddress("192.168.1.10"); You're setting the same values as you set in- 'setUp()-interfaceWithAttachedClusterNetworkA' Please extract the values to be final static. Line 244: address.setNetmask("255.255.255.0"); Line 245: address.setGateway("192.168.1.1"); Line 246: Line 247: IpConfiguration result = new IpConfiguration(); Line 335: } Line 336: Line 337: private void assertCustomProperties(Map<String, String> propertiesBeingPersisted, Line 338: Map<String, String> interfaceCustomProperties) { Line 339: assertThat(propertiesBeingPersisted.size(), is(2)); why is the '2' hard-coded? Please use- interfaceCustomProperties.size(). Line 340: for (Map.Entry<String, String> entry : interfaceCustomProperties.entrySet()) { Line 341: String key = entry.getKey(); Line 342: assertThat(propertiesBeingPersisted.containsKey(key), is(true)); Line 343: assertThat(propertiesBeingPersisted.get(key), is(interfaceCustomProperties.get(key))); -- 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: 44 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
