Ori Liel has posted comments on this change. Change subject: core: Add unit-tests for fence-proxy selection ......................................................................
Patch Set 2: (10 comments) 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 Done 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; > Exactly, it's very confusing to have instance variable and method variable Done 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 me Disagree but I don't really care about this, so I've done it. 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 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. Done 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 123: assertNull(proxyHost); Line 124: } Line 125: Line 126: @Test Line 127: public void findProxyHost_ChooseTheSupportedCluster() { > Done That's what the next test does. This test shows that it preferes a host from the same cluster, the next test shows that if there is no host from the same cluster, it will choose a host from the same datacenter. 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 142: } Line 143: Line 144: @Test Line 145: public void findProxyHost_ChooseByDCWhenNoClusterMatch() { Line 146: when(vdsDao.getAll()).thenReturn(createHosts()).thenReturn(createHosts()); > I don't follow here. When you define when(methodA).thenReturn(resultA) then I don't know why, but without this, the second time the hosts are retrieved the list is empty. It must be something about the way I use the testing infrastructure. So I agree the code is not very elegant this way, but the test works... 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() { > IMHO there's no need to extract code into createHosts() method, initializat I looked at it again and I actually agree with you on this instance. But as I explained in the previous comment, I need to re-create the hosts in order for the test to work, probably because I'm not using the infrastructure 100% right. The test will fail if I'll reuse the list of hosts instead of recreating it. I don't want to spend more time on this since the test does work, it's just not very elegant. I can live with small 'violations' like this in the testing code. 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); > So, please don't use same cluster ID for different clusters in different da Done 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 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 The condition checked for is: vds.getSupportedClusterVersionsSet().contains(minimalSupportedVersion); where vds.getsupported_cluster_levels() is a list of all cluster-versions supported by this host, and 'minimalSupportedVersion' is the minimal cluster-version which supports fencing-policy. This latter value is received by: FencingPolicyHelper.getMinimalSupportedVersion(fencingPolicy), which returns by default 3.0, and can't be mocked since it's static. I can make it return other values by setting policy.setSkipFencingIfSDActive(true), but then more code inside FencingPolicyHelper, which is not the subject of this test, will run, including a DB call to get a configuration value, which would have to be mocked. By simply adding 3.0 to the host's supported cluster levels, I make sure there's a match between vds.getSupportedClusterVersionsSet() and minimalSupportedVersion, simulating a legal situation. I believe that this is a good way to test this, considering the difficulty in mocking. 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 See my reply to your previous comment. This time vds.getsupported_cluster_levels() will return 3.1, and minimalSupportedVersion would be 3.0, simulating a situation where there is no match between the versions and the validation does not pass. I believe this is a good way to test this, refraining from unnecessary mocking and minimizing access to code from other classes, which are not the subject of this test. 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
