Tests for CollectOxoGMetrics, CollectJumpingLibraryMetrics and BamTBfq classes #694
Conversation
There was a problem hiding this comment.
@ekazachkova Look's great, just a few minor changes requested.
| Assert.assertEquals(metrics.JUMP_DUPLICATE_PAIRS, 1); | ||
| Assert.assertEquals(metrics.JUMP_DUPLICATE_PCT, 0.25); | ||
| Assert.assertEquals(metrics.JUMP_LIBRARY_SIZE, 6); | ||
| Assert.assertEquals((int)metrics.JUMP_MEAN_INSERT_SIZE, 176); |
There was a problem hiding this comment.
It's not a good idea to cast inside of an assert.
This should be something more like...
Assert.assertEquals(metrics.JUMP_MEAN_INSERT_SIZE, 176.0);
| Assert.assertEquals((int)metrics.NONJUMP_DUPLICATE_PCT, 0); | ||
| Assert.assertEquals(metrics.NONJUMP_LIBRARY_SIZE, 0); | ||
| Assert.assertEquals((int)metrics.NONJUMP_MEAN_INSERT_SIZE, 96); | ||
| Assert.assertEquals((int)metrics.NONJUMP_STDEV_INSERT_SIZE, 0); |
There was a problem hiding this comment.
NONJUMP_STDEV_INSERT_SIZE evaluates to NaN, by casting it to int it becomes a 0. Instead you should check to see that it is NaN.
| Assert.assertEquals(metrics.NONJUMP_DUPLICATE_PAIRS, 0); | ||
| Assert.assertEquals((int)metrics.NONJUMP_DUPLICATE_PCT, 0); | ||
| Assert.assertEquals(metrics.NONJUMP_LIBRARY_SIZE, 0); | ||
| Assert.assertEquals((int)metrics.NONJUMP_MEAN_INSERT_SIZE, 96); |
| final MetricsFile<CollectOxoGMetrics.CpcgMetrics, Comparable<?>> output = new MetricsFile<>(); | ||
| output.read(new FileReader(outputFile)); | ||
|
|
||
| final CollectOxoGMetrics.CpcgMetrics cpcgMetrics = output.getMetrics().get(4); |
There was a problem hiding this comment.
It's not clear what index 4 refers to, could you create a variable with a useful name?
|
Done. @fleharty have a look, please. |
There was a problem hiding this comment.
@ekazachkova Great job! Thanks for addressing all the comments!
|
@ekazachkova Feel free to rebase, squash, and merge! |
|
@fleharty you should merge if you are sure it's fine.
…On Tue, Jan 24, 2017 at 3:20 PM, Mark Fleharty ***@***.***> wrote:
@ekazachkova <https://github.com/ekazachkova> Feel free to rebase,
squash, and merge!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#694 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACnk0rTOg7_k31Hb4JdZe5ZfLqPnyx2-ks5rVl0NgaJpZM4LBoFP>
.
|
ekazachkova
force-pushed
the
ls-epam_increased_test_coverage
branch
from
41797cf
to
3c0597f
Compare
|
Thanks! @fleharty you can merge. |
Current version Picard Tools doesn't contain test suite for CollectOxoGMetrics, CollectJumpingLibraryMetrics and BamToBfq classes. We represent PR that contains acceptance tests for this classes and new test data for BamToBfq class.