Lior Vernia 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 '}' character. 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 doesn't depend on them - see if you can annotate only that test with this (or a similar annotation). 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 harm in always mocking the messages (this actually better depicts "reality") and having this method only receive the isPrefixAllowed argument. 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 to use any(String.class). any() is a static method of Mockito, you'll need to static import it. 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 isMaskValid. 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? 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 check that this validation returns true/false according to "real life" input. This test's responsibility is only to check proper mapping of error messages. As a side note, this check isn't correct either. It should have checked that actualResult.getSuccess() equals (isNetmaskValidFormat && isMaskValid) || (isPrefixAllowed && isPrefixValid). 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 '}' character. 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 check that the reasons collection is empty, it's never compared to anything. You can just pass null instead of it. 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 after you respond to the comments there (and re-evaluate the proper expected values). 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 either Netmask Prefix or Routing Prefix (Netmask Prefix is meaningless). 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
