Improved SRA tests, corrected error message (#638)

* Fixed SRA tests issues, corrected error message

* Network related tests now depend on ability to resolve single SRR000123 accession

build.gradle

@@ -129,6 +129,7 @@ task testSRA(type: Test) {
 
     description "Run the SRA tests"
     useTestNG {
+        configFailurePolicy 'continue'
         includeGroups "sra"
     }
 }

src/main/java/htsjdk/samtools/SRAFileReader.java

@@ -61,7 +61,8 @@ public SRAFileReader(final SRAAccession acc) {
         this.acc = acc;
 
         if (!acc.isValid()) {
-            throw new IllegalArgumentException("Invalid SRA accession was passed to SRA reader: " + acc);
+            throw new IllegalArgumentException("SRAFileReader: cannot resolve SRA accession '" + acc + "'\n" +
+                "Possible causes are an invalid SRA accession or a connection problem.");
         }
 
         try {

src/main/java/htsjdk/samtools/sra/SRAAccession.java

@@ -54,6 +54,8 @@
     private final static String defaultAppVersionString = "[unknown software]";
     private final static String htsJdkVersionString = "HTSJDK-NGS";
 
+    static final String REMOTE_ACCESSION_PATTERN = "^[SED]RR[0-9]{6,9}$";
+
     private String acc;
 
     static {
@@ -127,7 +129,7 @@ public static boolean isValid(String acc) {
             // anything else local other than a file is not an SRA archive
             looksLikeSRA = false;
         } else {
-            looksLikeSRA = acc.toUpperCase().matches ( "^[SED]RR[0-9]{6,9}$" );
+            looksLikeSRA = acc.toUpperCase().matches ( REMOTE_ACCESSION_PATTERN );
         }
 
         if (!looksLikeSRA) return false;

src/test/java/htsjdk/samtools/sra/AbstractSRATest.java

@@ -4,21 +4,53 @@
 import htsjdk.samtools.SAMRecordIterator;
 import org.testng.Assert;
 import org.testng.SkipException;
+import org.testng.annotations.BeforeGroups;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+import java.lang.reflect.Method;
 import java.util.NoSuchElementException;
 
 @Test(groups = "sra")
 public abstract class AbstractSRATest {
+    private static boolean canResolveNetworkAccession = false;
+    private static String checkAccession = "SRR000123";
+
+    @BeforeGroups(groups = "sra")
+    public final void checkIfCanResolve() {
+        if (!SRAAccession.isSupported()) {
+            return;
+        }
+        canResolveNetworkAccession = SRAAccession.isValid(checkAccession);
+    }
 
     @BeforeMethod
-    public final void assertSRAIsSupported(){
+    public final void assertSRAIsSupported() {
         if(!SRAAccession.isSupported()){
             throw new SkipException("Skipping SRA Test because SRA native code is unavailable.");
         }
     }
 
+    @BeforeMethod
+    public final void skipIfCantResolve(Method method, Object[] params) {
+        String accession = null;
+
+        if (params.length > 0) {
+            Object firstParam = params[0];
+            if (firstParam instanceof String) {
+                accession = (String)firstParam;
+            } else if (firstParam instanceof SRAAccession) {
+                accession = firstParam.toString();
+            }
+        }
+
+        if (accession != null &&
+                accession.matches(SRAAccession.REMOTE_ACCESSION_PATTERN) && !canResolveNetworkAccession) {
+            throw new SkipException("Skipping network SRA Test because cannot resolve remote SRA accession '" +
+                    checkAccession + "'.");
+        }
+    }
+
     /**
      * Exhaust the iterator and check that it produce the expected number of mapped and unmapped reads.
      * Also checks that the hasNext() agrees with the actual results of next() for the given iterator.

src/test/java/htsjdk/samtools/sra/SRAAccessionTest.java

@@ -13,8 +13,7 @@
     private Object[][] getIsValidAccData() {
         return new Object[][] {
             { "SRR000123", true },
-            { "DRR000001", true },
-            { "SRR000000", false },
+            { "DRR010511", true },
             { "src/test/resources/htsjdk/samtools/sra/test_archive.sra", true },
             { "src/test/resources/htsjdk/samtools/compressed.bam", false },
             { "src/test/resources/htsjdk/samtools/uncompressed.sam", false },

src/test/java/htsjdk/samtools/sra/SRAIndexTest.java

@@ -46,7 +46,7 @@
  * Created by andrii.nikitiuk on 10/28/15.
  */
 public class SRAIndexTest extends AbstractSRATest {
-    private static final SRAAccession DEFAULT_ACCESSION = new SRAAccession("SRR1298981");
+    private static final SRAAccession DEFAULT_ACCESSION = new SRAAccession("SRR2096940");
     private static final int LAST_BIN_LEVEL = GenomicIndexUtil.LEVEL_STARTS.length - 1;
     private static final int SRA_BIN_OFFSET = GenomicIndexUtil.LEVEL_STARTS[LAST_BIN_LEVEL];
 

src/test/java/htsjdk/samtools/sra/SRALazyRecordTest.java

@@ -12,7 +12,7 @@
  * Tests for SRA extension of SAMRecord objects which load fields on demand
  */
 public class SRALazyRecordTest extends AbstractSRATest {
-    private static final SRAAccession DEFAULT_ACCESSION = new SRAAccession("SRR1298981");
+    private static final SRAAccession DEFAULT_ACCESSION = new SRAAccession("SRR2096940");
 
     @DataProvider(name = "serializationTestData")
     private Object[][] getSerializationTestData() {

src/test/java/htsjdk/samtools/sra/SRATest.java

@@ -101,13 +101,8 @@ public void testCountsBySpan(String acc, List<Chunk> chunks, int expectedNumMapp
     @DataProvider(name = "testGroups")
     private Object[][] createDataForGroups() {
         return new Object[][] {
-            {"SRR822962", new TreeSet<>(Arrays.asList(
-                    "GS54389-FS3-L08", "GS57511-FS3-L08", "GS54387-FS3-L02", "GS54387-FS3-L01",
-                    "GS57510-FS3-L01", "GS57510-FS3-L03", "GS54389-FS3-L07", "GS54389-FS3-L05",
-                    "GS54389-FS3-L06", "GS57510-FS3-L02", "GS57510-FS3-L04", "GS54387-FS3-L03",
-                    "GS46253-FS3-L03"))
-            },
-            {"SRR2096940", new HashSet<>(Arrays.asList("SRR2096940"))}
+            {"SRR1035115", new TreeSet<>(Arrays.asList("15656144_B09YG", "15656144_B09MR"))},
+            {"SRR2096940", new TreeSet<>(Arrays.asList("SRR2096940"))}
         };
     }
 
@@ -148,15 +143,17 @@ public void testGroups(String acc, Set<String> groups) {
     private Object[][] createDataForReferences() {
         return new Object[][] {
             // primary alignment only
-            {"SRR1063272", 1,
-                    Arrays.asList("supercont2.1", "supercont2.2", "supercont2.3", "supercont2.4",
-                                  "supercont2.5", "supercont2.6", "supercont2.7", "supercont2.8",
-                                  "supercont2.9", "supercont2.10", "supercont2.11", "supercont2.12",
-                                  "supercont2.13", "supercont2.14"),
-                    Arrays.asList(2291499, 1621675, 1575141, 1084805,
-                                  1814975, 1422463, 1399503, 1398693,
-                                  1186808, 1059964, 1561994, 774062,
-                                  756744, 926563)},
+            {"SRR353866", 9,
+                    Arrays.asList(
+                            "AAAB01001871.1", "AAAB01002233.1", "AAAB01004056.1", "AAAB01006027.1",
+                            "AAAB01008846.1", "AAAB01008859.1", "AAAB01008960.1", "AAAB01008982.1",
+                            "AAAB01008987.1"
+                    ),
+                    Arrays.asList(
+                            1115, 1034, 1301, 1007,
+                            11308833, 12516315, 23099915, 1015562,
+                            16222597
+                    )},
         };
     }
 
@@ -208,67 +205,66 @@ public void testReferences(String acc, int numberFirstReferenceFound, List<Strin
     private Object[][] createDataForRowsTest() {
         return new Object[][] {
             // primary alignment only
-            {"SRR1063272", 0, 99, "SRR1063272.R.1",
-                    "ACTCGACATTCTGCCTTCGACCTATCTTTCTCCTCTCCCAGTCATCGCCCAGTAGAATTACCAGGCAATGAACCAGGGCCTTCCATCCCAACGGCACAGCA",
-                    "@@CDDBDFFBFHFIEEFGIGGHIEHIGIGGFGEGAFDHIIIIIGGGDFHII;=BF@FEHGIEEH?AHHFHFFFFDC5'5=?CC?ADCD@AC??9BDDCDB<",
-                    86, "101M", "supercont2.1", 60, true, false},
+            {"SRR2127895", 1, 83, "SRR2127895.R.1",
+                    "CGTGCGCGTGACCCATCAGATGCTGTTCAATCAGTGGCAAATGCGGAACGGTTTCTGCGGGTTGCCGATATTCTGGAGAGTAATGCCAGGCAGGGGCAGGT",
+                    "DDBDDDDDBCABC@CCDDDC?99CCA:CDCDDDDDDDECDDDFFFHHHEGIJIIGIJIHIGJIJJJJJJJIIJIIHIGJIJJJIJJIHFFBHHFFFDFBBB",
+                    366, "29S72M", "gi|152968582|ref|NC_009648.1|", 147, true, false, false},
 
             // small SRA archive
             {"SRR2096940", 1, 16, "SRR2096940.R.3",
                     "GTGTGTCACCAGATAAGGAATCTGCCTAACAGGAGGTGTGGGTTAGACCCAATATCAGGAGACCAGGAAGGAGGAGGCCTAAGGATGGGGCTTTTCTGTCACCAATCCTGTCCCTAGTGGCCCCACTGTGGGGTGGAGGGGACAGATAAAAGTACCCAGAACCAGAG",
                     "AAAABFFFFFFFGGGGGGGGIIIIIIIIIIIIIIIIIIIIIIIIIIIIII7IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIGGGGGFGFFDFFFFFC",
-                    55627016, "167M", "CM000681.1", 42, false, false},
+                    55627016, "167M", "CM000681.1", 42, false, false, false},
 
             {"SRR2096940", 10591, 4, "SRR2096940.R.10592",
                     "CTCTGGTTCTGGGTACTTTTATCTGTCCCCTCCACCCCACAGTGGCGAGCCAGATTCCTTATCTGGTGACACAC",
                     "IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII",
-                    -1, null, null, -1, false, false},
+                    -1, null, null, -1, false, false, false},
 
             // primary and secondary alignments
             {"SRR833251", 81, 393, "SRR833251.R.51",
                     "ATGCAAATCCGAATGGGCTATTTGTGGGTACTTGGGCAGGTAAGTAGCTGGCAATCTTGGTCGGTAAACCAATACCCAAGTTCACATAGGCACCATCGGGA",
                     "CCCFFFFFHHHHHIJJJIJJJJJIIJJJGIJIJIIJIJJJDGIGIIJIJIHIJJJJJJGIGHIHEDFFFFDDEEEDDDDDCDEEDDDDDDDDDDDDDBBDB",
-                    1787186, "38M63S", "gi|169794206|ref|NC_010410.1|", 11, true, true},
+                    1787186, "38M63S", "gi|169794206|ref|NC_010410.1|", 11, true, true, true},
 
             // local SRA file
             {"src/test/resources/htsjdk/samtools/sra/test_archive.sra", 1, 99, "test_archive.R.2",
                     "TGTCGATGCTGAAAGTGTCTGCGGTGAACCACTTCATGCACAGCGCACACTGCAGCTCCACTTCACCCAGCTGACGGCCGTTCTCATCGTCTCCAGAGCCCGTCTGAGCGTCCGCTGCTTCAGAACTGTCCCCGGCTGTATCCTGAAGAC",
                     "BBAABBBFAFFFGGGGGGGGGGGGEEFHHHHGHHHHHFHHGHFDGGGGGHHGHHHHHHHHHHHHFHHHGHHHHHHGGGGGGGHGGHHHHHHHHHGHHHHHGGGGHGHHHGGGGGGGGGHHHHEHHHHHHHHHHGCGGGHHHHHHGBFFGF",
-                    2811570, "150M", "NC_007121.5", 60, true, false}
+                    2811570, "150M", "NC_007121.5", 60, true, false, false}
         };
     }
 
     @Test(dataProvider = "testRows")
     public void testRows(String acc, int recordIndex, int flags, String readName, String bases, String quals, int refStart, String cigar,
-                         String refName, int mapQ, boolean hasMate, boolean isSecondaryAlignment) {
+                         String refName, int mapQ, boolean hasMate, boolean isSecondOfPair, boolean isSecondaryAlignment) {
         SAMRecord record = getRecordByIndex(acc, recordIndex, false);
 
-        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondaryAlignment);
+        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondOfPair, isSecondaryAlignment);
     }
 
     @Test(dataProvider = "testRows")
     public void testRowsAfterIteratorDetach(String acc, int recordIndex, int flags, String readName, String bases, String quals,
                                             int refStart, String cigar, String refName, int mapQ, boolean hasMate,
-                                            boolean isSecondaryAlignment) {
+                                            boolean isSecondOfPair, boolean isSecondaryAlignment) {
         SAMRecord record = getRecordByIndex(acc, recordIndex, true);
 
-        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondaryAlignment);
+        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondOfPair, isSecondaryAlignment);
     }
 
     @Test(dataProvider = "testRows")
     public void testRowsOverrideValues(String acc, int recordIndex, int flags, String readName, String bases, String quals,
                                        int refStart, String cigar, String refName, int mapQ, boolean hasMate,
-                                       boolean isSecondaryAlignment) {
+                                       boolean isSecondOfPair, boolean isSecondaryAlignment) {
         SAMRecord record = getRecordByIndex(acc, recordIndex, true);
         SAMFileHeader header = record.getHeader();
 
-
         record.setFlags(0);
         record.setReadUnmappedFlag(refStart == -1);
         record.setReadBases("C".getBytes());
         record.setBaseQualities(SAMUtils.fastqToPhred("A"));
         if (refStart == -1) {
-            checkSAMRecord(record, 4, readName, "C", "A", refStart, "1M", refName, mapQ, false, false);
+            checkSAMRecord(record, 4, readName, "C", "A", refStart, "1M", refName, mapQ, false, false, false);
         } else {
             int sequenceIndex = header.getSequenceIndex(refName);
             Assert.assertFalse(sequenceIndex == -1);
@@ -288,14 +284,14 @@ public void testRowsOverrideValues(String acc, int recordIndex, int flags, Strin
             record.setMappingQuality(mapQ - 1);
             record.setReferenceIndex(sequenceIndex);
 
-            checkSAMRecord(record, 0, readName, "C", "A", refStart - 100, "1M", refName, mapQ - 1, false, false);
+            checkSAMRecord(record, 0, readName, "C", "A", refStart - 100, "1M", refName, mapQ - 1, false, false, false);
         }
     }
 
     @Test(dataProvider = "testRows")
     public void testRowsBySpan(String acc, int recordIndex, int flags, String readName, String bases, String quals,
                                             int refStart, String cigar, String refName, int mapQ, boolean hasMate,
-                                            boolean isSecondaryAlignment) {
+                                            boolean isSecondOfPair, boolean isSecondaryAlignment) {
         SamReader reader = SamReaderFactory.make().validationStringency(ValidationStringency.SILENT).open(
                 SamInputResource.of(new SRAAccession(acc))
         );
@@ -330,13 +326,13 @@ public void testRowsBySpan(String acc, int recordIndex, int flags, String readNa
             }
         }
 
-        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondaryAlignment);
+        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondOfPair, isSecondaryAlignment);
     }
 
     @Test(dataProvider = "testRows")
     public void testRowsByIndex(String acc, int recordIndex, int flags, String readName, String bases, String quals,
                                 int refStart, String cigar, String refName, int mapQ, boolean hasMate,
-                                boolean isSecondaryAlignment) {
+                                boolean isSecondOfPair, boolean isSecondaryAlignment) {
         SamReader reader = SamReaderFactory.make().validationStringency(ValidationStringency.SILENT).open(
                 SamInputResource.of(new SRAAccession(acc))
         );
@@ -366,13 +362,15 @@ public void testRowsByIndex(String acc, int recordIndex, int flags, String readN
                 continue;
             }
 
-            if (currentRecord.getReadName().equals(readName)) {
+            if (currentRecord.getReadName().equals(readName)
+                    && currentRecord.getNotPrimaryAlignmentFlag() == isSecondaryAlignment
+                    && (!hasMate || currentRecord.getSecondOfPairFlag() == isSecondOfPair)) {
                 record = currentRecord;
                 break;
             }
         }
 
-        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondaryAlignment);
+        checkSAMRecord(record, flags, readName, bases, quals, refStart, cigar, refName, mapQ, hasMate, isSecondOfPair, isSecondaryAlignment);
     }
 
     private SAMRecord getRecordByIndex(String acc, int recordIndex, boolean detach) {
@@ -401,19 +399,23 @@ private SAMRecord getRecordByIndex(String acc, int recordIndex, boolean detach)
 
     private void checkSAMRecord(SAMRecord record, int flags, String readName, String bases, String quals,
                                 int refStart, String cigar, String refName, int mapQ, boolean hasMate,
-                                boolean isSecondaryAlignment) {
+                                boolean isSecondOfPair, boolean isSecondaryAlignment) {
 
         Assert.assertNotNull(record, "Record with read id: " + readName + " was not found by span created from index");
 
         List<SAMValidationError> validationErrors = record.isValid();
         Assert.assertNull(validationErrors, "SRA Lazy record is invalid. List of errors: " +
                 (validationErrors != null ? validationErrors.toString() : ""));
 
+        Assert.assertEquals(record.getReadName(), readName);
         Assert.assertEquals(new String(record.getReadBases()), bases);
         Assert.assertEquals(record.getBaseQualityString(), quals);
         Assert.assertEquals(record.getReadPairedFlag(), hasMate);
         Assert.assertEquals(record.getFlags(), flags);
         Assert.assertEquals(record.getNotPrimaryAlignmentFlag(), isSecondaryAlignment);
+        if (hasMate) {
+            Assert.assertEquals(record.getSecondOfPairFlag(), isSecondOfPair);
+        }
         if (refStart == -1) {
             Assert.assertEquals(record.getReadUnmappedFlag(), true);
             Assert.assertEquals(record.getAlignmentStart(), 0);