Moti Asayag has posted comments on this change. Change subject: engine: introduce HostNicVfsConfigHelper ......................................................................
Patch Set 1: Code-Review-1 (7 comments) http://gerrit.ovirt.org/#/c/37720/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostNicVfsConfigHelperImpl.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/HostNicVfsConfigHelperImpl.java: Line 22: private final InterfaceDao interfaceDao; Line 23: private final HostDeviceDao hostDeviceDao; Line 24: private final HostNicVfsConfigDao hostNicVfsConfigDao; Line 25: Line 26: @Inject couldn't you have an empty c'tor and instead passing the daos, simply intialize them in the c'tor by injecting on the the dbFacade ? meaning add: @Inject private DbFacade dbFacade; HostNicVfsConfigHelperImpl() { this.interfaceDao = dbFacade.getInterfaceDao(); ... } Line 27: HostNicVfsConfigHelperImpl(InterfaceDao interfaceDao, Line 28: HostDeviceDao hostDeviceDao, Line 29: HostNicVfsConfigDao hostNicVfsConfigDao) { Line 30: this.interfaceDao = interfaceDao; Line 153: public boolean areAllVfsFree(VdsNetworkInterface nic) { Line 154: List<HostDevice> deviceList = getDevicesByHostId(nic.getVdsId()); Line 155: HostDevice pciDevice = getPciDeviceByNic(nic, deviceList); Line 156: Line 157: if (!isSriovDevice(pciDevice)) { Findbugs/Coverity will honour you with NPE potential error here. since the deviceList might return as empty and there is a change that pciDevice will be null - invoking line 157 will end with NPE. Line 158: throw new UnsupportedOperationException(); Line 159: } Line 160: Line 161: List<HostDevice> vfs = getVfs(pciDevice, deviceList); Line 154: List<HostDevice> deviceList = getDevicesByHostId(nic.getVdsId()); Line 155: HostDevice pciDevice = getPciDeviceByNic(nic, deviceList); Line 156: Line 157: if (!isSriovDevice(pciDevice)) { Line 158: throw new UnsupportedOperationException(); no need to log this one ? or provide a description to the error ? or if this might brought up to the user intention, replace this with VdcBllException Line 159: } Line 160: Line 161: List<HostDevice> vfs = getVfs(pciDevice, deviceList); Line 162: http://gerrit.ovirt.org/#/c/37720/1/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/HostNicVfsConfigHelperImplTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/HostNicVfsConfigHelperImplTest.java: Line 28: Line 29: @RunWith(MockitoJUnitRunner.class) Line 30: public class HostNicVfsConfigHelperImplTest { Line 31: Line 32: private static final String NIC_NAME = "eth1"; the names can be randomize instead using static value. i.e. RandomUtils.getInstance().nextString(5) Line 33: private static final Guid NIC_ID = Guid.newGuid(); Line 34: private static final Guid HOST_ID = Guid.newGuid(); Line 35: private static final String NET_DEVICE_NAME = "net_device"; Line 36: private static final String PCI_DEVICE_NAME = "pci_device"; Line 60: private HostNicVfsConfigHelperImpl hostNicVfsConfigHelper; Line 61: Line 62: @Before Line 63: public void setUp() { Line 64: hostNicVfsConfigHelper = spy(new HostNicVfsConfigHelperImpl(interfaceDao, hostDeviceDao, hostNicVfsConfigDao)); spying the helper isn't required for all of the tests. It suppose to be required when you need to mock some specific behaviour and according to the test it should be performed with areAllVfsFreeCommon. Line 65: Line 66: when(netDevice.getHostId()).thenReturn(HOST_ID); Line 67: when(netDevice.getDeviceName()).thenReturn(NET_DEVICE_NAME); Line 68: when(netDevice.getNetworkInterfaceName()).thenReturn(NIC_NAME); Line 124: } Line 125: Line 126: @Test Line 127: public void isNetworkDevicePossitive() { Line 128: assertEquals(false, isNetworkDevice(pciDevice)); please remove isNetworkDevice and replace it with a direct call to the method (this method byitself doesn't add any value - both it and the helper method has the same name and no improvement for the readability). Line 129: } Line 130: Line 131: @Test Line 132: public void isNetworkDeviceNegtive() { Line 129: } Line 130: Line 131: @Test Line 132: public void isNetworkDeviceNegtive() { Line 133: assertEquals(true, isNetworkDevice(netDevice)); same Line 134: } Line 135: Line 136: @Test Line 137: public void updateHostNicVfsConfigWithNumVfsData() { -- To view, visit http://gerrit.ovirt.org/37720 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I588d619a296cb894f86cce6491337351206e21c2 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <[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
