Martin Peřina has posted comments on this change. Change subject: engine: Add unit-tests for fence-proxy selection ......................................................................
Patch Set 2: (15 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 after that it should not be changed (same as fenced host instance). 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: Please add comments for each test methods, it's not always clear what kind of test is executed in each method. Also please don't use underscore char in method name. Underscore chars in Java should be used only in upper case constant names. For example: public void findProxyHostExcludeSelf() Line 1: package org.ovirt.engine.core.bll; Line 2: Line 3: import java.util.LinkedList; Line 4: import java.util.List; 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 confused with VDS vds used in each test method Line 53: Line 54: @Mock Line 55: private DbFacade dbFacade; Line 56: Line 66: when(dbFacade.getVdsDao()).thenReturn(vdsDao); Line 67: DbFacadeLocator.setDbFacade(dbFacade); Line 68: } Line 69: Line 70: private void mockVds() { I don's see a reason for code splitting here, please move content of the method into setup(). Line 71: when(vds.getName()).thenReturn(HOST_NAME); Line 72: when(vds.getId()).thenReturn(FENCECD_HOST_ID); Line 73: when(vds.getVdsGroupId()).thenReturn(FENCED_HOST_CLUSTER_ID); Line 74: when(vds.getStoragePoolId()).thenReturn(FENCED_HOST_DATACENTER_ID); Line 91: public void findProxyHost_ExcludesSelf() I would also add positive test case (test this with another host in hosts list, so we know that selection works). And I would prefer public void findProxyHostExcludeSelf() Line 96: assertNull(proxyHost); Line 97: } Line 98: Line 99: @Test Line 100: public void findProxyHost_ExcludedHost() { Here I would also add test with another host. And I would prefer public void findProxyHostExcludeHost() Line 101: List<VDS> hosts = new LinkedList<>(); Line 102: VDS vds = new VDS(); Line 103: vds.setId(OTHER_HOST_ID_1); Line 104: vds.setVdsGroupId(FENCED_HOST_CLUSTER_ID); 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. And I would prefer public void findProxyHostExcludeUnreachable() 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 public void findProxyHostFromSameCluster() And I would add another method public void findProxyHostFromAnotherCluster() 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 public void findProxyHostFromAnotherDC() 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? 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 findProxyHost_ChooseByDCWhenNoClusterMatch() 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 point of view)? 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 public void findProxyHostSelectUpHost() 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 fencing policy instance is support by any version >= 3.0 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 supported by any version >= 3.0. You should add for example policy.setSkipFencingIfSDActive(true) in order to support only clusters >= 3.5 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 <ol...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
