Lior Vernia has posted comments on this change. Change subject: common: adding maskValidator util ......................................................................
Patch Set 12: (13 comments) http://gerrit.ovirt.org/#/c/35144/12/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/MaskValidatorTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/MaskValidatorTest.java: Line 19: Line 20: public MaskValidatorTest(String mask, boolean isNetmaskValidFormat, boolean isMaskValid, boolean isPrefixValid) { Line 21: this.mask = mask; Line 22: this.isNetmaskValidFormat = isNetmaskValidFormat; Line 23: this.isMaskValid = isMaskValid; These two members probably should be similarly named, e.g. isNetmaskValidFormat and isNetmaskValidValue. Line 24: this.isPrefixValid = isPrefixValid; Line 25: } Line 26: Line 27: @Test Line 32: Line 33: @Test Line 34: public void checkPrefixFormatValidation() { Line 35: assertEquals("Failed to validate prefix's Format: " + mask, Line 36: isMaskValid && isPrefixValid, MaskValidator.getInstance().isPrefixValid(mask)); The isMaskValid parameter is irrelevant here. This test only checks that the input is in valid prefix format. Line 37: } Line 38: Line 39: @Test Line 40: public void checkNetmaskValidValue() { Line 51: public static Collection<Object[]> namesParams() { Line 52: return Arrays.asList(new Object[][] { Line 53: Line 54: // Bad Format Line 55: { null, false, false, true }, //$NON-NLS-1$ Should null be a valid prefix? Line 56: { "", false, false, false }, //$NON-NLS-1$ Line 57: { "a.a.a.a", false, false, false }, //$NON-NLS-1$ Line 58: { "255.255.0", false, false, false }, //$NON-NLS-1$ Line 59: { "255.255.0", false, false, false }, //$NON-NLS-1$ Line 52: return Arrays.asList(new Object[][] { Line 53: Line 54: // Bad Format Line 55: { null, false, false, true }, //$NON-NLS-1$ Line 56: { "", false, false, false }, //$NON-NLS-1$ Whenever the first parameter is false, an expected result for the second parameter is ill-defined, and the result should be the same no matter whether true or false was passed. In those cases, I would pass a random true/false value (you could add a local method in this class that generates it and pass an invocation of it as the third parameter). This best relays the message that the parameter value isn't pertinent. Line 57: { "a.a.a.a", false, false, false }, //$NON-NLS-1$ Line 58: { "255.255.0", false, false, false }, //$NON-NLS-1$ Line 59: { "255.255.0", false, false, false }, //$NON-NLS-1$ Line 60: { "255.255.0.0.0", false, false, false }, //$NON-NLS-1$ Line 55: { null, false, false, true }, //$NON-NLS-1$ Line 56: { "", false, false, false }, //$NON-NLS-1$ Line 57: { "a.a.a.a", false, false, false }, //$NON-NLS-1$ Line 58: { "255.255.0", false, false, false }, //$NON-NLS-1$ Line 59: { "255.255.0", false, false, false }, //$NON-NLS-1$ This test is identical to the one above it. Line 60: { "255.255.0.0.0", false, false, false }, //$NON-NLS-1$ Line 61: { "255.255.0.0.", false, false, false }, //$NON-NLS-1$ Line 62: Line 63: { "31 ", false, false, false }, //$NON-NLS-1$ /*note extra space*/ Line 61: { "255.255.0.0.", false, false, false }, //$NON-NLS-1$ Line 62: Line 63: { "31 ", false, false, false }, //$NON-NLS-1$ /*note extra space*/ Line 64: { "33", false, false, false }, //$NON-NLS-1$ Line 65: { "33", false, false, false }, //$NON-NLS-1$ This too. Line 66: { "31/", false, false, false }, //$NON-NLS-1$ Line 67: { "31*", false, false, false }, //$NON-NLS-1$ Line 68: { "//31 ", false, false, false }, //$NON-NLS-1$ Line 69: { "/31 ", false, false, false }, //$NON-NLS-1$ /*note extra space*/ Line 68: { "//31 ", false, false, false }, //$NON-NLS-1$ Line 69: { "/31 ", false, false, false }, //$NON-NLS-1$ /*note extra space*/ Line 70: { "01", false, false, true }, //$NON-NLS-1$ Line 71: { "01/", false, false, false }, //$NON-NLS-1$ Line 72: { "33", false, false, false }, //$NON-NLS-1$ Third duplicate, why do you need so many of the same test? Line 73: { "/33", false, false, false }, //$NON-NLS-1$ Line 74: Line 75: // Not Valid Line 76: { "255.255.0.1", true, false, false }, //$NON-NLS-1$ Line 73: { "/33", false, false, false }, //$NON-NLS-1$ Line 74: Line 75: // Not Valid Line 76: { "255.255.0.1", true, false, false }, //$NON-NLS-1$ Line 77: { "255.255.0.1", true, false, false }, //$NON-NLS-1$ Duplicate. Line 78: { "255.0.255.0", true, false, false }, //$NON-NLS-1$ Line 79: { "255.0.0.255", true, false, false }, //$NON-NLS-1$ Line 80: { "224.0.255.0", true, false, false }, //$NON-NLS-1$ Line 81: Line 80: { "224.0.255.0", true, false, false }, //$NON-NLS-1$ Line 81: Line 82: // Valid Line 83: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Line 84: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Duplicate. Line 85: { "255.255.255.255", true, true, false }, //$NON-NLS-1$ Line 86: Line 87: { "31", false, true, true }, //$NON-NLS-1$ Line 88: { "/31", false, true, true }, //$NON-NLS-1$ Line 83: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Line 84: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Line 85: { "255.255.255.255", true, true, false }, //$NON-NLS-1$ Line 86: Line 87: { "31", false, true, true }, //$NON-NLS-1$ In these four cases as well, the second parameter should be irrelevant and passed randomly. Line 88: { "/31", false, true, true }, //$NON-NLS-1$ Line 89: { "7", false, true, true }, //$NON-NLS-1$ Line 90: { "/7", false, true, true }, //$NON-NLS-1$ Line 91: Line 88: { "/31", false, true, true }, //$NON-NLS-1$ Line 89: { "7", false, true, true }, //$NON-NLS-1$ Line 90: { "/7", false, true, true }, //$NON-NLS-1$ Line 91: Line 92: { "255.255.0.1", true, false, true }, //$NON-NLS-1$ Duplicate. Line 93: { "255.255.0.1", true, false, false }, //$NON-NLS-1$ Line 94: Line 95: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Line 96: Line 89: { "7", false, true, true }, //$NON-NLS-1$ Line 90: { "/7", false, true, true }, //$NON-NLS-1$ Line 91: Line 92: { "255.255.0.1", true, false, true }, //$NON-NLS-1$ Line 93: { "255.255.0.1", true, false, false }, //$NON-NLS-1$ Third duplicate of its kind, again (and also with a wrong expected value in the third parameter). Line 94: Line 95: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Line 96: Line 97: }); Line 91: Line 92: { "255.255.0.1", true, false, true }, //$NON-NLS-1$ Line 93: { "255.255.0.1", true, false, false }, //$NON-NLS-1$ Line 94: Line 95: { "255.255.0.0", true, true, false }, //$NON-NLS-1$ Duplicate. Line 96: Line 97: }); Line 98: } Line 99: -- To view, visit http://gerrit.ovirt.org/35144 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04fb0aa2193801688f1bfc5dc7684cabdfa2827d Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
