Martin Peřina has posted comments on this change. Change subject: engine: Add unit-tests for fence-proxy selection ......................................................................
Patch Set 2: (5 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) { > I disagree; There is no initialization involving the FencingPolicy which wo Well, I disagree here :-) The instance of FenceProxyLocator is created for exactly one host (VDS instance) which belongs to the exactly one cluster (with FencingPolicy instance), so these attributes should be considered final. IMHO it's much better to create new instance of FenceProxyLocator for each VDS and FencingPolicy than reuse FenceProxyLocator instance with different host/policy. And when you add getter/setter for such attributes, you allows to change those instances and this can potentionally lead to errors which are very hard to discover. 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; > this VDS is used everywhere Exactly, it's very confusing to have instance variable and method variable with the same name as you use it in findProxyHost* test methods. If you have instance variable fencedVds, then there's no confusion with vds variables in findProxyHost* methods Line 53: Line 54: @Mock Line 55: private DbFacade dbFacade; Line 56: Line 142: } Line 143: Line 144: @Test Line 145: public void findProxyHost_ChooseByDCWhenNoClusterMatch() { Line 146: when(vdsDao.getAll()).thenReturn(createHosts()).thenReturn(createHosts()); > It's called twice and the second time returned null until I added another . I don't follow here. When you define when(methodA).thenReturn(resultA) then you can call methodA multiple times and resultA will always be returned. Or am I missing something? 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() { > Disagree, I believe it makes the code more readable. In my eyes, ideally, a IMHO there's no need to extract code into createHosts() method, initialization is quite straightforward and this just complicates 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); > No, I'm almost sure the same cluster can't belong to two different data-cen So, please don't use same cluster ID for different clusters in different datacenters Line 163: vds.setStoragePoolId(FENCED_HOST_DATACENTER_ID); Line 164: vds.setVdsGroupCompatibilityVersion(Version.v3_5); Line 165: hosts.add(vds); Line 166: return hosts; -- 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
