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.

Sign up for GitHub

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

Jump to bottom

Unit tests for Utils.concat() #7918

Merged

droazen merged 6 commits into master from oc_fixes7916_concatunittests

Jun 30, 2022

Contributor

orlicohen commented Jun 27, 2022

Fixes #7916. Data providers and unit tests for the various overloads of Utils.concat()

orlicohen added 2 commits

June 24, 2022 17:07


          unit tests for concat 2 methods (multiple byte arrays, 2 byte arrays)

4bd8ff3


          Added data providers/unit tests for 3 versions of concat()

9e779d6

orlicohen requested a review from droazen

June 27, 2022 16:41

orlicohen assigned droazen

orlicohen added 2 commits

June 27, 2022 13:39


          updated test case

5906c81


          added edge test cases

428a1a1

codecov bot commented Jun 28, 2022 •

edited

Loading

Codecov Report

Merging #7918 (7c5025c) into master (e81a361) will increase coverage by 0.101%.
The diff coverage is 84.615%.

@@               Coverage Diff               @@
##              master     #7918       +/-   ##
===============================================
+ Coverage     86.981%   87.082%   +0.101%     
+ Complexity     36962     36961        -1     
===============================================
  Files           2221      2215        -6     
  Lines         173894    173717      -177     
  Branches       18785     18778        -7     
===============================================
+ Hits          151254    151276       +22     
+ Misses         15998     15816      -182     
+ Partials        6642      6625       -17

Impacted Files	Coverage Δ
...broadinstitute/hellbender/utils/UtilsUnitTest.java	`93.151% <84.615%> (-0.458%)`	⬇️
...on/MultisampleMultidimensionalKernelSegmenter.java	`89.375% <0.000%> (-4.375%)`	⬇️
...lkers/vqsr/VariantRecalibratorIntegrationTest.java	`98.315% <0.000%> (-0.481%)`	⬇️
...s/copynumber/models/AlleleFractionLikelihoods.java	`100.000% <0.000%> (ø)`
...s/solver/SynchronizedUnivariateSolverUnitTest.java
...bender/utils/solver/RobustBrentSolverUnitTest.java
...ute/hellbender/utils/solver/RobustBrentSolver.java
...r/utils/solver/UnivariateSolverSpecifications.java
...der/utils/solver/SynchronizedUnivariateSolver.java
...r/utils/solver/UnivariateSolverJobDescription.java
... and 22 more

droazen suggested changes

View reviewed changes

Contributor

droazen left a comment

@orlicohen Looks good, just a few minor changes needed

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java

+                              new Object[]{ new byte[] {100, 100}, new byte[] {100, 100}, new byte[] { } },
+                              new Object[]{ ArrayUtils.EMPTY_BYTE_ARRAY, new byte[] { }, new byte[] { } },
+                              new Object[]{ new byte[] {100, 100, 100, 100}, new byte[] {100, 100}, new byte[] {100, 100} },
+                      };

Contributor

droazen Jun 28, 2022

Comments on the test cases for the varargs version of concat():

Calling concat() with 2 byte arrays actually calls the two-arg specialization of concat(), not the varargs version, so none of your test cases here should have 2 input arrays. Call concat() with 0, 1, or 3+ arrays to test the varargs version.
The values inside your arrays should not all be the same (100), otherwise you are not actually testing whether the method is preserving the original order of the values correctly during concatenation.

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java Outdated

+                              new Object[]{ new byte[] { }, new byte[] { }, ArrayUtils.EMPTY_BYTE_ARRAY },
+                              new Object[]{ new byte[] {100, 100}, new byte[] { }, new byte[] {100, 100} },
+                              new Object[]{ new byte[] { }, new byte[] {100, 100}, new byte[] {100, 100} },
+                              new Object[]{ new byte[] {100, 100}, new byte[] {100, 100}, new byte[] {100, 100, 100, 100} },

Contributor

droazen Jun 28, 2022

