Eliraz Levi has posted comments on this change. Change subject: webadmin: allowing prefix as mask staticIP conf ......................................................................
Patch Set 13: (11 comments) http://gerrit.ovirt.org/#/c/35145/13/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 52: if (!getMaskValidator().isPrefixValid(mask)) { Line 53: return failWith(getBadPrefixOrNetmaskFormatMessage()); Line 54: } Line 55: } Line 56: else { > Style conventions request that you put the else one row up, after the '}' c Done Line 57: return failWith(getBadNetmaskFormatMessage()); Line 58: } Line 59: Line 60: } http://gerrit.ovirt.org/#/c/35145/13/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 13: import static org.mockito.Mockito.doReturn; Line 14: import static org.mockito.Mockito.mock; Line 15: import static org.mockito.Mockito.spy; Line 16: Line 17: @RunWith(Parameterized.class) > Only one of the tests need the parameters passed to it, i.e. the other test will required the use of JUnitParamsRunner which result in a new dependency demand. recommend to keep it as is. Line 18: public class SubnetMaskValidationTest { Line 19: Line 20: private final String mask; Line 21: private final boolean isMaskValid; Line 26: this.isMaskValid = isMaskValid; Line 27: this.isPrefixAllowed = isPrefixAllowed; Line 28: } Line 29: Line 30: private SubnetMaskValidation createUnderTest(boolean isPrefixAllowed, > This method shouldn't receive the String messages as arguments - there's no Done Line 31: String InvalidMaskOutput, Line 32: String BadNetmaskFormatMessageOutput, Line 33: String BadPrefixOrNetmaskFormatMessageOutput) { Line 34: SubnetMaskValidation underTest = spy(new SubnetMaskValidation(isPrefixAllowed)); Line 59: boolean isMaskValid, Line 60: boolean isPrefixValid, Line 61: ErrorMessage errorType) { Line 62: Line 63: final String MASK = "TESTED MASK"; //$NON-NLS-1$ > You don't need this - the way to signify that the input doesn't matter is t Done Line 64: MaskValidator validator = mock(MaskValidator.class); Line 65: doReturn(isPrefixValid && isMaskValid).when(validator).isPrefixValid(MASK); Line 66: doReturn(isNetmaskValidFormat).when(validator).isValidNetmaskFormat(MASK); Line 67: doReturn(isMaskValid).when(validator).isNetmaskValid(MASK); Line 61: ErrorMessage errorType) { Line 62: Line 63: final String MASK = "TESTED MASK"; //$NON-NLS-1$ Line 64: MaskValidator validator = mock(MaskValidator.class); Line 65: doReturn(isPrefixValid && isMaskValid).when(validator).isPrefixValid(MASK); > Similar to the comment on the common test class, this shouldn't depend on i Done Line 66: doReturn(isNetmaskValidFormat).when(validator).isValidNetmaskFormat(MASK); Line 67: doReturn(isMaskValid).when(validator).isNetmaskValid(MASK); Line 68: Line 69: SubnetMaskValidation subnetMaskValidationSpy = Line 70: createUnderTest(isPrefixAllowed, Line 71: ErrorMessage.invalidMask.name(), Line 72: ErrorMessage.badNetmaskFormatMessage.name(), Line 73: ErrorMessage.badPrefixOrNetmaskFormatMessage.name()); Line 74: Mockito.doReturn(validator).when(subnetMaskValidationSpy).getMaskValidator(); > Why do you need the "Mockito." prefix? Done Line 75: Line 76: ValidationResult actualResult = subnetMaskValidationSpy.validate(MASK); Line 77: Line 78: final String exceptionMessage = Line 82: isNetmaskValidFormat, Line 83: isMaskValid, Line 84: isPrefixValid); Line 85: Line 86: assertEquals(exceptionMessage, isMaskValid, actualResult.getSuccess());//$NON-NLS-1$ > This is not for this test to check - it's the job of the other test to chec Done Line 87: if (actualResult.getSuccess()) { Line 88: assertEquals(exceptionMessage, isMaskValid, actualResult.getReasons().isEmpty());//$NON-NLS-1$ Line 89: } Line 90: else { Line 86: assertEquals(exceptionMessage, isMaskValid, actualResult.getSuccess());//$NON-NLS-1$ Line 87: if (actualResult.getSuccess()) { Line 88: assertEquals(exceptionMessage, isMaskValid, actualResult.getReasons().isEmpty());//$NON-NLS-1$ Line 89: } Line 90: else { > Again formatting-wise, else would typically be on the same line as the '}' Done Line 91: assertEquals(exceptionMessage, errorType.name(), actualResult.getReasons().get(0));//$NON-NLS-1$ Line 92: } Line 93: Line 94: } Line 93: Line 94: } Line 95: Line 96: enum ErrorMessage { Line 97: NoError, > This enum value isn't required. Since when the result is valid you only che it's needed for the exceptionMessage build. null pointer exception will be thrown because errorType will be null Line 98: badPrefixOrNetmaskFormatMessage, Line 99: badNetmaskFormatMessage, Line 100: invalidMask Line 101: } Line 100: invalidMask Line 101: } Line 102: Line 103: @Parameterized.Parameters Line 104: public static Collection<Object[]> namesParams() { > Please update the test cases according what's in the common test class afte Done Line 105: return Arrays.asList(new Object[][] { Line 106: Line 107: // Bad Format Line 108: { null, false, true }, //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/35145/13/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("Netmask/Routing Prefix") > Please add space around the '/' character - so it doesn't seem like we mean not enough space for doing so 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: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
