Improved SRA tests, corrected error message #638

a-nikitiuk · 2016-06-07T23:44:16Z

Description

These changes are intended to resolve most of previously reported SRA-related issues:

SRA test creating a huge local file/cache #442 - updated tests to use much smaller files, so that footprint by SRA cache on Mac OS X is about 30 Mb (on other OS issue does not exist, since we use sparse files there and footprint is even smaller)
Disable failing SRA tests until they can be made more robust #459, Fix inexplicable SRA test failures #461 - now each network-related test checks if it can resolve an accession, and skips the test in case of failure
Improve error messages when instantiating an SRAFileReader #460, SRA tests fail with missing accession #545 - updated error message in SRAFileReader, so that it tells about a possibility of a connection problem

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)

coveralls · 2016-06-08T00:35:07Z

Coverage decreased (-0.006%) to 68.053% when pulling 5d75514 on a-nikitiuk:sra-fixes into e38b849 on samtools:master.

yfarjoun · 2016-06-10T04:40:48Z

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" +


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?

yfarjoun · 2016-06-10T04:49:08Z

@a-nikitiuk back to you. only one comment really: can you not distinguish between a bad connection and a bad accesssion string? It seems that some part of the code must know, so perhaps you can give an return code to isValid() that will be more than a boolean and work with that. The problem is that if the accession becomes "bad" because of code change we will never find out because we will skip the test.

a-nikitiuk · 2016-06-10T15:06:23Z

Unfortunately, it is not possible in current version of NGS API. We are working on improvement, but that is not an easy and quick fix.

yfarjoun · 2016-06-10T20:33:35Z

In that case @a-nikitiuk I think it would be better to test connectivity with a single @BeforeGroup using an accession that is bound to exist (perhaps you have some sort of testing object that can be used for that?) and then remove the @BeforeTest so that each of the individual tests will fail if their accession is not valid. while this is fragile wrt losing connectivity in the middle of running the tests, it doesn't require a separate check that all the accessions are valid.

yfarjoun · 2016-06-28T18:47:05Z

@a-nikitiuk This needs to be rebased, and also did you have time to think about my suggestion of using @BeforeGroup rather than @BeforeTest ?

yfarjoun · 2016-07-12T18:31:54Z

@a-nikitiuk will you have time to rebase and replace @BeforeMethod with @BeforeTest ?

We would love to have these changes....

a-nikitiuk · 2016-07-13T16:58:53Z

Yeah, will do that. Sorry, did not have much time lately

a-nikitiuk · 2016-07-13T17:49:53Z

Rebased and added @BeforeGroups method

coveralls · 2016-07-13T18:20:48Z

Coverage increased (+0.006%) to 68.416% when pulling 2fcb11c on a-nikitiuk:sra-fixes into e7c7bf6 on samtools:master.

yfarjoun · 2016-07-14T03:00:14Z

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

