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

Fix an NPE in CRAMRecordReadFeatures.restoreReadBases #1655

Merged
merged 3 commits into from
Feb 21, 2023

Conversation

lbergelson
Copy link
Member

@cmnbroad This is a bug related to handling embedded references.

Copy link
Contributor

@droazen droazen left a comment

Choose a reason for hiding this comment

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

👍 merge after addressing minor comments

final SAMRecord cramRecordEmbedded = cramEmbeddedIterator.next();
Assert.assertEquals(cramRecordEmbedded, cramRecord);
}
Assert.assertEquals(count, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Also assert that both iterators are exhausted after the loop

final SAMRecord cramRecord = cramIterator.next();
final SAMRecord cramRecordEmbedded = cramEmbeddedIterator.next();
Assert.assertEquals(cramRecordEmbedded, cramRecord);
}
Assert.assertTrue( count >0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add text message for when assertion fails

@lbergelson lbergelson changed the title Fix an NPE in CRAMRecordReadFeatures.restoreReadBaases Fix an NPE in CRAMRecordReadFeatures.restoreReadBases Feb 21, 2023
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.

So I ran this in the debugger, and the reason the other embedded ref tests don't catch this issue is they have less interesting/varied read features than the other embedded ref tests (specifically this one has two read features with a gap between them). So I think this change is fine. We could use some better embedded ref tests.

@lbergelson lbergelson merged commit 8f8d567 into master Feb 21, 2023
@lbergelson lbergelson deleted the lb_fix_cram_restorebases_npe branch February 21, 2023 23:13
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.

3 participants