Fixing zero-length interval bug in IntervalList.merge

* Fixing a bug in IntervalList.merge() that caused it to break when merging 0 length intervals * Improving tests for merge * The bug was introduced in #1265
samtools · Mar 6, 2019 · 21e9461 · 21e9461
1 parent d678af3
commit 21e9461
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 17 deletions.
diff --git a/src/main/java/htsjdk/samtools/util/IntervalList.java b/src/main/java/htsjdk/samtools/util/IntervalList.java
@@ -379,13 +379,14 @@ private static List<Interval> breakIntervalAtBandMultiples(final Interval interv
     }
 
     /**
-     * Merges a sorted collection of intervals and optionally concatenates unique names or takes the first name.
+     * Merges a collection of intervals and optionally concatenates unique names or takes the first name.
+     * Uses the contig and strand of the first interval seen without validating that the others match.
      */
     static Interval merge(final Iterable<Interval> intervals, final boolean concatenateNames) {
         final Interval first = intervals.iterator().next();
         final String chrom = first.getContig();
         int start = first.getStart();
-        int end = start;
+        int end = first.getEnd();
         final boolean neg = first.isNegativeStrand();
         final LinkedHashSet<String> names = new LinkedHashSet<>();
         final String name;

diff --git a/src/test/java/htsjdk/samtools/util/IntervalListTest.java b/src/test/java/htsjdk/samtools/util/IntervalListTest.java
@@ -564,22 +564,42 @@ public void testFromSequenceName(final Path intervalList, final String reference
         Assert.assertEquals(test.getIntervals(), CollectionUtil.makeList(new Interval(referenceName, 1, length)));
     }
 
-    @Test
-    public void testMerges() {
-        final SortedSet<Interval> intervals = new TreeSet<Interval>() {{
-            add(new Interval("1", 500, 600, false, "foo"));
-            add(new Interval("1", 550, 650, false, "bar"));
-            add(new Interval("1", 625, 699, false, "splat"));
-        }};
-
-        Interval out = IntervalList.merge(intervals, false);
-        Assert.assertEquals(out.getStart(), 500);
-        Assert.assertEquals(out.getEnd(), 699);
+    @DataProvider
+    public Object[][] getMergeTestCases(){
+        final String contig = "1";
+        final Interval foo = new Interval(contig, 500, 600, false, "foo");
+        final Interval bar = new Interval(contig, 550, 650, false, "bar");
+        final Interval splat = new Interval(contig, 625, 699, false, "splat");
+        final List<Interval> threeInOrderIntervals = Arrays.asList(foo, bar, splat);
+        final List<Interval> threeOutOfOrderIntervals = Arrays.asList(bar, foo, splat);
+        final Interval zeroLengthInterval = new Interval(contig, 626, 625);
+        final Interval interval600To601 = new Interval(contig, 600, 601);
+        final Interval interval600To625 = new Interval(contig, 600, 625);
+        final Interval normalInterval = new Interval(contig, 626, 629, true, "whee");
+        return new Object[][]{
+                {threeInOrderIntervals, true, new Interval(contig, 500, 699, false, "foo|bar|splat")},
+                {threeInOrderIntervals, false, new Interval(contig, 500, 699, false, "foo")},
+                {threeOutOfOrderIntervals, true, new Interval(contig, 500, 699, false, "bar|foo|splat")},
+                {threeOutOfOrderIntervals, false, new Interval(contig, 500, 699, false, "bar")},
+                {Collections.singletonList(normalInterval), true, normalInterval},
+                {Collections.singletonList(normalInterval), false, normalInterval},
+                {Collections.singletonList(zeroLengthInterval), true, zeroLengthInterval},
+                {Collections.singletonList(zeroLengthInterval), false, zeroLengthInterval},
+                {Arrays.asList(zeroLengthInterval, interval600To601), true, interval600To625},
+                {Arrays.asList(zeroLengthInterval, interval600To601), false, interval600To625},
+                {Arrays.asList(zeroLengthInterval, interval600To601), true, interval600To625},
+                {Arrays.asList(interval600To601, new Interval(contig, 100, 200, false, "hasName")), true, new Interval(contig, 100, 601, false, "hasName")},
+                {Arrays.asList(interval600To601, new Interval(contig, 100, 200, false, "hasName")), false, new Interval(contig, 100, 601, false, "hasName")}
+        };
+    }
 
-        intervals.add(new Interval("1", 626, 629, false, "whee"));
-        out = IntervalList.merge(intervals, false);
-        Assert.assertEquals(out.getStart(), 500);
-        Assert.assertEquals(out.getEnd(), 699);
+    @Test(dataProvider ="getMergeTestCases")
+    public void testMerges(Iterable<Interval> intervals, boolean concatNames, Interval expected) {
+        final Interval merged = IntervalList.merge(intervals, concatNames);
+        Assert.assertEquals(merged.getContig(), expected.getContig());
+        Assert.assertEquals(merged.getStart(), expected.getStart());
+        Assert.assertEquals(merged.getStrand(), expected.getStrand());
+        Assert.assertEquals(merged.getName(), expected.getName());
     }
 
     @Test