Martin Mucha has posted comments on this change. Change subject: core: pool per DC ......................................................................
Patch Set 42: (6 comments) http://gerrit.ovirt.org/#/c/26799/42/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 44: private DbFacade dbFacadeMock; Line 45: Line 46: @Before Line 47: public void setUp() throws Exception { Line 48: dbFacadeMock = mock(DbFacade.class); > You should use @Mock annotation for all these mocks and achieve cleaner cod Done Line 49: storagePoolDaoMock = mock(StoragePoolDAO.class); Line 50: vmNicDaoMock = mock(VmNicDao.class); Line 51: macPoolDaoMock = mock(MacPoolDao.class); Line 52: Line 67: pool.initialize(); Line 68: Mockito.verify(dbFacadeMock).getMacPoolDao(); Line 69: Mockito.verify(macPoolDaoMock).getAll(); Line 70: Line 71: Mockito.verifyNoMoreInteractions(storagePoolDaoMock, vmNicDaoMock, macPoolDaoMock, dbFacadeMock); > I believe that this should come after the second call, not before.. shit, you're right. Using it for first time. Sorry. Line 72: pool.initialize(); Line 73: //should not fail. Line 74: } Line 75: Line 87: final MacPoolPerDc pool = new MacPoolPerDc(); Line 88: mockStoragePools(storagePool); Line 89: mockGettingAllMacPools(macPool); Line 90: pool.initialize(); Line 91: assertThat(pool.getMacPoolId(storagePool.getId()), is(notNullValue())); > You shouldn't be testing this method since it's not public. testing non-public methods is perfectly normal, you won't be able to find any respectable source recommending not doing so. It will be quite easy to find sources recommending testing package local methods as well. But we can say it differently, that we don't want this extra checking, thus this method needs not to be accessible and can be private. And private methods are the ones (the only ones) you shouldn't be testing. Done Line 92: assertThat(pool.poolForDataCenter(storagePool.getId()), is(notNullValue())); Line 93: } Line 94: Line 95: @Test Line 121: @Test Line 122: public void testCreatePoolWhichExists() throws Exception { Line 123: final MacPoolPerDc pool = new MacPoolPerDc(); Line 124: Line 125: mockGettingAllMacPools(macPool); > No need for this.. maybe I'm not following you. if I remove this, method 'initialize' wouldn't initialize macPool and last line creating it would be creating it for the first and not second time. Line 126: pool.initialize(); Line 127: Line 128: expectedException.expect(IllegalStateException.class); Line 129: expectedException.expectMessage(MacPoolPerDc.UNABLE_TO_CREATE_MAC_POOL_IT_ALREADY_EXIST); Line 219: vmNic.setMacAddress("00:1a:4a:15:c0:fe"); Line 220: return vmNic; Line 221: } Line 222: Line 223: protected void mockStoragePools(StoragePool storagePool) { > s/mockStoragePools/mockStoragePool/ Done Line 224: //mock obtaining storagepool by id. Line 225: when(storagePoolDaoMock.get(eq(storagePool.getId()))).thenReturn(storagePool); Line 226: } Line 227: Line 220: return vmNic; Line 221: } Line 222: Line 223: protected void mockStoragePools(StoragePool storagePool) { Line 224: //mock obtaining storagepool by id. > This comment is unnecessary (code is self explanatory) Done Line 225: when(storagePoolDaoMock.get(eq(storagePool.getId()))).thenReturn(storagePool); Line 226: } Line 227: Line 228: private void expectNotInitializedException() { -- 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: 42 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
