Martin Mucha has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 44: (7 comments) http://gerrit.ovirt.org/#/c/26799/44/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolPerDcTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/MacPoolPerDcTest.java: Line 32: import org.ovirt.engine.core.dao.StoragePoolDAO; Line 33: import org.ovirt.engine.core.dao.network.VmNicDao; Line 34: Line 35: @RunWith(MockitoJUnitRunner.class) Line 36: public class MacPoolPerDcTest { > You can extract a field for the entire class (I see it in every test): Done Line 37: @Rule Line 38: public ExpectedException expectedException = ExpectedException.none(); Line 39: Line 40: @Rule Line 69: when(dbFacade.getVmNicDao()).thenReturn(vmNicDao); Line 70: when(dbFacade.getMacPoolDao()).thenReturn(macPoolDao); Line 71: Line 72: macFrom = "00:1a:4a:15:c0:00"; Line 73: macTo = "00:1a:4a:15:c0:ff"; > These two seem to me like constants so should be converted to constants.. Done Line 74: macPool = createMacPool(macFrom, macTo); Line 75: dataCenter = createStoragePool(macPool); Line 76: vmNic = createVmNic(); Line 77: } Line 145: Line 146: @Test Line 147: public void testModifyOfExistingMacPool() throws Exception { Line 148: final MacPoolPerDc pool = new MacPoolPerDc(); Line 149: MacPool macPool = createMacPool("00:00:00:00:00:01", "00:00:00:00:00:01"); > "00:00:00:00:00:01" should be extracted to a local constant. Done Line 150: StoragePool dataCenter = createStoragePool(macPool); Line 151: Line 152: when(dbFacade.getAuditLogDao()).thenReturn(auditLogDao); Line 153: Line 146: @Test Line 147: public void testModifyOfExistingMacPool() throws Exception { Line 148: final MacPoolPerDc pool = new MacPoolPerDc(); Line 149: MacPool macPool = createMacPool("00:00:00:00:00:01", "00:00:00:00:00:01"); Line 150: StoragePool dataCenter = createStoragePool(macPool); > You could override the fields that you already have, but it's not critical I don't want to explain why, but that's wrong. If I don't have to do it, lets leave it as it is. Line 151: Line 152: when(dbFacade.getAuditLogDao()).thenReturn(auditLogDao); Line 153: Line 154: mockStoragePool(dataCenter); Line 148: final MacPoolPerDc pool = new MacPoolPerDc(); Line 149: MacPool macPool = createMacPool("00:00:00:00:00:01", "00:00:00:00:00:01"); Line 150: StoragePool dataCenter = createStoragePool(macPool); Line 151: Line 152: when(dbFacade.getAuditLogDao()).thenReturn(auditLogDao); > This is constant behavior so it can be done in setUp method constant, but not common to all tests. But done. Line 153: Line 154: mockStoragePool(dataCenter); Line 155: mockGettingAllMacPools(macPool); Line 156: pool.initialize(); Line 157: Line 158: assertThat(pool.poolForDataCenter(dataCenter.getId()).addMac(macFrom), is(true)); Line 159: assertThat(pool.poolForDataCenter(dataCenter.getId()).addMac(macFrom), is(false)); Line 160: Line 161: pool.poolForDataCenter(dataCenter.getId()).allocateNewMac(); > You should check that you get the expected value here. I thought that your valid request to change test so it uses one mac range was motivated by proper argument, that we shouldn't be dependent on some obscure coincidence, i.e. that method allocateNewMacs allocates from left bound. That would be real improvement. Now I don't understand why you requested this change. Test added. Line 162: try { Line 163: pool.poolForDataCenter(dataCenter.getId()).allocateNewMac(); Line 164: Assert.fail("this allocation should not succeed."); Line 165: } catch (VdcBLLException e) { Line 174: macPool.setRanges(Collections.singletonList(macRange)); Line 175: pool.modifyPool(macPool); Line 176: Line 177: assertThat(pool.poolForDataCenter(dataCenter.getId()).addMac(macFrom), is(true)); Line 178: assertThat(pool.poolForDataCenter(dataCenter.getId()).allocateNewMac(), notNullValue()); > You should check that it's the second MAC, as the first one was already all Done Line 179: } Line 180: Line 181: @Test Line 182: public void testModifyOfNotExistingMacPool() throws Exception { -- To view, visit http://gerrit.ovirt.org/26799 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If522a0b0d5f810281bed010b46b5242f7dbdcc29 Gerrit-PatchSet: 44 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
