Eliraz Levi 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. isNetmaskValidFo Done 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 th Done 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? Done 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 pa Done 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. Done 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. Done 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? Done 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. Done 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. Done 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 Done 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. Done 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 Done 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. Done 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
