Follow-up to #1276: Remove multiple versions of Slice/Container getCRAIEntries() #1329

jmthibault79 · 2019-03-15T21:05:05Z

Encapsulate Container.byteOffset and distribute to Slices on set
enforce prerequisites to index entry creation

Description

Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?

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)

jmthibault79 · 2019-03-15T21:05:39Z

src/main/java/htsjdk/samtools/BAMIndexMetaData.java

@@ -159,7 +159,7 @@ void recordMetaData(final Slice slice) {
            unAlignedRecords += slice.unmappedReadsCount;
        }

-        final long start = slice.offset;


renamed the Slice indexing parameters to make their function clearer.

jmthibault79 · 2019-03-15T21:05:55Z

src/main/java/htsjdk/samtools/CRAMBAIIndexer.java

@@ -125,14 +124,12 @@ public void processContainer(final Container container, final ValidationStringen

        int sliceIndex = 0;
        for (final Slice slice : container.slices) {
-            slice.containerOffset = container.offset;


this is now set elsewhere

Container.setSlices() sets this value. If the slices exist prior to the setting of this value, then Container.setByteOffset() sets the slices' values.

jmthibault79 · 2019-03-15T21:06:50Z

src/main/java/htsjdk/samtools/cram/CRAIIndex.java

@@ -59,7 +59,7 @@ public void writeIndex(final OutputStream os) {
     * @param container the container to index
     */
    public void processContainer(final Container container) {
-        addEntries(container.getCRAIEntriesSplittingMultiRef());
+        addEntries(container.getCRAIEntries());


we always split multiref now

jmthibault79 · 2019-03-15T21:11:20Z

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

@@ -58,8 +58,7 @@ public Container buildContainer(final List<CramCompressionRecord> records) {
            slices.add(slice);
        }

-        final Container container = Container.initializeFromSlices(slices);
-        container.compressionHeader = compressionHeader;
+        final Container container = Container.initializeFromSlices(slices, compressionHeader);


We need the compressionHeader when we call getCraiEntries() so requiring it's here helps ensure it's valid

jmthibault79 · 2019-03-15T21:12:17Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

-    /**
-     * Retrieve the list of CRAI Index entries corresponding to this Container.
-     *
-     * TODO: investigate why we sometimes split multi-ref Slices


now we always split

jmthibault79 · 2019-03-15T21:12:40Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

-     * TODO: investigate why we sometimes split multi-ref Slices
-     * into different entries and sometimes do not
-     *
-     * TODO: clearly identify and enforce preconditions, e.g.


precondition enforcement is in craiIndexInitializationCheck()

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

jmthibault79 · 2019-03-15T21:15:29Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

        if (referenceContext.isMultiRef()) {
            final Map<ReferenceContext, AlignmentSpan> spans = getMultiRefAlignmentSpans(compressionHeader, ValidationStringency.DEFAULT_STRINGENCY);

            return spans.entrySet().stream()
                    .map(e -> new CRAIEntry(e.getKey().getSerializableId(),
                            e.getValue().getStart(),
                            e.getValue().getSpan(),
-                            containerStartByteOffset,


can trust the Slice now instead of relying on the Container, for these 2 values

jmthibault79 · 2019-03-15T21:15:43Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

        if (referenceContext.isMultiRef()) {
            final Map<ReferenceContext, AlignmentSpan> spans = getMultiRefAlignmentSpans(compressionHeader, ValidationStringency.DEFAULT_STRINGENCY);

            return spans.entrySet().stream()
                    .map(e -> new CRAIEntry(e.getKey().getSerializableId(),
                            e.getValue().getStart(),
                            e.getValue().getSpan(),
-                            containerStartByteOffset,
-                            landmarks[index],
-                            size))


same member variable. just renamed.

jmthibault79 · 2019-03-15T21:16:50Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

+                    sequenceId,
+                    alignmentStart,
+                    alignmentSpan,
+                    containerByteOffset,


containerByteOffset newly uses the Slice member variable.
byteOffsetFromContainer and byteSize are continuing uses of the Slice member variables, renamed.

codecov-io · 2019-03-15T21:35:51Z

Codecov Report

Merging #1329 into master will increase coverage by 0.026%.
The diff coverage is 82.278%.

@@               Coverage Diff               @@
##              master     #1329       +/-   ##
===============================================
+ Coverage     67.822%   67.848%   +0.026%     
- Complexity      8258      8273       +15     
===============================================
  Files            562       562               
  Lines          33663     33678       +15     
  Branches        5644      5653        +9     
===============================================
+ Hits           22831     22850       +19     
+ Misses          8658      8653        -5     
- Partials        2174      2175        +1

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/cram/CRAIEntry.java	`73.684% <ø> (ø)`	`25 <0> (ø)`	⬇️
...mtools/cram/build/CramContainerHeaderIterator.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
...samtools/cram/build/CramSpanContainerIterator.java	`71.053% <100%> (-2.118%)`	`9 <0> (ø)`
...a/htsjdk/samtools/cram/build/ContainerFactory.java	`96.552% <100%> (-0.115%)`	`5 <0> (ø)`
...va/htsjdk/samtools/cram/build/ContainerParser.java	`96.774% <100%> (ø)`	`10 <0> (ø)`	⬇️
...ava/htsjdk/samtools/CRAMContainerStreamWriter.java	`72.34% <100%> (-0.117%)`	`54 <0> (ø)`
src/main/java/htsjdk/samtools/CRAMFileReader.java	`76.02% <100%> (ø)`	`53 <0> (ø)`	⬇️
...rc/main/java/htsjdk/samtools/BAMIndexMetaData.java	`77.67% <100%> (ø)`	`22 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/cram/CRAIIndex.java	`88.571% <100%> (+0.166%)`	`26 <1> (ø)`	⬇️
...ain/java/htsjdk/samtools/cram/structure/Slice.java	`67.593% <100%> (+3.307%)`	`43 <10> (+9)`	⬆️
... and 9 more

cmnbroad

One question (see comments) to start off with.

cmnbroad · 2019-03-19T22:16:31Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

+            for (final Slice slice : slices) {
+                slice.containerByteOffset = byteOffset;
+            }
+        }


Is there actually a case where this is called and slices == null ? If so, and the slices are somehow populated at a later time, how can we ensure that the offset will be propagated then ?

@jmthibault79 It seems like it should be possible to reduce the number of call sites that need to update these offsets. Every call to this method (except for the one in ContainerStreamWriter, when a new container is being created from records) is immediately preceded by a call (either directly or indirectly), to ContainerIO.readContainer. It would be much simpler to make ContainerIO responsible for all of those cases, even if it means passing the offset in (although I think it could often derive the offset from the stream position). Would that work ? I think it would reduce the complexity around understanding who/when the slice offsets get propagated, etc.

I do think this is possible, and I have started an attempt at this. I'll work on that some more.

Good point about propagation. I will have to think more about that.

Addressed both of these in a new commit.

jmthibault79 · 2019-03-20T16:34:51Z

src/main/java/htsjdk/samtools/CRAMCRAIIndexer.java

-        catch (IOException e) {
-            throw new RuntimeIOException("Error writing CRAI index to output stream");
+        // get the first container
+        Container container = ContainerIO.readContainer(cramVersion, cramStream);


offset is set inside readContainer() by querying the stream

jmthibault79 · 2019-03-20T16:35:22Z

src/main/java/htsjdk/samtools/CRAMFileReader.java

@@ -400,7 +400,7 @@ public SAMRecordIterator assertSorted(final SortOrder sortOrder) {
            seekableStream.seek(0);
            newIterator = new CRAMIterator(seekableStream, referenceSource, validationStringency);
            seekableStream.seek(startOfLastLinearBin >>> 16);
-            final Container container = ContainerIO.readContainerHeader(newIterator.getCramHeader().getVersion().major, seekableStream);
+            final Container container = ContainerHeaderIO.readContainerHeader(newIterator.getCramHeader().getVersion().major, seekableStream);


this was driving me nuts

TODO obviously: move this to a new ContainerHeader class. Just not today.

jmthibault79 · 2019-03-20T16:40:26Z

src/main/java/htsjdk/samtools/cram/build/CramContainerHeaderIterator.java

+     * @param countingStream the {@link CountingInputStream} to read from
+     * @return The next Container's header from the stream, returned as a Container.
+     */
+    @Override


This tripped me up!

Hopefully this explicit Override and Javadoc will save some future grief.

Yeah, good catch.

jmthibault79 · 2019-03-20T16:41:11Z

src/main/java/htsjdk/samtools/cram/build/CramContainerIterator.java

@@ -17,20 +17,14 @@
    private CountingInputStream countingInputStream;
    private Container nextContainer;
    private boolean eof = false;
-    private long offset = 0;


containers can now take care of this. don't need it here.

jmthibault79 · 2019-03-20T17:07:33Z

src/main/java/htsjdk/samtools/cram/build/CramIO.java

@@ -293,36 +297,39 @@ private static SAMFileHeader readSAMFileHeader(final Version version, InputStrea
            } else {
                /*
                 * pending issue: container.containerByteSize inputStream 2 bytes shorter


I have not investigated this

jmthibault79 · 2019-03-20T17:08:01Z

src/main/java/htsjdk/samtools/cram/build/CramIO.java

                block = Block.read(version.major, inputStream);
            }
        }

-        inputStream = new ByteArrayInputStream(block.getUncompressedContent());
+        byte[] bytes;


not really related to this PR. Just a little cleanup while I'm here.

jmthibault79 · 2019-03-20T17:08:43Z

src/main/java/htsjdk/samtools/cram/build/CramSpanContainerIterator.java

-                final Container c = ContainerIO.readContainer(cramHeader.getVersion(), seekableStream);
-                c.offset = offset;
-                return c;
+                return ContainerIO.readContainer(cramHeader.getVersion(), seekableStream);


readContainer() takes care of setting the offset by querying the SeekableStream

jmthibault79 · 2019-03-20T17:09:14Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

@@ -65,13 +65,55 @@
    public CompressionHeader compressionHeader;

    // slices found in the container:
-    public Slice[] slices;
+    private Slice[] slices;


Can't set the slices without also setting their byteOffsets now

jmthibault79 · 2019-03-20T17:11:52Z

src/main/java/htsjdk/samtools/cram/structure/ContainerHeaderIO.java

@@ -70,9 +77,42 @@ public static Container readContainerHeader(final int major, final InputStream i
        if (major >= 3)
            container.checksum = CramInt.readInt32(inputStream);

+        container.setByteOffset(containerByteOffset);


this is now the only remaining external call to setByteOffset()

jmthibault79 · 2019-03-20T17:12:16Z

src/main/java/htsjdk/samtools/cram/structure/ContainerHeaderIO.java

        return container;
    }

+    // convenience methods for SeekableStream and CountingInputStream
+    // TODO: merge these two classes?


these 2 seem VERY similar. It's annoying to have to account for both.

Maybe Seekable can inherit from Counting?

jmthibault79 · 2019-03-20T17:14:38Z

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

+    public static Container readContainer(final Version version,
+                                          final InputStream inputStream,
+                                          final long containerByteOffset) {
+        Container container = readContainerInternal(version.major, inputStream, containerByteOffset);


Another version of this readContainer() consolidation is in #1306

This readContainer method can be made private now.

jmthibault79 · 2019-03-20T17:15:43Z

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

        if (container.isEOF()) {
            return container;
        }

        container.compressionHeader = CompressionHeader.read(major, inputStream);

-        howManySlices = Math.min(container.landmarks.length, howManySlices);


howManySlices and fromSlice were never used. See #1306 for more details.

jmthibault79 · 2019-03-20T17:16:21Z

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

-     * @param outputStream        the stream to write to
-     * @return the number of bytes written
-     */
-    public static int writeContainerHeader(final int major, final Container container, final OutputStream outputStream) {


why did this exist

jmthibault79 · 2019-03-20T17:19:58Z

src/test/java/htsjdk/samtools/CRAMSliceMD5Test.java

-        final CramHeader cramHeader = CramIO.readCramHeader(bais);
-        final Container container = ContainerIO.readContainer(cramHeader.getVersion(), bais);
-        final Slice slice = container.slices[0];
+        Container container;


more unrelated misc cleanup

cmnbroad

Ran into some new questions, per our slack conversation. Checkpointing what I have so far.

src/main/java/htsjdk/samtools/cram/build/CramContainerHeaderIterator.java

cmnbroad · 2019-03-20T17:56:37Z

src/main/java/htsjdk/samtools/cram/build/CramContainerHeaderIterator.java

+     * @param countingStream the {@link CountingInputStream} to read from
+     * @return The next Container's header from the stream, returned as a Container.
+     */
+    @Override


Yeah, good catch.

cmnbroad · 2019-03-20T18:02:40Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

@@ -65,13 +65,55 @@
    public CompressionHeader compressionHeader;

    // slices found in the container:
-    public Slice[] slices;
+    private Slice[] slices;


src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

src/main/java/htsjdk/samtools/cram/build/CramIO.java

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

cmnbroad · 2019-03-20T19:24:18Z

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

            slices.add(SliceIO.read(major, inputStream));
        }

-        container.populateSlicesAndIndexingParameters(slices);
+        container.populateSlicesAndIndexingParameters(slices, containerByteOffset);


This method has the same signature as setSlices - could you just call that instead, and have it figure out whether to call populateSlicesAndIndexingParameters based on whether landmarks are set ?populateSlicesAndIndexingParameters could then be private. You'd have to normalize array->list, but maybe the setSlices signature should be the one to change to accommodate.

Or even better, this could just call setSlices, and defer populating the index info for the slices since its only needed when indexing happens on the container. It might be cleaner to defer the index info calculation until we actually need it to create an index, by which time there should always be landmarks.

Updated by removing its parameters entirely (and renaming to distributeIndexingParametersToSlices). Slices must be now be set before calling this method.

Added tests to ContainerTest and CRAMBAiIndexerTest

cmnbroad · 2019-03-20T21:21:23Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

+    // BAI only
+    public int index = UNINITIALIZED_INDEXING_PARAMETER;
+
+    void baiIndexInitializationCheck() {


This looks like a good check, but it doesn't appear to be called from anywhere except within one test (craiIndexInitializationCheck below is called before indexing).

I started looking at this a bit more. It looks like populateSlicesAndIndexingParameters is only called when we're creating an index for a CRAM file that we've read in from disk, but I'm unclear on how the slice index info gets populated in the case where we're creating the index on-the-fly, when writing a new CRAM file. Since the tests were passing, I assumed it was happening somehow. So, I naively tried adding a call to this method from within CRAMBAIIndexer.processContainer, and then ran as many of the indexing tests as I easily could. 6 of them failed. So this led to a few questions:

Where should the call to baiIndexInitializationCheck method go ?

Does the equivalent of populateSlicesAndIndexingParameters happen in the case where we're writing a cram out from SAMRecords ?

If we aren't populating the slice index info when writing a new CRAM file on this branch, how/why is it that all of the index tests are still passing ?

Is there a similar issue already on master that isn't being caught by tests ?

comments and clarification for CRAMBAIIndexer

- Encapsulate Container.byteOffset and distribute to Slices on set

# This is the 1st commit message: move Container.setByteOffset() call inside ContainerIO.readContainer() - also ContainerHeaderIO.readContainerHeader() restrict access to setByteOffset() and encapsulate Container.slices # This is the commit message #2: oops # This is the commit message #3: a little unrelated cleanup # This is the commit message #4: oops # This is the commit message #5: better CRAIEntryTest # This is the commit message #6: test improvements and a fix # This is the commit message #7: comment # This is the commit message #8: javadoc # This is the commit message #9: review comments # This is the commit message #10: comments and clarification for CRAMBAIIndexer

…Slices - called on Container write now as well

cmnbroad

This is looking better and better. I proposed a couple more method consolidations that I think are simplifying - haven't looked at tests yet though, since they may change based on the outcome of this round.

src/main/java/htsjdk/samtools/cram/build/ContainerFactory.java

cmnbroad · 2019-03-27T13:33:42Z

src/main/java/htsjdk/samtools/cram/build/CramContainerIterator.java

-        final long containerSizeInBytes = countingInputStream.getCount() - offset;
-
-        nextContainer.offset = offset;
-        offset += containerSizeInBytes;


This is cleaning up nicely.

src/main/java/htsjdk/samtools/cram/build/CramContainerHeaderIterator.java

cmnbroad · 2019-03-27T13:49:02Z

src/main/java/htsjdk/samtools/cram/build/CramContainerIterator.java

@@ -17,20 +17,14 @@
    private CountingInputStream countingInputStream;
    private Container nextContainer;
    private boolean eof = false;
-    private long offset = 0;


cmnbroad · 2019-03-27T14:10:21Z

src/main/java/htsjdk/samtools/CRAMBAIIndexer.java

        if (container == null || container.isEOF()) {
            return;
        }

+        container.distributeIndexingParametersToSlices();


It looks like there is no corresponding call to this in the CRAIIndexer ? But rather than add one there, I'd propose that we should actually remove this one, and just have validation checks in both the BAI and CRAI indexers, instead of actual distribute calls. In the current strategy, the slice index info is always propagated as soon as we have it (either on read or on write). If for any reason we ever reach this point in the code and it hasn't already been done, it indicates an error somewhere. Calling this here could mask that. I think this should just be a validation check, rather than an actual distribute operation. Also, I would make both BAI and CRAI validation container-level (and check all slices), rather than slice granularity, towards the goal of eventually making slice hidden from container consumers.

Good catch: yes this can be removed.

There is a validation check later on, in processAsSingleReferenceSlice() because that is where all BAI code paths end up.

Since the indexing values all live in Slice (currently) I feel that's the appropriate place to validate them. Maybe in the future that will change, but that's beyond the scope of this PR.

cmnbroad · 2019-03-27T14:46:52Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

+     */
+    void setSlices(final List<Slice> slices, final long byteOffset) {
+        this.slices = slices.toArray(new Slice[0]);
+        setByteOffsetInSlices(byteOffset);


This method has a byteoffset that it propagates to slices, but doesn't set it on the container itself (setByteOffset does that). I don't see any meaningful distinction now between setByteOffset and setByteOffsetInSlices. However the byteoffset is established, it should always be set on both the container and on the slices (if they exist) at the same time, in complete lockstep, so there should only be one method that always does both.

sounds good

cmnbroad · 2019-03-27T14:55:46Z

src/main/java/htsjdk/samtools/cram/structure/Container.java

-    public static Container initializeFromSlices(final List<Slice> containerSlices) {
+    public static Container initializeFromSlices(final List<Slice> containerSlices,
+                                                 final CompressionHeader compressionHeader,
+                                                 final long containerByteOffset) {


The only (non test) external caller to setSlices that exists has the slices, the containerByteOffset, and the compression header, and could call this method directly. I think setSlices and initializeFromSlices can be consolidated into a single method that does everything ?

…sContainer()

mini cleanup

cmnbroad

I've reviewed all the files now including tests, a few more changes requested.

cmnbroad · 2019-03-28T16:14:28Z

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java

+    public static Container readContainer(final Version version,
+                                          final InputStream inputStream,
+                                          final long containerByteOffset) {
+        Container container = readContainerInternal(version.major, inputStream, containerByteOffset);


This readContainer method can be made private now.

cmnbroad · 2019-03-28T16:17:14Z

src/main/java/htsjdk/samtools/cram/structure/Slice.java

+        }
+    }
+
+    public void craiIndexInitializationCheck() {


This could be private except for tests, so it could be package-protected/VisibleForTesting. But I'm not sure its useful to have separate methods for the BAI and CRAI validation anyway. They're only subtly different (which is hard to spot - I thought they were the same when I first saw them), but the slice is either ready for indexing or it isn't. Whether it fails for BAI or CRAI seems like a minor detail - you could remove a bunch of code and test code if they were consolidated into a single method that checked for everything.

I'll add a comment to make the difference clearer.

I'd prefer to keep them separate, since they both test for exactly what is needed under their own requirements.

I don't see why the distinction, or the increased complexity and code duplication, is useful - there is only one distributeIndexingParametersToSlices thats establishes these values. But ok.

cmnbroad · 2019-03-28T16:45:59Z

src/main/java/htsjdk/samtools/CRAMBAIIndexer.java

- * finish() is called at the end.
+ * 1) Building a bam index (BAI) while building the CRAM file
+ * 2) Building a bam index (BAI) from an existing CRAI file
+ *


There is a third pattern implemented in this file, which is creating an index from an existing CRAM file (see createIndex). But createIndex appears to be unused anywhere, except in CRAMFileBAIIndexerTest. Which means CRAMFileBAIIndexerTest is somewhat bogus in that its testing index streams that are not created using real code. (This is confirmed in my test inventory notes from a couple of days ago - I just didn't realize when I did that inventory that the method being used by this test was not actually used anywhere else).

Right - that's why I didn't include it. I can add it here with a note that it's unused.

src/main/java/htsjdk/samtools/CRAMBAIIndexer.java

src/test/java/htsjdk/samtools/cram/CRAIEntryTest.java

jmthibault79 added the cram label Mar 15, 2019

jmthibault79 commented Mar 15, 2019

View reviewed changes

src/main/java/htsjdk/samtools/cram/structure/ContainerIO.java Outdated Show resolved Hide resolved

jmthibault79 commented Mar 15, 2019

View reviewed changes

cmnbroad self-assigned this Mar 18, 2019

jmthibault79 force-pushed the jt_crai_consolidation branch from 68ed944 to 72b71cd Compare March 19, 2019 14:47

jmthibault79 requested a review from cmnbroad March 19, 2019 15:57

cmnbroad reviewed Mar 20, 2019

View reviewed changes

jmthibault79 commented Mar 20, 2019

View reviewed changes

cmnbroad reviewed Mar 20, 2019

View reviewed changes

jmthibault79 added 4 commits March 26, 2019 14:26

Add bai and crai index init checks to Slice

f1b63fe

comments and clarification for CRAMBAIIndexer

Remove multiple versions of Slice/Container getCRAIEntries()

c031882

- Encapsulate Container.byteOffset and distribute to Slices on set

populateSlicesAndIndexingParameters -> distributeIndexingParametersTo…

1e1f348

…Slices - called on Container write now as well

jmthibault79 force-pushed the jt_crai_consolidation branch from 59980f4 to 1e1f348 Compare March 26, 2019 19:24

jmthibault79 added 3 commits March 26, 2019 16:22

Update CRAMBAIIndexer and tests

5732bae

Change setSlices signature to take a List<Slice>

210c74c

cleanup

6738967

cmnbroad reviewed Mar 27, 2019

View reviewed changes

jmthibault79 added 5 commits March 27, 2019 13:28

ContainerFactory.buildContainer() comment

0f3ed2d

rm call to container.distributeIndexingParametersToSlices() in proces…

13be7ff

…sContainer()

rm extra param

e2a23ae

mini cleanup

can simply container byteOffset

f82ea60

more byteOffset

f05a6cb

cmnbroad requested changes Mar 28, 2019

View reviewed changes

jmthibault79 added 6 commits March 29, 2019 10:48

private readContainer

e9d009f

init check comments

c334167

add comment to CRAMBAIIndexer about building BAI from CRAM

18b8262

rm javadoc link

4d5dff8

rm duplicate test code

9bcc358

DataProviders must be in classes which extend HtsJdkTest

325ab2a

cmnbroad approved these changes Mar 29, 2019

View reviewed changes

jmthibault79 merged commit aa89809 into master Apr 1, 2019

jmthibault79 deleted the jt_crai_consolidation branch April 1, 2019 14:51

Follow-up to #1276: Remove multiple versions of Slice/Container getCRAIEntries() #1329

Follow-up to #1276: Remove multiple versions of Slice/Container getCRAIEntries() #1329

Conversation

jmthibault79 commented Mar 15, 2019 • 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

codecov-io commented Mar 15, 2019 • 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

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

cmnbroad Mar 28, 2019 • 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

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

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 left a comment

Choose a reason for hiding this comment

cmnbroad Mar 28, 2019 • 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

jmthibault79 commented Mar 15, 2019 •

edited

Loading

codecov-io commented Mar 15, 2019 •

edited

Loading

cmnbroad Mar 28, 2019 •

edited

Loading

cmnbroad Mar 28, 2019 •

edited

Loading