/htsjdk

Improved SRA tests, corrected error message #638

Merged
merged 2 commits into from Jul 29, 2016
Commits
Jump to file or symbol
Failed to load files and symbols.
+82 −45
Split
View
1 build.gradle
@@ -129,6 +129,7 @@ task testSRA(type: Test) {
description "Run the SRA tests"
useTestNG {
+ configFailurePolicy 'continue'
includeGroups "sra"
}
}
View
3 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" +
@yfarjoun

yfarjoun Jun 10, 2016

Contributor

isn't it clear at some point in the code that a connection has been established but that the accession is bad? why not inform the user?
+ "Possible causes are an invalid SRA accession or a connection problem.");
}
try {
View
4 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;
View
34 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) {
@yfarjoun

yfarjoun Jul 14, 2016

Contributor

Since you are no longer asking to connect for every accession, wouldn't it be better to remove the first condition, so that if someone puts in a non-matching accession to a new test, it will fail?

@a-nikitiuk

a-nikitiuk Jul 14, 2016

Contributor

If someone puts a non-matching accession, then this code will do nothing. However, it detects remote accessions from its first argument, and if remote accession is detected and canResolveNetworkAccession is false then it skips the test.

@yfarjoun

yfarjoun Jul 14, 2016

Contributor

Oh, OK. can you then simply change the text in the Exception to:

  1. note the accession that cannot be resolved, and
  2. change "SRA accession" with "remote SRA accession"?
+ 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.
View
3 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 },
View
2 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");
@yfarjoun

yfarjoun Jun 10, 2016

Contributor

why were these changed? what's wrong with the old ones?

@yfarjoun

yfarjoun Jun 10, 2016

Contributor

sorry, I see the comment now: I supposed these are the smaller files.

@a-nikitiuk

a-nikitiuk Jun 10, 2016

Contributor

yes, that is the reason
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];
View
2 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() {
View
78 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);