ah-6766-NPE #6847
ah-6766-NPE #6847
Conversation
…tests to guard against this.
| .collect(Collectors.toList()); | ||
| return String.join(AnnotationUtils.ALLELE_SPECIFIC_RAW_DELIM, alleleStrings); | ||
|
|
||
| public static String makeRawAnnotationString(final List<Allele> vcAlleles, final Map<Allele, List<Integer>> perAlleleValues) { |
There was a problem hiding this comment.
Since you are reverting this method back to a version that handles nulls, I think you should remove the perAlleleValues.values().removeIf(Objects::isNull); line you added to computeSBAnnotation() above as well, for consistency with the pre-bug behavior.
There was a problem hiding this comment.
reverted to previous code
| * Test for issue #6766 | ||
| */ | ||
| @Test | ||
| public void testCombineRawData() { |
There was a problem hiding this comment.
Give the test a more specific name: testCombineRawDataNullPointerExceptionIssue6766()
| // These are both 0 because we did not set any SB data for either allele | ||
| Assert.assertEquals(combinedListString, "0,0|0,0"); | ||
| } catch (NullPointerException npe) { | ||
| Assert.fail("This code should not be throwing a NullPointerException"); |
There was a problem hiding this comment.
Can you confirm that with the previous version of the StrandBiasUtils methods we do get a NullPointerException here?
| * Test for issue #6766 | ||
| */ | ||
| @Test | ||
| public void testMakeRawAnnotationStrings() { |
There was a problem hiding this comment.
Give this one a more specific name too: testMakeRawAnnotationStringsNullPointerExceptionIssue6766()
| try { | ||
| StrandBiasUtils.makeRawAnnotationString(vc.getAlleles(), perAlleleValues); | ||
| } catch (NullPointerException npe) { | ||
| Assert.fail("This code should not be throwing a NullPointerException"); |
There was a problem hiding this comment.
Same question here: did the NullPointerException get triggered with the previous version of StrandBiasUtils?
|
yes, both tests did trigger a null pointer exception with the previous code. (there are comments in one test because it had to do something a little weird to actually trigger the NPE. |
Reverted code back to previous version that did not throw NPE. Add 2 unit tests to guard against this. Resolves #6766
revert code back to previous code that did not throw NPE. Add 2 unit tests to guard against this.