Lior Vernia has posted comments on this change.
Change subject: engine: Refactored code into NetworkValidator class
......................................................................
Patch Set 6: (19 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/NetworkCommon.java
Line 26
Line 27
Line 28
Line 29
Line 30
Done
Line 33
Line 34
Line 35
Line 36
Line 37
Done
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java
Line 15: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 16:
Line 17: @SuppressWarnings("serial")
Line 18: public class UpdateNetworkCommand<T extends
AddNetworkStoragePoolParameters> extends NetworkCommon<T> {
Line 19: private List<NetworkCluster> clusterAttachments;
Fixed. Why was the call for it ever cached as in getClusterAttachments() when
it was only to be used once?
Line 20:
Line 21: public UpdateNetworkCommand(T parameters) {
Line 22: super(parameters);
Line 23: }
Line 75: return clusterAttachments;
Line 76: }
Line 77:
Line 78: private Network getOldNetwork() {
Line 79: Network newNetwork = getNetwork();
Done
Line 80: List<Network> networks =
getNetworkDAO().getAllForDataCenter(newNetwork.getDataCenterId());
Line 81:
Line 82: for (Network network : networks) {
Line 83: if (newNetwork.getId().equals(network.getId())) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkValidator.java
Line 41: return dataCenter;
Line 42: }
Line 43:
Line 44: /**
Line 45: * @return All existing networks in same data center - presumably
doesn't return null.
Done
Line 46: */
Line 47: protected List<Network> getNetworks() {
Line 48: if (networks == null) {
Line 49: networks =
getDbFacade().getNetworkDao().getAllForDataCenter(network.getDataCenterId());
Line 116:
Line 117: /**
Line 118: * @return An error iff the network doesn't exist.
Line 119: */
Line 120: public ValidationResult networkExists() {
Done
Line 121: return network == null
Line 122: ? new
ValidationResult(VdcBllMessages.NETWORK_NOT_EXISTS)
Line 123: : ValidationResult.VALID;
Line 124: }
Line 138:
Line 139: /**
Line 140: * @return An error iff the network is in fact the data center's
management network.
Line 141: */
Line 142: public ValidationResult notManagementNetwork() {
Done, although it makes sense to me that this functionality might be useful
elsewhere.
Line 143: String managementNetwork = Config.<String>
GetValue(ConfigValues.ManagementNetwork);
Line 144: return managementNetwork.equalsIgnoreCase(network.getName())
Line 145: ? new
ValidationResult(VdcBllMessages.NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK)
Line 146: : ValidationResult.VALID;
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/NetworkValidatorTest.java
Line 31:
Line 32: protected final String DEFAULT_NETWORK_NAME = "mynetwork";
Line 33: protected final String OTHER_NETWORK_NAME = "myothernetwork";
Line 34: protected final String MGMT_NETWORK_NAME = "ovirtmgmt";
Line 35: protected final String DEFAULT_GUID =
"00000000-0000-0000-0000-000000000000";
Done
Line 36: protected final String OTHER_GUID =
"00000000-0000-0000-0000-000000000001";
Line 37: protected final int DEFAULT_VLAN_ID = 0;
Line 38: protected final int OTHER_VLAN_ID = 1;
Line 39:
Line 70: }
Line 71:
Line 72: @Test
Line 73: public void networkNull() throws Exception {
Line 74: validator = new NetworkValidator(null); // replace default
validator with one validating null network
Done
Line 75: assertEquals(new
ValidationResult(VdcBllMessages.NETWORK_NOT_EXISTS), validator.networkExists());
Line 76: }
Line 77:
Line 78: @Test
Line 76: }
Line 77:
Line 78: @Test
Line 79: public void dataCenterDoesntExist() throws Exception {
Line 80: doReturn(null).when(validator).getDataCenter(); // replace
default mock data center with null data center
Done
Line 81: assertEquals(new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST),
Line 82: validator.dataCenterExists());
Line 83: }
Line 84:
Line 86: public void dataCenterExists() throws Exception {
Line 87: assertEquals(ValidationResult.VALID,
validator.dataCenterExists());
Line 88: }
Line 89:
Line 90: private void vmNetworkSetupTest(ValidationResult expected, boolean
isVmNetwork, boolean isFeatureSupported) {
Fixed it, but what's the semantic difference between the evaluation of:
* boolean isSomething()
* boolean isSomething
that would warrant different naming conventions?
Line 91:
mockConfigRule.mockConfigValue(ConfigValues.NonVmNetworkSupported, null,
isFeatureSupported);
Line 92: when(network.isVmNetwork()).thenReturn(isVmNetwork);
Line 93:
Line 94: assertEquals(expected.getMessage(),
validator.vmNetworkSetCorrectly().getMessage());
Line 87: assertEquals(ValidationResult.VALID,
validator.dataCenterExists());
Line 88: }
Line 89:
Line 90: private void vmNetworkSetupTest(ValidationResult expected, boolean
isVmNetwork, boolean isFeatureSupported) {
Line 91:
mockConfigRule.mockConfigValue(ConfigValues.NonVmNetworkSupported, null,
isFeatureSupported);
Done
Line 92: when(network.isVmNetwork()).thenReturn(isVmNetwork);
Line 93:
Line 94: assertEquals(expected.getMessage(),
validator.vmNetworkSetCorrectly().getMessage());
Line 95: }
Line 90: private void vmNetworkSetupTest(ValidationResult expected, boolean
isVmNetwork, boolean isFeatureSupported) {
Line 91:
mockConfigRule.mockConfigValue(ConfigValues.NonVmNetworkSupported, null,
isFeatureSupported);
Line 92: when(network.isVmNetwork()).thenReturn(isVmNetwork);
Line 93:
Line 94: assertEquals(expected.getMessage(),
validator.vmNetworkSetCorrectly().getMessage());
Done
Line 95: }
Line 96:
Line 97: @Test
Line 98: public void vmNetworkWhenSupported() throws Exception {
Line 115: false,
Line 116: false);
Line 117: }
Line 118:
Line 119: private void stpTest(ValidationResult expected, boolean
isVmNetwork, boolean isStp) {
Done
Line 120: when(network.isVmNetwork()).thenReturn(isVmNetwork);
Line 121: when(network.getStp()).thenReturn(isStp);
Line 122:
Line 123: assertEquals(expected, validator.stpForVmNetworkOnly());
Line 142: public void noStpWhenNonVmNetwork() throws Exception {
Line 143: stpTest(ValidationResult.VALID, false, false);
Line 144: }
Line 145:
Line 146: private void mtuValidTest(ValidationResult expected, int mtu,
boolean isFeatureSupported) {
Done
Line 147:
mockConfigRule.mockConfigValue(ConfigValues.MTUOverrideSupported, null,
isFeatureSupported);
Line 148: when(network.getMtu()).thenReturn(mtu);
Line 149:
Line 150: assertEquals(expected, validator.mtuValid());
Line 150: assertEquals(expected, validator.mtuValid());
Line 151: }
Line 152:
Line 153: @Test
Line 154: public void nonzeroMtuWhenSupported() throws Exception {
Done
Line 155: mtuValidTest(ValidationResult.VALID, 1, true);
Line 156: }
Line 157:
Line 158: @Test
Line 155: mtuValidTest(ValidationResult.VALID, 1, true);
Line 156: }
Line 157:
Line 158: @Test
Line 159: public void nonzeroMtuWhenNotSupported() throws Exception {
Done
Line 160: mtuValidTest(new
ValidationResult(VdcBllMessages.NETWORK_MTU_OVERRIDE_NOT_SUPPORTED), 1, false);
Line 161: }
Line 162:
Line 163: @Test
Line 294: @Test
Line 295: public void networkNameTakenBySameNetwork() throws Exception {
Line 296: Network otherNetwork = new Network();
Line 297: otherNetwork.setName(DEFAULT_NETWORK_NAME);
Line 298: otherNetwork.setId(new Guid(DEFAULT_GUID));
Done
Line 299:
Line 300: networkNameAvailableTest(ValidationResult.VALID,
Collections.singletonList(otherNetwork));
Line 301: }
Line 302:
Line 299:
Line 300: networkNameAvailableTest(ValidationResult.VALID,
Collections.singletonList(otherNetwork));
Line 301: }
Line 302:
Line 303: private void networkNotUsedTest(ValidationResult expected,
List<Nameable> nameables) {
I understand the reasoning. But knowing that the three functions use the same
underlying function, why triple our number of tests when they do the same
thing? Just in case somebody changes the implementation in the future? Other
than that, there's no reason. Also, that would put me in a testing hole with
mocking the DbFacade. Haven't fixed, awaiting further instructions.
Line 304: assertEquals(expected, validator.networkNotUsed(nameables,
VdcBllMessages.VAR__ENTITIES__VMS));
Line 305: }
Line 306:
Line 307: @Test
--
To view, visit http://gerrit.ovirt.org/10940
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf40d81f0481c6b6dec141a71363888dc9e9a941
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Livnat Peer <lp...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
