Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 4 CRAM compliance tests from htslib #1185

Merged
merged 5 commits into from
Oct 5, 2018
Merged

Conversation

jmthibault79
Copy link
Contributor

@jmthibault79 jmthibault79 commented Oct 4, 2018

Description

Looking at htslib CRAM round trip, I noticed 6 that were missing from CramComplianceTest. These 4 pass with Partial Verification but need further investigation. Two others c2#pad and md#1 are not included here because they do not pass Partial Verification.

CRAM files generated using samtools view

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)

@jmthibault79 jmthibault79 requested a review from cmnbroad October 4, 2018 17:13
@codecov-io
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

Merging #1185 into master will increase coverage by 0.009%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##             master     #1185       +/-   ##
==============================================
+ Coverage      68.4%   68.409%   +0.009%     
- Complexity     8014      8030       +16     
==============================================
  Files           541       542        +1     
  Lines         32690     32769       +79     
  Branches       5529      5537        +8     
==============================================
+ Hits          22360     22417       +57     
- Misses         8126      8147       +21     
- Partials       2204      2205        +1
Impacted Files Coverage Δ Complexity Δ
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java 62.162% <0%> (-2.703%) 6% <0%> (-1%)
.../htsjdk/samtools/util/ProgressLoggerInterface.java 0% <0%> (ø) 0% <0%> (?)
...rc/main/java/htsjdk/samtools/util/BinaryCodec.java 69.686% <0%> (+0.42%) 70% <0%> (+15%) ⬆️
src/main/java/htsjdk/samtools/SAMFileHeader.java 68.263% <0%> (+1.198%) 45% <0%> (+1%) ⬆️
...a/htsjdk/samtools/util/AbstractProgressLogger.java 45.902% <0%> (+5.517%) 7% <0%> (+1%) ⬆️

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmthibault79 Adding these test seems like a good idea. Since all of the existing tests in the partialVerification provider have corresponding tickets and a comment that explains the why they fail full verification, can we do that for the new ones ? Otherwise I feel like we're just adding more mystery. Especially if any of them fail 2.1 round tripping but succeed for 3.0. I'd lobby to not spend time on 2.1 round tripping failures, and just focus on 3.0.

As for c2#pad and md#1, I'd say the same thing (add them as tests, completely commented out since they fail even partial verification, and add a ticket the failure). Otherwise lets just delete them iif they're unreferenced.

@jmthibault79
Copy link
Contributor Author

c2#pad passes fully with regenerated CRAM files, so I added it here.

Added links to issues to the 5 new failing tests, including md#1

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding those @jmthibault79. @lbergelson this LGTM for merging once the tests pass.

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. 👍

@jmthibault79 jmthibault79 merged commit 23f3223 into master Oct 5, 2018
@jmthibault79 jmthibault79 deleted the jt_verification branch October 5, 2018 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants