Permalink
Browse files

BWA.t, Bowtie.t, Maq.t & Samtools.t: Moved tests using

"_translate_params" subroutine (translate opts to command line)
into TODO blocks, since they randomly fail
because of hash randomization introduced in perl 5.17+.
Used the TODO message from WBCommandExts.t for consistency.
  • Loading branch information...
1 parent 35bb7f1 commit a6d0dc23d6d1b726be228f9635d8b61b3c6c87ce @fjossandon fjossandon committed Dec 13, 2013
Showing with 23 additions and 9 deletions.
  1. +6 −2 t/BWA.t
  2. +6 −2 t/Bowtie.t
  3. +6 −2 t/Maq.t
  4. +5 −3 t/Samtools.t
View
@@ -71,8 +71,12 @@ is_deeply( $bwafac->{_options}->{_prefixes},
is_deeply( $bwafac->{_options}->{_params},
[qw( command max_edit_dist max_gap_opens max_gap_extns deln_protect_3p deln_protect_ends subseq_seed max_edit_dist_seed n_threads mm_penalty gap_open_penalty gap_extn_penalty subopt_hit_threshold trim_parameter )],
"commands filtered by prefix");
-is( join(' ', @{$bwafac->_translate_params}),
- "aln -R 35 -t 1", "translate params" );
+
+TODO: {
+ local $TODO ='Determine whether the order of the parameters should be set somehow; this sporadically breaks hash randomization introduced in perl 5.17+';
+ is( join(' ', @{$bwafac->_translate_params}),
+ "aln -R 35 -t 1", "translate params" );
+}
# test run_bwa filearg parsing
# a pipeline...
View
@@ -108,8 +108,12 @@ is_deeply( $bowtiefac->{_options}->{_params},
defaul_mapq sam_rg suppress_columns alignmed_file
unaligned_file excess_file threads offrate random_seed )],
"commands filtered by prefix");
-is( join(' ', @{$bowtiefac->_translate_params}),
- "-v 4 --solexa-quals -y -S", "translate params" ); # we default to SAM so '-S' appears
+
+TODO: {
+ local $TODO ='Determine whether the order of the parameters should be set somehow; this sporadically breaks hash randomization introduced in perl 5.17+';
+ is( join(' ', @{$bowtiefac->_translate_params}),
+ "-v 4 --solexa-quals -y -S", "translate params" ); # we default to SAM so '-S' appears
+}
# test run_bowtie filearg parsing
View
@@ -70,8 +70,12 @@ is_deeply( $maqfac->{_options}->{_prefixes},
is_deeply( $maqfac->{_options}->{_params},
[qw( command error_dep_coeff het_fraction max_mismatches max_quality_sum min_map_quality num_haplotypes)],
"commands filtered by prefix");
-is( join(' ', @{$maqfac->_translate_params}),
- "assemble -m 4 -r 0.005 -s", "translate params" );
+
+TODO: {
+ local $TODO ='Determine whether the order of the parameters should be set somehow; this sporadically breaks hash randomization introduced in perl 5.17+';
+ is( join(' ', @{$maqfac->_translate_params}),
+ "assemble -m 4 -r 0.005 -s", "translate params" );
+}
# test run_maq filearg parsing
# a pipeline...
View
@@ -65,9 +65,12 @@ is_deeply( $samt->{_options}->{_prefixes},
is_deeply( $samt->{_options}->{_params},
[qw( command refseq map_qcap ref_list site_list theta n_haplos exp_hap_diff indel_prob )],
"commands filtered by prefix");
-is( join(' ', @{$samt->_translate_params}),
- "pileup -T 0.05 -f my.fas", "translate params" );
+TODO: {
+ local $TODO ='Determine whether the order of the parameters should be set somehow; this sporadically breaks hash randomization introduced in perl 5.17+';
+ is( join(' ', @{$samt->_translate_params}),
+ "pileup -T 0.05 -f my.fas", "translate params" );
+}
SKIP : {
test_skip( -requires_executable => $samt,
@@ -110,4 +113,3 @@ SKIP : {
unlink('sorted_bam');
}
-

9 comments on commit a6d0dc2

@majensen
Member

Francisco, I think I can fix this (be able to remove todos) by using Tie::IxHash in Bio::Tools::Run::WrapperBase (in bioperl-live). Want me to try?

@fjossandon
Member

Thanks for the offering Mark!!

If you want to do it please go ahead, but before adding work on you, I would like to ask the TODO question: 'Determine whether the order of the parameters should be set somehow'. I think that the test have to prove that all passed arguments are there to be used by when the program called was run (necessary to debug if something went wrong with the program), but is it really necessary that parameters are ordered?? Maybe just a "sort{$a cmp $b}" inside "_translate_params" before returning the array would be enough to return a consistent string between runs without adding a new dependency??

Also, in WrapperBase the input parameters don't come from an literal string like 'aln -R 35 -t 1' (in which case it would probably be desirable that both input and _translate_params string would be equal for comparison purpose), it comes from an object with arguments:
$bwafac->set_parameters(
-command => 'aln',
-n_threads => 1,
-subopt_hit_threshold => 35
);
Then, if Tie::IxHash is used, then the order of "_translate_params" would be the same than the order written above? Also, that same order would also be applied to "_run" when the string is created to pass the arguments to the program that will run?

What do you think? =)

@majensen
Member

Thanks Francisco -- I think you may be right. The order shouldn't matter (as long as the options and their values are kept together). So this can probably be fixed by just doing the right test -- not changing the code at all. This:
is( join(' ', @{$samt->_translate_params}), "pileup -T 0.05 -f my.fas", "translate params" );
is too simplistic, eh? I will think some more.

@majensen
Member

Thinking more, I think that test was pretty naive. Maybe lucky that it ever passed!

@majensen
Member

Ok Francisco -
I did a test with perl 5.18, and the test as written fails about 50% of the time.
The following always succeeds:

   my @a = @{$maqfac->_translate_params};
    is shift @a, 'assemble';
    shift @a;
    my ($k,%h);
    for (@a) {
        if (/^-/) {
            $h{$k = $_} = undef;
        }
        else {
            $h{$k} = $_;
        }
    }
    is_deeply( \%h, { '-m' => 4, '-r' => 0.005, '-s' => undef });`

It's ugly, but it doesn't depend on the order!

@fjossandon
Member

I think using hashes like that is the solution Mark. Your second "shift @a" is making it lose the first argument after 'assembly' though. How about using a ternary to get it more compact? =)

my @a = @{$maqfac->_translate_params};
is shift @a, 'assemble';
my ($k, %h);
for (@a) {
      (/^-/) ? ( $h{$k = $_} = undef )
    :          ( $h{$k}      = $_    );
}
is_deeply( \%h, { '-m' => 4, '-r' => 0.005, '-s' => undef });

I say that you go for it!!! "_translate_params" does not return the command value in some cases though (like the Bowtie.t), so the "shift @a" should be omitted in those cases. Besides these 4 files, "bioperl-live/t/Tools/Run/WBCommandExts.t" will need the fix too.

@majensen
Member
@majensen
Member

Ok - done at commit 0fca574
MAJ

@fjossandon
Member

That's great! Thanks Mark! =D 👍

Please sign in to comment.