Closed jmarshall closed 6 years ago
Interestingly, travis is happy will all tested combinations of perl and htslib except 5.26/1.5 where the cram tests fail. Could you try reproducing with the same setting?
For completeness, would it be possible to add similar tests in the cram/sam case? It's fine if it's not.
Will have a look at both of these.
Marvelous, thanks :+1:
I've added CRAM tests, but coverage is not applicable for SAM as the required -seq_id
option requires an index.
Nothing has been changed (only a few tests added), yet travis is now happy even on the Perl 5.26/htslib 1.5 combination.
Running the t/03cram.t tests under valgrind shows a double-free bug (the cram index is freed via both hts_close
and hts_idx_destroy
), present also on the master branch and with any version of HTSlib, that occasionally leads to a segfault (only once in my attempts to reproduce this). I suspect it is this that caused the failed travis run, which ran the t/03cram.t tests twice: once successfully (via ./Build test
) and once with a segfault (via travisci/harness.sh
).
That's excellent @jmarshall :+1: To be honest, I didn't and still don't understand the reason why two sets of tests should be executed on travis. At the same time, this has proven to be useful since it's exposed the bug. Your observation above reminds me of some additional tests I had thought to add when initially looking at the test suite, namely using Test::LeakTrace which I suspect acts on behalf of valgrind.
Thanks again for you contribution, this is good to go for me.
At present, using
features(-type=>'coverage', -filter => sub { … })
ends up with your filter subroutine being silently ignored. This fixes that, implementing it as a filter that is called once for each read that (now optionally) goes into the pileup from which coverage is calculated.Also some minor warning and leak fixes. This took rather longer than I anticipated as it took a long time to figure out what the
bam_dup1()
inhts_fetch_fun()
was all about, so I have expanded on the comments there to hopefully clarify this.