+        }
+
+        if (accession != null &&
+                accession.matches(SRAAccession.REMOTE_ACCESSION_PATTERN) && !canResolveNetworkAccession) {


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?

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.

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

note the accession that cannot be resolved, and

change "SRA accession" with "remote SRA accession"?

yfarjoun · 2016-07-26T18:18:11Z

@a-nikitiuk are you planing to make these changes, or should I do them?

a-nikitiuk · 2016-07-26T18:20:34Z

You can do them if you would like to. Or I will try to find time for that today-tomorrow

yfarjoun · 2016-07-26T23:07:48Z

OK. I'll leave it to you then! thanks!

On Tue, Jul 26, 2016 at 2:20 PM, a-nikitiuk [email protected]
wrote:

You can do them if you would like to. Or I will try to find time for that
today-tomorrow

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#638 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACnk0o2DcApbHYeMg6Q2-lK8vfCBKtdtks5qZk_0gaJpZM4Iwd2p
.

…23 accession

a-nikitiuk · 2016-07-28T19:30:34Z

Done

coveralls · 2016-07-28T19:42:44Z

Coverage decreased (-0.006%) to 68.45% when pulling b780a55 on a-nikitiuk:sra-fixes into 87b1e87 on samtools:master.

yfarjoun · 2016-07-29T02:17:45Z

thanks! 👍

* Fixed SRA tests issues, corrected error message * Network related tests now depend on ability to resolve single SRR000123 accession

yfarjoun reviewed Jun 10, 2016
View reviewed changes

droazen assigned yfarjoun Jun 10, 2016

droazen mentioned this pull request Jun 28, 2016

Review "Party" #548

Closed

yfarjoun added the Review-party candidate label Jul 8, 2016

Fixed SRA tests issues, corrected error message

a060fd0

a-nikitiuk force-pushed the sra-fixes branch from 5d75514 to bbd1161 Compare July 13, 2016 17:48

a-nikitiuk force-pushed the sra-fixes branch from bbd1161 to 2fcb11c Compare July 13, 2016 18:07

yfarjoun reviewed Jul 14, 2016
View reviewed changes

Network related tests now depend on ability to resolve single SRR0001…

b780a55

…23 accession

a-nikitiuk force-pushed the sra-fixes branch from 2fcb11c to b780a55 Compare July 28, 2016 19:29

yfarjoun merged commit 0875b62 into samtools:master Jul 29, 2016

This was referenced Aug 23, 2016

SRA test creating a huge local file/cache #442

Closed

SRA tests fail with missing accession #545

Closed

Improve error messages when instantiating an SRAFileReader #460

Closed

This was referenced Aug 23, 2016

Fix inexplicable SRA test failures #461

Closed

Disable failing SRA tests until they can be made more robust #459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved SRA tests, corrected error message #638

Improved SRA tests, corrected error message #638

a-nikitiuk commented Jun 7, 2016 •

edited

Loading

coveralls commented Jun 8, 2016

yfarjoun Jun 10, 2016

yfarjoun commented Jun 10, 2016

a-nikitiuk commented Jun 10, 2016

yfarjoun commented Jun 10, 2016

yfarjoun commented Jun 28, 2016

yfarjoun commented Jul 12, 2016

a-nikitiuk commented Jul 13, 2016

a-nikitiuk commented Jul 13, 2016

coveralls commented Jul 13, 2016

yfarjoun Jul 14, 2016

a-nikitiuk Jul 14, 2016

yfarjoun Jul 14, 2016

yfarjoun commented Jul 26, 2016

a-nikitiuk commented Jul 26, 2016

yfarjoun commented Jul 26, 2016

a-nikitiuk commented Jul 28, 2016

coveralls commented Jul 28, 2016

yfarjoun commented Jul 29, 2016

Improved SRA tests, corrected error message #638

Improved SRA tests, corrected error message #638

Conversation

a-nikitiuk commented Jun 7, 2016 • edited Loading

Description

Checklist

coveralls commented Jun 8, 2016

yfarjoun Jun 10, 2016

Choose a reason for hiding this comment

yfarjoun commented Jun 10, 2016

a-nikitiuk commented Jun 10, 2016

yfarjoun commented Jun 10, 2016

yfarjoun commented Jun 28, 2016

yfarjoun commented Jul 12, 2016

a-nikitiuk commented Jul 13, 2016

a-nikitiuk commented Jul 13, 2016

coveralls commented Jul 13, 2016

yfarjoun Jul 14, 2016

Choose a reason for hiding this comment

a-nikitiuk Jul 14, 2016

Choose a reason for hiding this comment

yfarjoun Jul 14, 2016

Choose a reason for hiding this comment

yfarjoun commented Jul 26, 2016

a-nikitiuk commented Jul 26, 2016

yfarjoun commented Jul 26, 2016

a-nikitiuk commented Jul 28, 2016

coveralls commented Jul 28, 2016

yfarjoun commented Jul 29, 2016

a-nikitiuk commented Jun 7, 2016 •

edited

Loading