Martin Mucha has posted comments on this change. Change subject: core: util for removing overlaps in ranges ......................................................................
Patch Set 15: (3 comments) answers. http://gerrit.ovirt.org/#/c/26403/15/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/DisjointRangesTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/DisjointRangesTest.java: Line 25: return Arrays.asList(new Object[][]{ Line 26: { Collections.<Pair<Long, Long>>emptyList(), Collections.<Pair<Long, Long>>emptyList(), null, null }, Line 27: { Arrays.asList(pair(1, 2)), Arrays.asList(pair(1, 2)), null, null }, Line 28: { Arrays.asList( pair( 1, 2 ), pair( 3, 4 ) ), Arrays.asList( pair( 1, 2), pair(3, 4 ) ), null, null }, Line 29: { Arrays.asList( pair( 2, 1 ), pair( 3, 4 ) ), Collections.<Pair<Long, Long>>emptyList(), IllegalArgumentException.class, "badly defined range" }, > Not sure why you decided to delete this test... cannot happen. LongRange implementation fixes this misuse. Line 30: { Arrays.asList( pair( 1, 3 ), pair( 2, 5 ) ), Arrays.asList( pair( 1, 5 ) ), null, null }, Line 31: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 6, 7) ), Arrays.asList( pair( 1, 3 ), pair(5, 7) ), null, null }, Line 32: { Arrays.asList( pair( 1, 2 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 2 ), pair(4, 7) ), null, null }, Line 33: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 3), pair(4, 7 ) ), null, null }, Line 58: this.expectedErrorMessage = expectedErrorMessage; Line 59: } Line 60: Line 61: @Test Line 62: public void test() throws Exception { > Randomization allows to cover more cases than you though of, and is a cheap ok, using same seed it's doable. Lets try it. DONE. Line 63: Line 64: if (expectedExceptionClass != null) { Line 65: this.expectedException.expect(expectedExceptionClass); Line 66: this.expectedException.expectMessage(expectedErrorMessage); Line 62: public void test() throws Exception { Line 63: Line 64: if (expectedExceptionClass != null) { Line 65: this.expectedException.expect(expectedExceptionClass); Line 66: this.expectedException.expectMessage(expectedErrorMessage); > The criteria of being consistent in what you write. I think I am consistent upon *mine* criteria :) The criteria for mine approach is very simple: method is big enough that I cannot remember name of field, so I use 'this' and code completion to find out. Well is not and project-wide standard — only in VDS class there are 157 violations of this rule. But even if it's just an <we'd like it have it that way> standard, I agree to stick to it. I setup IDE appropriately to consider this as an error. Please if there are code conventions overriding oracle's, let me know(mail them to me, please) so I can write the code in respect to them. Now what to do with current existing violations? Lets say I have to work with file this invalid. In IDEA there's this nice button 'fix-them-all'. Would it be appropriate to click it? For example for VDS file(I'm not doing it, just asking). Nothing can be broken; it's automatic and trivial fix. Takes no time, improves code base to <wanna-have> standards. Line 67: } Line 68: Line 69: List<Pair<Long, Long>> result = DisjointRanges.removePotentialOverlaps(inputRanges); Line 70: Assert.assertArrayEquals("Input: " + Arrays.toString(inputRanges.toArray()) + "\nActual: " + Arrays.toString(result.toArray()) + "\n" + "Expected: " + Arrays.toString(expectedRanges.toArray()) + "\n\n", expectedRanges.toArray(), result.toArray()); -- To view, visit http://gerrit.ovirt.org/26403 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7dbacd11b610a5885d574356a695c6e879dcdbc Gerrit-PatchSet: 15 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
