-
Notifications
You must be signed in to change notification settings - Fork 589
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
Added check for null sequence dictionaries #6147
Conversation
@@ -146,6 +146,8 @@ public static void validateDictionaries( final String name1, | |||
final SAMSequenceDictionary dict2, | |||
final boolean requireSuperset, | |||
final boolean checkContigOrdering ) { | |||
Utils.nonNull(dict1, "Something went wrong with sequence dictionary detection, check that "+name1+" has a valid sequence dictionary"); | |||
Utils.nonNull(dict2, "Something went wrong with sequence dictionary detection, check that "+name2+" has a valid sequence dictionary"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that fails without this patch, and passes with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@droazen added a test, back to you. |
@droazen can this be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment to address, then go ahead and merge once tests pass @jamesemery
@Test (expectedExceptions = java.lang.IllegalArgumentException.class) | ||
// test asserting that if the reference dictionary exists but is not valid we get a more helpful exception than a null pointer exception | ||
public void testBrokenReferenceDictionaryErrorMessage() throws IOException { | ||
createTempFile("reference", ".dict"); // file because we want an empty file that "exists" for the reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this unnecessary given your creation of emptyDict
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its marking it for extermination later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine, removed and will merge when tests pass
…at the reference diciotnary is called upon
7c0682e
to
1756f70
Compare
^ The failed build was evidently due to not having rebased the branch. |
As a compromise fix, I have added a check to the validation code that asserts the dictionaries actually exist to save ourselves the potential null-pointer exceptions. @droazen
Fixes #6142