Unit Tests and fixes for a few classes in samtools.cram.io #1198

jmthibault79 · 2018-10-16T20:08:17Z

Description

Increasing test coverage for the samtools.cram.io package, and fixes to bugs found by these tests

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

- add IOTestCases and CramIntTest - add 64 bit CramInt - byte[] and ByteBuffer tests and fixes - add CramLong and CramLongTest

jmthibault79 · 2018-10-16T20:10:11Z

src/main/java/htsjdk/samtools/cram/io/CramInt.java

@@ -40,7 +40,10 @@ public static int int32(final byte[] data) {
     * @return an integer value read from the buffer
     */
    public static int int32(final ByteBuffer buffer) {
-        return buffer.get() | buffer.get() << 8 | buffer.get() << 16 | buffer.get() << 24;


These shifts were being attempted in a byte context

Do you have a test case that fails without this change?

Does my 0xff comment work for you on this point?

jmthibault79 · 2018-10-16T20:11:34Z

src/main/java/htsjdk/samtools/cram/io/CramIntArray.java

@@ -7,7 +7,7 @@
 /**
 * Methods to read and write CRAM array of integers data type.
 */
-public class CramArray {
+public class CramIntArray {


Renamed because this gave me the impression of a generic array type, though it's limited to integer. I can revert this if you prefer.

Works for me.

jmthibault79 · 2018-10-16T20:12:10Z

src/main/java/htsjdk/samtools/cram/io/CramLong.java

+ * Methods to read and write CRAM long values as given in the file format specification.
+ */
+
+public class CramLong {


All methods correspond to those in CramInt

So, it looks like longs are a valid data type in the spec, but are unused by the implementation ? Its strange that we've never encountered a problem decoding CRAMs produced by other software, unless they don't use it either. It may be that longs are always written as LTF8 ? I think we should try to resolve that before we commit code and tests that aren't used anywhere. If we are missing functionality, we can keep this and then create a ticket that we need to hook it up. Do you know if/how htslib uses these ?

Looking through cram_io.h in htslib, they only seem to use ITF8, LFT8, and int32.

CramInt in htsjdk is only used for Block checksums and Container Header sizes and checksums. Most other such values are ITF8/LTF8.

So you're right that this does seem unused. I can pull this change out of this PR into a different branch we can bring back if we find a later use for it.

jmthibault79 · 2018-10-16T20:13:13Z

src/test/java/htsjdk/cram/io/ExternalCompressionTest.java

@@ -1,29 +0,0 @@
-package htsjdk.cram.io;


moved from htsjdk.cram.io to htsjdk.samtools.cram.io like the class it's testing

jmthibault79 · 2018-10-16T20:14:13Z

src/test/java/htsjdk/samtools/cram/io/CramIntTest.java

+        ByteBuffer bb = ByteBuffer.wrap(bytes);
+
+        // a manually-advanced byte buffer
+        final int manualBufferSize = 4;


To exercise the read from byte[] method

jmthibault79 · 2018-10-16T20:15:41Z

src/test/java/htsjdk/samtools/cram/io/IOTestCases.java

+        return params;
+    }
+
+    static List<Integer> int32Tests() {


moved from ITF8Test for reuse

Nice consolidation!

jmthibault79 · 2018-10-16T20:16:07Z

src/test/java/htsjdk/samtools/cram/io/IOTestCases.java

+        };
+    }
+
+    static List<Long> int64Tests() {


moved from LTF8Test for reuse

jmthibault79 · 2018-10-16T20:16:32Z

src/test/java/htsjdk/samtools/cram/io/IOTestCases.java

+        }
+
+        // special cases:
+        list.add(Long.MAX_VALUE);


these two were not in the original test cases

codecov-io · 2018-10-16T20:22:57Z

Codecov Report

Merging #1198 into master will increase coverage by 0.058%.
The diff coverage is 91.667%.

@@               Coverage Diff               @@
##              master     #1198       +/-   ##
===============================================
+ Coverage     68.414%   68.472%   +0.058%     
- Complexity      8019      8025        +6     
===============================================
  Files            542       542               
  Lines          32701     32704        +3     
  Branches        5530      5530               
===============================================
+ Hits           22372     22393       +21     
+ Misses          8125      8109       -16     
+ Partials        2204      2202        -2

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/htsjdk/samtools/cram/io/CramIntArray.java	`88.889% <0%> (ø)`	`4 <0> (?)`
...ain/java/htsjdk/samtools/cram/structure/Block.java	`62.5% <100%> (ø)`	`30 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/cram/io/CramInt.java	`85% <100%> (+43.824%)`	`5 <1> (+2)`	⬆️
...n/java/htsjdk/samtools/cram/structure/SliceIO.java	`88.889% <100%> (ø)`	`15 <0> (ø)`	⬇️
...jdk/samtools/cram/structure/ContainerHeaderIO.java	`86.842% <100%> (ø)`	`6 <0> (ø)`	⬇️
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java	`64.865% <0%> (+2.703%)`	`7% <0%> (+1%)`	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	`76% <0%> (+4%)`	`13% <0%> (+1%)`	⬆️
...a/htsjdk/samtools/cram/io/ExternalCompression.java	`88.571% <0%> (+20%)`	`11% <0%> (+2%)`	⬆️

cmnbroad

Yay - more tests. A few questions/comments.

cmnbroad · 2018-10-17T12:54:00Z

src/main/java/htsjdk/samtools/cram/io/CramIntArray.java

@@ -7,7 +7,7 @@
 /**
 * Methods to read and write CRAM array of integers data type.
 */
-public class CramArray {
+public class CramIntArray {


Works for me.

cmnbroad · 2018-10-17T13:13:47Z

src/main/java/htsjdk/samtools/cram/io/CramLong.java

+ * Methods to read and write CRAM long values as given in the file format specification.
+ */
+
+public class CramLong {


So, it looks like longs are a valid data type in the spec, but are unused by the implementation ? Its strange that we've never encountered a problem decoding CRAMs produced by other software, unless they don't use it either. It may be that longs are always written as LTF8 ? I think we should try to resolve that before we commit code and tests that aren't used anywhere. If we are missing functionality, we can keep this and then create a ticket that we need to hook it up. Do you know if/how htslib uses these ?

cmnbroad · 2018-10-17T13:20:48Z

src/test/java/htsjdk/samtools/cram/io/CramIntArrayTest.java

+    public void runTest(List<Integer> ints) throws IOException {
+
+        int[] inputArray = ints.stream().mapToInt(Integer::intValue).toArray();
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();


Can you use try-with-resources for these to ensure they're closed even if an exception is thrown.

cmnbroad · 2018-10-17T13:21:02Z

src/test/java/htsjdk/samtools/cram/io/CramIntArrayTest.java

+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        CramIntArray.write(inputArray, baos);
+
+        ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());


try-with-resources

cmnbroad · 2018-10-17T14:01:23Z

src/main/java/htsjdk/samtools/cram/io/CramInt.java

+        return (0xFF & (int) buffer.get()) |
+                (0xFF & (int) buffer.get()) << 8 |
+                (0xFF & (int) buffer.get()) << 16 |
+                (0xFF & (int) buffer.get()) << 24;


While we're in this class I'd be in favor of renaming the int32 overloads in to readInt32 so they're symmetric with the writeInt32 ones. WDYT ?

I like that

cmnbroad · 2018-10-17T14:05:53Z

src/main/java/htsjdk/samtools/cram/io/CramLong.java

+     * @return a long value read
+     * @throws IOException as per java IO contract
+     */
+    public static long int64(final InputStream inputStream) throws IOException {


Same naming comment here if we keep these.

cmnbroad · 2018-10-17T14:10:53Z

src/test/java/htsjdk/samtools/cram/io/CramIntTest.java

+        }
+
+        baos.close();
+        bais.close();


We should probably add something to verify that these methods are really respecting little endianness.

I'd prefer to see this factored so there is one test method per method (or one test method per read/write overload/pair), rather than mixing them all up, with common code factored out as appropriate.

cmnbroad · 2018-10-17T14:20:42Z

src/test/java/htsjdk/samtools/cram/io/IOTestCases.java

+        return params;
+    }
+
+    static List<Integer> int32Tests() {


Nice consolidation!

- split CramInt tests into multiple - try-with-resources - rename to readInt32

jmthibault79 · 2018-10-17T18:41:36Z

Comments addressed

cmnbroad

@lbergelson more tests @jmthibault79 LGTM - thx!

pshapiro4broad · 2018-10-18T18:47:43Z

src/main/java/htsjdk/samtools/cram/io/CramInt.java

-        return buffer.get() | buffer.get() << 8 | buffer.get() << 16 | buffer.get() << 24;
+    public static int readInt32(final ByteBuffer buffer) {
+        return (0xFF & (int) buffer.get()) |
+                (0xFF & (int) buffer.get()) << 8 |


Casting to int should be unnecessary here, as byte is promoted to int before the bit shift operation is applied.

Also am not sure why 0xff & is necessary as buffer.get() returns byte. I must be missing something here...

I'll remove the int casts - good catch.

0xFF & is still necessary because int-promotion extends the negative sign into higher bytes.

OK I now see why this is necessary, I forgot that byte isn't an unsigned type. Why, Java? Why?

pshapiro4broad · 2018-10-18T18:52:41Z

src/test/java/htsjdk/samtools/cram/io/IOTestCases.java

+    }
+
+    private static <T> Object[][] asDataProvider(List<T> list) {
+        Object[][] params = new Object[list.size()][] ;


extra space

Suggested change

Object[][] params = new Object[list.size()][] ;

Object[][] params = new Object[list.size()][];

pshapiro4broad · 2018-10-18T18:53:08Z

src/test/java/htsjdk/samtools/cram/io/IOTestCases.java

+
+    private static <T> Object[][] asDataProvider(List<T> list) {
+        Object[][] params = new Object[list.size()][] ;
+        for (int i=0; i<params.length; i++)


missing space

Suggested change

for (int i=0; i<params.length; i++)

for (int i = 0; i < params.length; i++)

pshapiro4broad · 2018-10-18T18:53:50Z

src/test/java/htsjdk/samtools/cram/io/ExternalCompressionTest.java

+
+public class ExternalCompressionTest extends HtsjdkTest {
+    public static final File BZIP2_FILE = new File("src/test/resources/htsjdk/samtools/cram/io/bzip2-test.bz2");
+    public static final byte [] TEST_BYTES = "This is a simple string to test compression".getBytes();


odd spacing, I would expect to not have a space before []

Suggested change

public static final byte [] TEST_BYTES = "This is a simple string to test compression".getBytes();

public static final byte[] TEST_BYTES = "This is a simple string to test compression".getBytes();

pshapiro4broad · 2018-10-18T18:56:18Z

src/main/java/htsjdk/samtools/cram/io/CramInt.java

@@ -40,7 +40,10 @@ public static int int32(final byte[] data) {
     * @return an integer value read from the buffer
     */
    public static int int32(final ByteBuffer buffer) {
-        return buffer.get() | buffer.get() << 8 | buffer.get() << 16 | buffer.get() << 24;


Do you have a test case that fails without this change?

lbergelson

👍

jmthibault79 added 3 commits October 16, 2018 15:48

Additional roundtrips for ExternalCompressionTest

0cb5dbf

moved inside samtools

cb9686a

Rename CramArray to CramIntArray and add tests

66fc5e5

- add IOTestCases and CramIntTest - add 64 bit CramInt - byte[] and ByteBuffer tests and fixes - add CramLong and CramLongTest

jmthibault79 added bug cram labels Oct 16, 2018

jmthibault79 commented Oct 16, 2018

View reviewed changes

jmthibault79 requested review from cmnbroad and lbergelson October 16, 2018 20:17

cmnbroad requested changes Oct 17, 2018

View reviewed changes

jmthibault79 added 2 commits October 17, 2018 14:36

Add endianness tests

5f5203e

- split CramInt tests into multiple - try-with-resources - rename to readInt32

rm CramLong / CramLongTest

c120a4b

cmnbroad approved these changes Oct 18, 2018

View reviewed changes

pshapiro4broad reviewed Oct 18, 2018

View reviewed changes

jmthibault79 added 2 commits October 18, 2018 16:05

Don't need to cast to int because bitshifting already does that

7677f8e

spacing

baa4a24

pshapiro4broad approved these changes Oct 19, 2018

View reviewed changes

lbergelson approved these changes Oct 19, 2018

View reviewed changes

jmthibault79 merged commit 37069a3 into master Oct 19, 2018

jmthibault79 deleted the jt_io_ut branch October 19, 2018 15:26

	Object[][] params = new Object[list.size()][] ;
	Object[][] params = new Object[list.size()][];

	for (int i=0; i<params.length; i++)
	for (int i = 0; i < params.length; i++)

	public static final byte [] TEST_BYTES = "This is a simple string to test compression".getBytes();
	public static final byte[] TEST_BYTES = "This is a simple string to test compression".getBytes();

Unit Tests and fixes for a few classes in samtools.cram.io #1198

Unit Tests and fixes for a few classes in samtools.cram.io #1198

Conversation

jmthibault79 commented Oct 16, 2018 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 16, 2018 • edited Loading

Codecov Report

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmthibault79 commented Oct 17, 2018

cmnbroad left a comment

Choose a reason for hiding this comment

pshapiro4broad Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

jmthibault79 commented Oct 16, 2018 •

edited

Loading

codecov-io commented Oct 16, 2018 •

edited

Loading

cmnbroad Oct 17, 2018 •

edited

Loading

pshapiro4broad Oct 18, 2018 •

edited

Loading