Mike Kolesnik has posted comments on this change. Change subject: core: util for removing overlaps in ranges ......................................................................
Patch Set 15: (4 comments) http://gerrit.ovirt.org/#/c/26403/15/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/DisjointRanges.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/DisjointRanges.java: Line 6: import java.util.ListIterator; Line 7: Line 8: import org.ovirt.engine.core.common.utils.Pair; Line 9: Line 10: public class DisjointRanges { > I think this is a classic example of methods so simple and obvious that one The idea is to specify the API since Java is not enough restrictive (yet) to specify it in a programmatic way. For example, exception that's being thrown if you expect something is worth mentioning. Also expected input and outputs, etc. Line 11: Line 12: private List<Pair<Long, Long>> disjointRanges = new LinkedList<>(); Line 13: Line 14: 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... 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 { > tests need to perform always the same if possible. Randomization is no good Randomization allows to cover more cases than you though of, and is a cheap way to achieve more actual coverage. Using the same seed you will always get the same test so I don't agree with your statement. 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); > tell me, please, why is this so important to you? I know, that 'this' can b The criteria of being consistent in what you write. Also this project doesn't use this except constructors and setters, it's the de-facto standard. 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