Comments on the test cases for the two-arg version of concat():

As above, the values inside your arrays should not all be the same (100), otherwise you are not actually testing whether the method is preserving the original order of the values correctly during concatenation.
Include at least one additional test case involving an array of length 1, as that is a common edge case for methods like this.

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java

+                                      new Integer[] {4, 5, 6} },
+                              new Object[]{ new Integer[] {1, 2, 3}, new Integer[] {4, 5, 6}, (IntFunction<Integer[]>)Integer[]::new,
+                                      new Integer[] {1, 2, 3, 4, 5, 6} },
+                      };

Contributor

droazen Jun 28, 2022

Comments on the test cases for the generic version of concat():

Include an additional test case with { 4, 5, 6 } and { 1, 2, 3 } passed in the reverse order, giving { 4, 5, 6, 1, 2, 3 }. This will demonstrate that the method is preserving input order correctly and not doing something silly like sorting the values during concatenation.

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java

+                  @Test(expectedExceptions = IllegalArgumentException.class, dataProvider = "concatAnyTypeWithNullArrayData")
+                  public void testConcatAnyTypeWithNullArray(Integer[] arr1, Integer[] arr2, IntFunction<Integer[]> constructor){
+                      Utils.concat(arr1, arr2, constructor);

Contributor

droazen Jun 28, 2022

Null tests look good 👍

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java Outdated

@@ @@ -28,7 +30,8 @@ @@
                * Testing framework for general purpose utilities class.
                *
                */
-              public final class UtilsUnitTest extends GATKBaseTest {
+              public final class
+              UtilsUnitTest extends GATKBaseTest {

Contributor

droazen Jun 28, 2022

Class name should be on the same line as the declaration

droazen assigned orlicohen and unassigned droazen


          responding to review comments

f7eb396

orlicohen requested a review from droazen

June 28, 2022 18:10

droazen suggested changes

View reviewed changes

Contributor

droazen left a comment

@orlicohen Two quick remaining comments, then this should be good to go

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java Outdated

+                              new Object[]{ new byte[] {100, 100}, new byte[] {100, 100} },
+                              new Object[]{ new byte[] {100, 100, 10, 10}, new byte[] {100, 100}, new byte[] { }, new byte[] {10,10} },
+                              new Object[]{ ArrayUtils.EMPTY_BYTE_ARRAY, new byte[] { }, new byte[] { }, new byte[] { } },
+                              new Object[]{ new byte[] {10, 10, 15, 15, 20, 20}, new byte[] {10, 10}, new byte[] {15, 15}, new byte[] {20, 20} },

Contributor

droazen Jun 29, 2022

This test case checks that the method is preserving the relative ordering of the separate byte arrays, but does not test whether the ordering within each array is also preserved. You can fix this by having the values within one of the arrays differ as well, and ideally not be sorted -- for example, changing the first array to { 12, 10 }

src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java Outdated

+                              new Object[]{ new byte[] {10, 10}, new byte[] { }, new byte[] {10, 10} },
+                              new Object[]{ new byte[] { }, new byte[] {10, 10}, new byte[] {10, 10} },
+                              new Object[]{ new byte[] {10}, new byte[] {15, 15}, new byte[] {10, 15, 15} },
+                              new Object[]{ new byte[] {10, 10}, new byte[] {15, 15}, new byte[] {10, 10, 15, 15} },

Contributor

droazen Jun 29, 2022

Same comment here: change the first array to something like { 12, 10 } to demonstrate that ordering within each input array is being preserved.


          updated unit tests

7c5025c

droazen approved these changes

View reviewed changes

droazen merged commit c40187a into master

droazen deleted the oc_fixes7916_concatunittests branch

June 30, 2022 18:40

orlicohen added a commit that referenced this pull request


          Add unit tests for Utils.concat() methods (#7918)

cecccd5

Added comprehensive unit tests for the various overloads of Utils.concat(), which were not previously covered by tests.

Resolves #7916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet