ExperimentalReadShardBalancerUnitTest was being skipped; fixed

TestNG skips tests when an exception occurs in a data provider,
which is what was happening here.

This was due to an AWFUL AWFUL use of a non-final static for
ReadShard.MAX_READS. This is fine if you assume only one instance
of SAMDataSource, but with multiple tests creating multiple SAMDataSources,
and each one overwriting ReadShard.MAX_READS, you have a recipe for
problems. As a result of this the test ran fine individually, but not as
part of the unit test suite.

Quick fix for now to get the tests running -- this "mutable static"
interface should really be refactored away though, when I have time.
This commit is contained in:
David Roazen 2012-09-22 01:05:40 -04:00
parent e077347cc2
commit f6a22e5f50
3 changed files with 20 additions and 6 deletions

View File

@ -34,10 +34,21 @@ import java.util.*;
* @version 0.1 * @version 0.1
*/ */
public class ReadShard extends Shard { public class ReadShard extends Shard {
/**
* Default read shard buffer size
*/
public static final int DEFAULT_MAX_READS = 10000;
/** /**
* What is the maximum number of reads per BAM file which should go into a read shard. * What is the maximum number of reads per BAM file which should go into a read shard.
*
* TODO: this non-final static variable should either be made final or turned into an
* TODO: instance variable somewhere -- as both static and mutable it wreaks havoc
* TODO: with tests that use multiple instances of SAMDataSource (since SAMDataSource
* TODO: changes this value)
*/ */
public static int MAX_READS = 10000; public static int MAX_READS = DEFAULT_MAX_READS;
/** /**
* The reads making up this shard. * The reads making up this shard.
@ -51,6 +62,9 @@ public class ReadShard extends Shard {
/** /**
* Sets the maximum number of reads buffered in a read shard. Implemented as a weirdly static interface * Sets the maximum number of reads buffered in a read shard. Implemented as a weirdly static interface
* until we know what effect tuning this parameter has. * until we know what effect tuning this parameter has.
*
* TODO: this mutable static interface is awful and breaks tests -- need to refactor
*
* @param bufferSize New maximum number * @param bufferSize New maximum number
*/ */
static void setReadBufferSize(final int bufferSize) { static void setReadBufferSize(final int bufferSize) {

View File

@ -252,7 +252,7 @@ public class SAMDataSource {
validationStringency = strictness; validationStringency = strictness;
this.removeProgramRecords = removeProgramRecords; this.removeProgramRecords = removeProgramRecords;
if(readBufferSize != null) if(readBufferSize != null)
ReadShard.setReadBufferSize(readBufferSize); ReadShard.setReadBufferSize(readBufferSize); // TODO: use of non-final static variable here is just awful, especially for parallel tests
else { else {
// Choose a sensible default for the read buffer size. For the moment, we're picking 1000 reads per BAM per shard (which effectively // Choose a sensible default for the read buffer size. For the moment, we're picking 1000 reads per BAM per shard (which effectively
// will mean per-thread once ReadWalkers are parallelized) with a max cap of 250K reads in memory at once. // will mean per-thread once ReadWalkers are parallelized) with a max cap of 250K reads in memory at once.

View File

@ -90,7 +90,7 @@ public class ExperimentalReadShardBalancerUnitTest extends BaseTest {
new GenomeLocParser(header.getSequenceDictionary()), new GenomeLocParser(header.getSequenceDictionary()),
false, false,
SAMFileReader.ValidationStringency.SILENT, SAMFileReader.ValidationStringency.SILENT,
null, ReadShard.DEFAULT_MAX_READS, // reset ReadShard.MAX_READS to ReadShard.DEFAULT_MAX_READS for each test
downsamplingMethod, downsamplingMethod,
new ValidationExclusion(), new ValidationExclusion(),
new ArrayList<ReadFilter>(), new ArrayList<ReadFilter>(),
@ -180,10 +180,10 @@ public class ExperimentalReadShardBalancerUnitTest extends BaseTest {
for ( int numContigs = 1; numContigs <= 3; numContigs++ ) { for ( int numContigs = 1; numContigs <= 3; numContigs++ ) {
for ( int numStacksPerContig : Arrays.asList(1, 2, 4) ) { for ( int numStacksPerContig : Arrays.asList(1, 2, 4) ) {
// Use crucial read shard boundary values as the stack sizes // Use crucial read shard boundary values as the stack sizes
for ( int stackSize : Arrays.asList(ReadShard.MAX_READS / 2, ReadShard.MAX_READS / 2 + 10, ReadShard.MAX_READS, ReadShard.MAX_READS - 1, ReadShard.MAX_READS + 1, ReadShard.MAX_READS * 2) ) { for ( int stackSize : Arrays.asList(ReadShard.DEFAULT_MAX_READS / 2, ReadShard.DEFAULT_MAX_READS / 2 + 10, ReadShard.DEFAULT_MAX_READS, ReadShard.DEFAULT_MAX_READS - 1, ReadShard.DEFAULT_MAX_READS + 1, ReadShard.DEFAULT_MAX_READS * 2) ) {
for ( int numUnmappedReads : Arrays.asList(0, ReadShard.MAX_READS / 2, ReadShard.MAX_READS * 2) ) { for ( int numUnmappedReads : Arrays.asList(0, ReadShard.DEFAULT_MAX_READS / 2, ReadShard.DEFAULT_MAX_READS * 2) ) {
// The first value will result in no downsampling at all, the others in some downsampling // The first value will result in no downsampling at all, the others in some downsampling
for ( int downsamplingTargetCoverage : Arrays.asList(ReadShard.MAX_READS * 10, ReadShard.MAX_READS, ReadShard.MAX_READS / 2) ) { for ( int downsamplingTargetCoverage : Arrays.asList(ReadShard.DEFAULT_MAX_READS * 10, ReadShard.DEFAULT_MAX_READS, ReadShard.DEFAULT_MAX_READS / 2) ) {
new ExperimentalReadShardBalancerTest(numContigs, numStacksPerContig, stackSize, numUnmappedReads, downsamplingTargetCoverage); new ExperimentalReadShardBalancerTest(numContigs, numStacksPerContig, stackSize, numUnmappedReads, downsamplingTargetCoverage);
} }
} }