Skip to content

Unit tests for Utils.concat() #7918

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

Merged
merged 6 commits into from
Jun 30, 2022
Merged

Unit tests for Utils.concat() #7918

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4bd8ff3
unit tests for concat 2 methods (multiple byte arrays, 2 byte arrays)
orlicohen Jun 24, 2022
9e779d6
Added data providers/unit tests for 3 versions of concat()
orlicohen Jun 27, 2022
5906c81
updated test case
orlicohen Jun 27, 2022
428a1a1
added edge test cases
orlicohen Jun 28, 2022
f7eb396
responding to review comments
orlicohen Jun 28, 2022
7c5025c
updated unit tests
orlicohen Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions src/test/java/org/broadinstitute/hellbender/utils/UtilsUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.common.primitives.Ints;
import htsjdk.samtools.util.Log.LogLevel;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang3.ArrayUtils;
import org.apache.logging.log4j.Level;
import org.broadinstitute.hellbender.GATKBaseTest;
import org.broadinstitute.hellbender.utils.io.IOUtils;
Expand All @@ -18,6 +19,7 @@
import java.io.File;
import java.nio.charset.StandardCharsets;
import java.util.*;
import java.util.function.IntFunction;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -978,4 +980,81 @@ public void testTestFilterCollectionByExpressions(Set<String> values, Collection
Set<String> actual = Utils.filterCollectionByExpressions(values, filters, exactMatch);
Assert.assertEquals(actual, expected);
}

@DataProvider(name = "concatMultipleByteArraysData")
public Object[][] concatMultipleByteArraysData() {
return new Object[][]{
// expected, arrays
new Object[]{ ArrayUtils.EMPTY_BYTE_ARRAY, new byte[] { } },
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} },
Copy link
Contributor

Choose a reason for hiding this comment

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

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 }

};
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

// expected listed first to accomodate varargs arrays having to be last parameter
@Test(dataProvider = "concatMultipleByteArraysData")
public void testConcatMultipleByteArrays(byte[] expected, byte[] ... arrays) {
Assert.assertEquals(Utils.concat(arrays), expected);
}

@Test
public void testConcatMultipleWithNoArrays(){
Assert.assertEquals(Utils.concat(), ArrayUtils.EMPTY_BYTE_ARRAY);
}

@DataProvider(name = "concatTwoByteArraysData")
public Object[][] concatTwoByteArraysData() {
return new Object[][]{
// arr1, arr2, expected
new Object[]{ new byte[] { }, new byte[] { }, ArrayUtils.EMPTY_BYTE_ARRAY },
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} },
Copy link
Contributor

Choose a reason for hiding this comment

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

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

};
}

@Test(dataProvider = "concatTwoByteArraysData")
public void testConcatTwoByteArrays(byte[] arr1, byte[] arr2, byte[] expected) {
Assert.assertEquals(Utils.concat(arr1, arr2), expected);
}

@DataProvider(name = "concatAnyTypeArraysData")
public Object[][] concatAnyTypeArraysData() {
return new Object[][]{
// arr1, arr2, constructor, expected
new Object[]{ new Integer[] { }, new Integer[] { }, (IntFunction<Integer[]>)Integer[]::new,
new Integer[] { } },
new Object[]{ new Integer[] {1, 2, 3}, new Integer[] { }, (IntFunction<Integer[]>)Integer[]::new,
new Integer[] {1, 2, 3} },
new Object[]{ new Integer[] { }, new Integer[] {4, 5, 6}, (IntFunction<Integer[]>)Integer[]::new,
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} },
new Object[]{ new Integer[] {4, 5, 6}, new Integer[] {1, 2, 3}, (IntFunction<Integer[]>)Integer[]::new,
new Integer[] {4, 5, 6, 1, 2, 3} },
};
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

@Test(dataProvider = "concatAnyTypeArraysData")
public void testConcatAnyTypeArrays(Integer[] arr1, Integer[] arr2, IntFunction<Integer[]> constructor, Integer[] expected) {
Assert.assertEquals(Utils.concat(arr1, arr2, constructor), expected);
}

@DataProvider(name = "concatAnyTypeWithNullArrayData")
public Object[][] concatAnyTypeWithNullArrayData(){
return new Object[][]{
new Object[]{null, new Integer[] {1, 2, 3}, (IntFunction<Integer[]>) Integer[]::new},
new Object[]{new Integer[] {1, 2, 3}, null, (IntFunction<Integer[]>) Integer[]::new},
new Object[]{null, null, (IntFunction<Integer[]>) Integer[]::new}
};
}

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

Choose a reason for hiding this comment

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

Null tests look good 👍

}

}