Ori Liel has posted comments on this change. Change subject: engine: Add unit-tests for fence-proxy selection ......................................................................
Patch Set 2: (12 comments) http://gerrit.ovirt.org/#/c/36419/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceProxyLocator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceProxyLocator.java: Line 181: public FencingPolicy getFencingPolicy() { Line 182: return fencingPolicy; Line 183: } Line 184: Line 185: public void setFencingPolicy(FencingPolicy fencingPolicy) { > Please remove this method. Fencing policy should be set in constructor and I disagree; There is no initialization involving the FencingPolicy which would prevent re-setting it, and no general object-oriented-programming principal is violated by providing a setter. The setter simplifies mocking in unit-tests. Usually design changes brought about by test needs are in line with good coding practice. Having said that, if this point is important for you I'll remove the setter and adapt the tests. This is hardly some major point worth arguing about :) Line 186: this.fencingPolicy = fencingPolicy; Line 187: } http://gerrit.ovirt.org/#/c/36419/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/FenceProxyLocatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/FenceProxyLocatorTest.java: Line 48: MockConfigRule.mockConfig(ConfigValues.FenceProxyDefaultPreferences, "cluster,dc,other_dc"), Line 49: MockConfigRule.mockConfig(ConfigValues.VdsFenceOptionTypes, "secure=bool,port=int,slot=int")); Line 50: Line 51: @Mock Line 52: private VDS vds; > Please rename this to fencedVds or nonrespondingVds so it cannot be confuse this VDS is used everywhere Line 53: Line 54: @Mock Line 55: private DbFacade dbFacade; Line 56: Line 87: assertNotNull(proxyHost); Line 88: } Line 89: Line 90: @Test Line 91: public void findProxyHost_ExcludesSelf() { > I would also add positive test case (test this with another host in hosts l Done Line 92: List<VDS> hosts = new LinkedList<>(); Line 93: hosts.add(vds); Line 94: when(vdsDao.getAll()).thenReturn(hosts); Line 95: VDS proxyHost = fenceProxyLocator.findProxyHost(); Line 108: assertNull(proxyHost); Line 109: } Line 110: Line 111: @Test Line 112: public void findProxyHost_ExcludesUnreachable() { > Here I would also add test with another host. Done Line 113: List<VDS> hosts = new LinkedList<>(); Line 114: VDS vds = new VDS(); Line 115: vds.setId(OTHER_HOST_ID_2); Line 116: vds.setStatus(VDSStatus.NonResponsive); Line 123: assertNull(proxyHost); Line 124: } Line 125: Line 126: @Test Line 127: public void findProxyHost_ChooseTheSupportedCluster() { > I would prefer Done Line 128: List<VDS> hosts = new LinkedList<>(); Line 129: VDS vds = new VDS(); Line 130: vds.setId(OTHER_HOST_ID_1); Line 131: vds.setVdsGroupId(OTHER_CLUSTER_ID); Line 141: assertEquals(proxyHost.getId(), OTHER_HOST_ID_2); Line 142: } Line 143: Line 144: @Test Line 145: public void findProxyHost_ChooseByDCWhenNoClusterMatch() { > I would prefer Done Line 146: when(vdsDao.getAll()).thenReturn(createHosts()).thenReturn(createHosts()); Line 147: VDS proxyHost = fenceProxyLocator.findProxyHost(false); Line 148: assertNotNull(proxyHost); Line 149: assertEquals(proxyHost.getId(), OTHER_HOST_ID_2); Line 142: } Line 143: Line 144: @Test Line 145: public void findProxyHost_ChooseByDCWhenNoClusterMatch() { Line 146: when(vdsDao.getAll()).thenReturn(createHosts()).thenReturn(createHosts()); > Shouldn't be here only one thenReturn() call? It's called twice and the second time returned null until I added another .thenReturn(createHosts() Line 147: VDS proxyHost = fenceProxyLocator.findProxyHost(false); Line 148: assertNotNull(proxyHost); Line 149: assertEquals(proxyHost.getId(), OTHER_HOST_ID_2); Line 150: } Line 148: assertNotNull(proxyHost); Line 149: assertEquals(proxyHost.getId(), OTHER_HOST_ID_2); Line 150: } Line 151: Line 152: private List<VDS> createHosts() { > I don's see a reason for this method, please move it's content into findPro Disagree, I believe it makes the code more readable. In my eyes, ideally, any bunch of logic that can be given a meaningful name should be extracted to a method. That way the code tells a story: 1) didA() 2) didB() 3) didC() Any method which is longer than 10 lines should be regarded with suspicion and can probably be written better :) Line 153: List<VDS> hosts = new LinkedList<>(); Line 154: VDS vds = new VDS(); Line 155: vds.setId(OTHER_HOST_ID_1); Line 156: vds.setVdsGroupId(OTHER_CLUSTER_ID); Line 158: vds.setVdsGroupCompatibilityVersion(Version.v3_5); Line 159: hosts.add(vds); Line 160: vds = new VDS(); Line 161: vds.setId(OTHER_HOST_ID_2); Line 162: vds.setVdsGroupId(OTHER_CLUSTER_ID); > Do we allow same cluster UUID in different datacenters (I mean from DB poin No, I'm almost sure the same cluster can't belong to two different data-centers at the same time. Line 163: vds.setStoragePoolId(FENCED_HOST_DATACENTER_ID); Line 164: vds.setVdsGroupCompatibilityVersion(Version.v3_5); Line 165: hosts.add(vds); Line 166: return hosts; Line 166: return hosts; Line 167: } Line 168: Line 169: @Test Line 170: public void findProxyHost_PreferUpHost() { > I would prefer Done Line 171: List<VDS> hosts = new LinkedList<>(); Line 172: VDS vds = new VDS(); Line 173: vds.setId(OTHER_HOST_ID_1); Line 174: vds.setVdsGroupId(FENCED_HOST_CLUSTER_ID); Line 191: public void findProxyHost_FencingPolicySupported() { Line 192: FencingPolicy policy = new FencingPolicy(); Line 193: fenceProxyLocator.setFencingPolicy(policy); Line 194: VDS vds = new VDS(); Line 195: vds.setSupportedClusterLevels(Version.v3_0.toString()); > You don't need to lower supported cluster level version, because by default I'll revisit this Line 196: vds.setId(OTHER_HOST_ID_2); Line 197: vds.setVdsGroupId(FENCED_HOST_CLUSTER_ID); Line 198: vds.setVdsGroupCompatibilityVersion(Version.v3_5); Line 199: List<VDS> hosts = new LinkedList<>(); Line 204: } Line 205: Line 206: @Test Line 207: public void findProxyHost_FencingPolicyNotSupported() { Line 208: FencingPolicy policy = new FencingPolicy(); > This test cannot work, because by default fencing policy instance is suppor I'll revisit this Line 209: fenceProxyLocator.setFencingPolicy(policy); Line 210: VDS vds = new VDS(); Line 211: vds.setSupportedClusterLevels(Version.v3_1.toString()); Line 212: vds.setId(OTHER_HOST_ID_2); -- To view, visit http://gerrit.ovirt.org/36419 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95f00b38c78ef7b6a72ee141d9090bf5e60eb679 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ori Liel <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
