Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Increase Test Coverage, fix tiny bug in Node class (regarding OVERLAPS) #779
Conversation
yfarjoun
requested a review
from lbergelson
Jan 5, 2017
coveralls
commented
Jan 5, 2017
lbergelson
requested changes
Jan 6, 2017
@yfarjoun Always good to have more tests!
I don't think the DateParser tests you added actually test anything though. It looks like they're supposed to be roundtrip from date -> string and back but instead of asserting that they just print log messages and catch any exceptions that would indicate error conditions...
I think there are a few more off by 1 errors in some of the other methods of IntervalTree.Node<> as well.
| + private final IntervalTree<String> intervalTree = new IntervalTree<String>(); | ||
| + | ||
| + @BeforeMethod | ||
| + public void init(){ //due to the destructive nature of removeMany test... |
lbergelson
Jan 6, 2017
Contributor
In general I think its probably safer to replace these sorts of @BeforeMethod calls with direct calls to a function that just returns a new interval tree and call that explicitly in each test.
yfarjoun
Jan 7, 2017
Contributor
I'm assuming that this is due to concernes regarding multithreading. I'll add @Test(singleThreaded=true) to the class
| @@ -144,13 +143,22 @@ public void testCountMismatches(final String readString, final String cigar, fin | ||
| final SAMRecord rec = new SAMRecord(null); | ||
| rec.setReadName("test"); | ||
| rec.setReadString(readString); | ||
| + final byte[] byteArray = new byte[readString.length()]; | ||
| + for(int i =0; i< readString.length();i++){ |
| + rec.setReadString(readString); | ||
| + rec.setReadNegativeStrandFlag(!positiveStrand); | ||
| + final byte[] byteArray = new byte[readString.length()]; | ||
| + for(int i = 0; i < readString.length();i++){ |
| + assertEquals(qualities2, new byte[]{30, 20, 10}); | ||
| + } | ||
| + | ||
| + private void assertEquals(final byte[] actual, final byte[] expected) { |
lbergelson
Jan 6, 2017
Contributor
Isn't there an Assert.assertEquals(byte[], byte[]) in testng already?
lbergelson
Jan 10, 2017
Contributor
Very weird, I see one when I look at the testng source in intellij.
| + } | ||
| + | ||
| + @Test(dataProvider = "dateDate") | ||
| + public static void testString(final String string) { |
lbergelson
Jan 6, 2017
Contributor
What are these tests supposed to be accomplishing? They execute the date parsing code, but they don't actually assert anything.
On a side note, is the date parser something we should continue to support? I'm guessing it exists to deal with issues in java.util.Date and lack of functionality there. Now that we have java.util.time we may be able to deprecate and replace it.
| @@ -492,7 +492,7 @@ public int getRelationship( final Node<V1> interval ) | ||
| result = HAS_LESSER_PART; | ||
| if ( mEnd > interval.getEnd() ) | ||
| result |= HAS_GREATER_PART; | ||
| - if ( mStart < interval.getEnd() && interval.getStart() < mEnd ) | ||
| + if ( mStart <= interval.getEnd() && interval.getStart() <= mEnd ) |
lbergelson
Jan 6, 2017
Contributor
Good catch. I'm surprised we haven't noticed this bug when using OverlapDetector...
lbergelson
Jan 6, 2017
Contributor
I think there's a similar bug in getLength(). Shouldn't it be end - start +1?
Also in isAdjacent(), it needs to adjust by 1 on both ends I think...
yfarjoun
Jan 7, 2017
Contributor
thankfully all of these members are unused, which is why it went undetected.....the getLength function has no contract...so it's not clear what the intention was...but sure, I'll change it.
| @@ -836,6 +811,16 @@ else if (cigElOp.consumesReferenceBases()) { | ||
| return ret; | ||
| } | ||
| + /** Reverses and complements the bases in place. */ | ||
| + public static void reverseComplement(final byte[] bases) { | ||
| + reverseComplement(bases, 0, bases.length); |
yfarjoun
was assigned
by lbergelson
Jan 6, 2017
|
@lbergelson look again please? |
codecov-io
commented
Jan 7, 2017
Current coverage is 64.47% (diff: 85.71%)
|
| + | ||
| + assertDatesAreClose(orig, roundTrip); | ||
| + | ||
| + } catch (DateParser.InvalidDateException ex) { |
lbergelson
Jan 10, 2017
Contributor
I think we still need to remove these catch clauses because they could hide the fact that none of the dates actually parse.
| +public class DateParserTest { | ||
| + | ||
| + private static void test(final String isodate) { | ||
| + System.out.println("----------------------------------"); |
lbergelson
Jan 10, 2017
Contributor
I'd get rid of these pointless print statements, they just make spam in the test output.
| /** | ||
| * @author alecw@broadinstitute.org | ||
| */ | ||
| +@Test(singleThreaded=true) // to assure that the common resources aren't clobbered |
lbergelson
Jan 10, 2017
Contributor
I really think it would be easier to just have init() return an interval tree and call it in each method to get the interval tree to use, but I guess this is fine if a bit more esoteric.
|
@yfarjoun Those date tests are still a bit funky, |
|
better @lbergelson ? |
yfarjoun commentedJan 5, 2017
Description
Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist