Mike Kolesnik has posted comments on this change.
Change subject: engine: Refactored code into NetworkValidator class
......................................................................
Patch Set 6: (18 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
This is used for audit logging, I'm not sure you want to delete it.
Line 33
Line 34
Line 35
Line 36
Line 37
Same here...
....................................................
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;
Why save as field if it's only used inside the execute?
Line 20:
Line 21: public UpdateNetworkCommand(T parameters) {
Line 22: super(parameters);
Line 23: }
....................................................
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.
What do you mean "presumably"?
Either it can or can't be null..
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() {
I'd say "Exists" doesn't really match what you check logically, it's more like
"IsSet" or something since exists on the DC implies a check that it exists in
the system, as where this check doesn't really checks if it exists in the
system..
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() {
Seems to me like a check only interesting in "remove" scenario so perhaps
should sit there
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";
Why not save as type Guid?
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
It's not customary to write in-line comments
Besides, I think that the code speaks for itself so this comment is redundant
IMHO
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
Same here...
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) {
Usually "is" prefix is used for getters, not in variable names.
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);
You can use dc's compatibility version instead of null
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());
Why check the message and not the result itself?
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) {
Usually "is" prefix is used for getters, not in variable names.
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) {
Usually "is" prefix is used for getters, not in variable names.
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 {
Z should be capitalized
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 {
Same comment..
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));
Perhaps you can extract this code for creating network with name, since it
repeats in a few cases
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'm not sure it's a good idea to test protected methods, it you refer to the
tested class as a "black box" then you should test the individual public
methods.
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 <[email protected]>
Gerrit-Reviewer: Lior Vernia <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
