Lior Vernia has posted comments on this change. Change subject: webadmin: allowing prefix as mask staticIP conf ......................................................................
Patch Set 12: (6 comments) http://gerrit.ovirt.org/#/c/35145/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidation.java: Line 21: return isPrefixAllowed ? getBadPrefixFormatMessage() : getBadNetmaskFormatMessage(); Line 22: Line 23: } Line 24: Line 25: protected String getBadPrefixFormatMessage() { Naming isn't great, as this is the error for bad format of either prefix or netmask. Maybe getBadPrefixOrNetmaskFormatMessage()? Line 26: return ConstantsManager.getInstance() Line 27: .getConstants() Line 28: .thisFieldMustContainValidPrefix(); Line 29: } Line 24: Line 25: protected String getBadPrefixFormatMessage() { Line 26: return ConstantsManager.getInstance() Line 27: .getConstants() Line 28: .thisFieldMustContainValidPrefix(); Same comment. Line 29: } Line 30: Line 31: protected String getBadNetmaskFormatMessage() { Line 32: return ConstantsManager.getInstance() Line 42: public ValidationResult validate(Object value) { Line 43: assert value == null || value instanceof String : "This validation must be run on a String!";//$NON-NLS-1$ Line 44: String mask = (String) value; Line 45: Line 46: if (isPrefixAllowed) { This flow can be simplified: if (isValidNetmaskFormat()) { if (!isNetmaskValid) { return failWith(getInvalidMask()); } } else { if (isPrefixAllowed) { if (!isPrefixValid()) { failWith(getBadPrefixFormatMessage()); } else { failWith(getBadNetmaskFormatMessage()); } } } This way you don't repeat any method call. Line 47: if (!(MaskValidator.getInstance().isPrefixValid(mask) || (MaskValidator.getInstance() Line 48: .isValidNetmaskFormat(mask) && MaskValidator.getInstance().isNetmaskValid( Line 49: mask)))) { Line 50: return failWith(getBadPrefixFormatMessage()); http://gerrit.ovirt.org/#/c/35145/12/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidationTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidationTest.java: Line 18: Line 19: private SubnetMaskValidation validation; Line 20: Line 21: private final String mask; Line 22: private final boolean isMaskValidFormat; When testing SubnetMaskValidation, you shouldn't differentiate between different forms of failure, because SubnetMaskValidation doesn't. When SubnetMaskValidation returns false, you don't know what the cause was. The conditioned flow you perform in setupErrorMessage() is wrong in essence, because you make decisions according to expected result and not actual result. If you do want to check that the right error is returned for each case, the proper way to do that is write separate test methods, where MaskValidator is mocked in each one to return different values from its different methods. Then make sure that for each permutation of return values, the right error message is returned. But it's important to understand that this isn't related to any specific input string test - it's unrelated, as in such a test MaskValidator would be mocked and therefore wouldn't really evaluate any input string (its proper evaluation is already tested in its own test class). Line 23: private final boolean isMaskValid; Line 24: private final boolean isPrefixAllowed; Line 25: Line 26: public SubnetMaskValidationTest(String mask, boolean isMaskValidFormat, boolean isMaskValid, boolean isPrefixAllowed) { Line 43: public void checkValidMask() Line 44: { Line 45: assertEquals("Failed to validate mask: " + mask, isMaskValid, validation.validate(mask).getSuccess());//$NON-NLS-1$ Line 46: if (!isMaskValid) { Line 47: setupErrorMessage(); See above comment, error mapping validation should be a different test. Line 48: } Line 49: Line 50: } Line 51: http://gerrit.ovirt.org/#/c/35145/12/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 2558: Line 2559: @DefaultStringValue("IP") Line 2560: String ipHostPopup(); Line 2561: Line 2562: @DefaultStringValue("Mask\\Prefix") 1. Mask and prefix are too general terms. 2. Usually a slash (not backwards slash) is used for selection. I would use "Subnet Mask / Routing Prefix". If this takes up too much space, maybe "Netmask / Routing Prefix". Line 2563: String subnetMaskHostPopup(); Line 2564: Line 2565: @DefaultStringValue("Gateway") Line 2566: String gwHostPopup(); -- To view, visit http://gerrit.ovirt.org/35145 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If909eda53a61bda5030c342156a01e53576e77cb Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[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
