Updated tests as per review

src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterProducerTest.java

@@ -46,6 +46,18 @@ public abstract class AbstractBloomFilterProducerTest {
         return true;
     };
 
+    /**
+     * The shape of the Bloom filters for testing.
+     * <ul>
+     *  <li>Hash functions (k) = 17
+     *  <li>Number of bits (m) = 72
+     * </ul>
+     * @return the testing shape.
+     */
+    protected Shape getTestShape() {
+        return shape;
+    }
+
     @BeforeEach
     public void setup() {
         one.clear();

src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java

@@ -464,6 +464,15 @@ public void testCardinalityAndIsEmpty() {
         testCardinalityAndIsEmpty(createEmptyFilter(getTestShape()));
     }
 
+    @Test
+    public void testEmptyAfterMergeWithNothing() {
+        // test the case where is empty after merge
+        // in this case the internal cardinality == -1
+        BloomFilter bf = createEmptyFilter(getTestShape());
+        bf.merge(IndexProducer.fromIndexArray());
+        assertTrue(bf.isEmpty());
+    }
+
     /**
      * Testing class returns the value as the only value.
      */

src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java

@@ -16,31 +16,103 @@
  */
 package org.apache.commons.collections4.bloomfilter;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiPredicate;
+
+import org.apache.commons.lang3.tuple.Pair;
 import org.junit.jupiter.api.Test;
 
 public class CountingPredicateTest {
+    private List<Pair<Integer, Integer>> expected = new ArrayList<>();
+    private List<Pair<Integer, Integer>> result = new ArrayList<>();
+    private Integer[] ary = {Integer.valueOf(1), Integer.valueOf(2)};
+
+    private BiPredicate<Integer, Integer> makeFunc(BiPredicate<Integer, Integer> inner) {
+        return (x, y) -> {
+            if (inner.test(x, y)) {
+                result.add(Pair.of(x, y));
+                return true;
+            }
+            return false;
+        };
+    }
+
+    /**
+     * Test when the predicate array is shorter than other array as determined by the number
+     * of times cp.test() is called and all other values result in a true statement.
+     */
+    @Test
+    public void testPredicateShorter() {
+        Integer[] shortAry = {Integer.valueOf(3)};
+        expected.clear();
+        expected.add(Pair.of(3, 1));
+        expected.add(Pair.of(null, 2));
+        result.clear();
+        CountingPredicate<Integer> cp = new CountingPredicate<>(shortAry, makeFunc((x, y) -> true));
+        for (Integer i : ary) {
+            assertTrue(cp.test(i));
+        }
+        assertEquals(expected, result);
+        assertTrue(cp.forEachRemaining());
+        assertEquals(expected, result);
+    }
 
+    /**
+     * Test when the predicate array is shorter than other array as determined by the number
+     * of times cp.test() is called and all other values result in a true statement.
+     */
     @Test
-    public void testAryShort() {
-        CountingPredicate<Integer> cp = new CountingPredicate<>(new Integer[0], (x, y) -> x == null);
-        assertTrue(cp.test(Integer.valueOf(1)));
+    public void testPredicateSameLength() {
+        expected.clear();
+        expected.add( Pair.of(1, 3));
+        expected.add( Pair.of(2, 3));
+        result.clear();
+        CountingPredicate<Integer> cp = new CountingPredicate<>(ary, makeFunc((x, y) -> true));
+        assertTrue(cp.test(3));
+        assertTrue(cp.test(3));
+        assertEquals(expected, result);
+        assertTrue(cp.forEachRemaining());
+        assertEquals(expected, result);
     }
 
+    /**
+     * Test when the predicate array is longer than other array as determined by the number
+     * of times cp.test() is called and all other values result in a true statement.
+     */
     @Test
-    public void testAryLong() {
-        Integer[] ary = { Integer.valueOf(1), Integer.valueOf(2) };
-        CountingPredicate<Integer> cp = new CountingPredicate<>(ary, (x, y) -> y == null);
+    public void testPredicateLonger() {
+        expected.clear();
+        expected.add(Pair.of(1, 3));
+        result.clear();
+        CountingPredicate<Integer> cp = new CountingPredicate<>(ary, makeFunc((x, y) -> x!=null));
+        assertTrue(cp.test(Integer.valueOf(3)));
+        assertEquals(expected, result);
+        expected.add(Pair.of(2, null));
         assertTrue(cp.forEachRemaining());
+        assertEquals(expected, result);
 
-        // test last item not checked
-        cp = new CountingPredicate<>(ary, (x, y) -> y == Integer.valueOf(2));
-        assertFalse(cp.forEachRemaining());
+        // if the other array is zero length then cp.test() will not be called so
+        // we can just call cp.forEachRemaining() here.
+        expected.clear();
+        expected.add(Pair.of(1, null));
+        expected.add(Pair.of(2, null));
+        result.clear();
+        cp = new CountingPredicate<>(ary, makeFunc((x, y) -> x!=null));
+        assertTrue(cp.forEachRemaining());
+        assertEquals( expected, result);
 
-        // test last item fails
-        cp = new CountingPredicate<>(ary, (x, y) -> y == Integer.valueOf(1));
+        // If a test fails then the result should be false and the rest of the list should
+        // not be processed.
+        expected.clear();
+        expected.add(Pair.of(1, null));
+        result.clear();
+        cp = new CountingPredicate<>(ary,  makeFunc((x, y) -> x == Integer.valueOf(1)));
         assertFalse(cp.forEachRemaining());
+        assertEquals(expected, result);
     }
 }

src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java

@@ -21,7 +21,7 @@
 import static org.junit.Assert.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -34,6 +34,8 @@
 import java.util.function.Predicate;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 public class LayerManagerTest {
 
@@ -63,7 +65,9 @@ public void testNeverAdvance() {
         }
     }
 
-    private void testAdvanceOnCount(int breakAt) {
+    @ParameterizedTest
+    @ValueSource(ints = {4, 10, 2, 1})
+    public void testAdvanceOnCount(int breakAt) {
         Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt);
         LayerManager layerManager = testingBuilder().build();
         for (int i = 0; i < breakAt - 1; i++) {
@@ -74,42 +78,30 @@ private void testAdvanceOnCount(int breakAt) {
     }
 
     @Test
-    public void testAdvanceOnCount() {
-        testAdvanceOnCount(4);
-        testAdvanceOnCount(10);
-        testAdvanceOnCount(2);
-        testAdvanceOnCount(1);
+    public void testAdvanceOnCountInvalidArguments() {
         assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnCount(0));
         assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnCount(-1));
     }
 
-    @Test
-    public void testAdvanceOnCalculatedFull() {
-        Double maxN = shape.estimateMaxN();
-        Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnSaturation(shape.estimateMaxN());
-        LayerManager layerManager = testingBuilder().build();
-        while (layerManager.getTarget().getShape().estimateN(layerManager.getTarget().cardinality()) < maxN) {
-            assertFalse(underTest.test(layerManager));
-            layerManager.getTarget().merge(TestingHashers.randomHasher());
-        }
-        assertTrue(underTest.test(layerManager));
-    }
-
     @Test
     public void testAdvanceOnSaturation() {
         Double maxN = shape.estimateMaxN();
+        int hashStart = 0;
         Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnSaturation(maxN);
         LayerManager layerManager = testingBuilder().build();
         while (layerManager.getTarget().getShape().estimateN(layerManager.getTarget().cardinality()) < maxN) {
             assertFalse(underTest.test(layerManager));
-            layerManager.getTarget().merge(TestingHashers.randomHasher());
+            layerManager.getTarget().merge(new IncrementingHasher(hashStart, shape.getNumberOfHashFunctions()));
+            hashStart+=shape.getNumberOfHashFunctions();
         }
         assertTrue(underTest.test(layerManager));
         assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnSaturation(0));
         assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnSaturation(-1));
     }
 
-    private void testOnMaxSize(int maxSize) {
+    @ParameterizedTest
+    @ValueSource(ints = {5, 100, 2, 1})
+    public void testOnMaxSize(int maxSize) {
         Consumer<LinkedList<BloomFilter>> underTest = LayerManager.Cleanup.onMaxSize(maxSize);
         LinkedList<BloomFilter> list = new LinkedList<>();
         for (int i = 0; i < maxSize; i++) {
@@ -127,11 +119,7 @@ private void testOnMaxSize(int maxSize) {
     }
 
     @Test
-    public void testOnMaxSize() {
-        testOnMaxSize(5);
-        testOnMaxSize(100);
-        testOnMaxSize(2);
-        testOnMaxSize(1);
+    public void testOnMaxSizeIllegalValues() {
         assertThrows(IllegalArgumentException.class, () -> LayerManager.Cleanup.onMaxSize(0));
         assertThrows(IllegalArgumentException.class, () -> LayerManager.Cleanup.onMaxSize(-1));
     }
@@ -251,9 +239,10 @@ public void testNextAndGetDepth() {
 
     @Test
     public void testGet() {
-        LayerManager underTest = LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)).build();
+        SimpleBloomFilter f = new SimpleBloomFilter(shape);
+        LayerManager underTest = LayerManager.builder().setSupplier(() -> f).build();
         assertEquals(1, underTest.getDepth());
-        assertNotNull(underTest.get(0));
+        assertSame(f, underTest.get(0));
         assertThrows(NoSuchElementException.class, () -> underTest.get(-1));
         assertThrows(NoSuchElementException.class, () -> underTest.get(1));
     }

src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java

@@ -81,14 +81,12 @@ private LayeredBloomFilter setupFindTest() {
     @Test
     public void testFindBloomFilter() {
         LayeredBloomFilter filter = setupFindTest();
+        int[] expected = {0, 3};
         int[] result = filter.find(TestingHashers.FROM1);
-        assertEquals(2, result.length);
-        assertEquals(0, result[0]);
-        assertEquals(3, result[1]);
+        assertArrayEquals(expected, result);
+        expected = new int[] {1, 3};
         result = filter.find(TestingHashers.FROM11);
-        assertEquals(2, result.length);
-        assertEquals(1, result[0]);
-        assertEquals(3, result[1]);
+        assertArrayEquals(expected, result);
     }
 
     @Test
@@ -98,34 +96,30 @@ public void testFindBitMapProducer() {
         IndexProducer idxProducer = TestingHashers.FROM1.indices(getTestShape());
         BitMapProducer producer = BitMapProducer.fromIndexProducer(idxProducer, getTestShape().getNumberOfBits());
 
+        int[] expected = {0, 3};
         int[] result = filter.find(producer);
-        assertEquals(2, result.length);
-        assertEquals(0, result[0]);
-        assertEquals(3, result[1]);
+        assertArrayEquals(expected, result);
 
+        expected = new int[]{1, 3};
         idxProducer = TestingHashers.FROM11.indices(getTestShape());
         producer = BitMapProducer.fromIndexProducer(idxProducer, getTestShape().getNumberOfBits());
         result = filter.find(producer);
-        assertEquals(2, result.length);
-        assertEquals(1, result[0]);
-        assertEquals(3, result[1]);
+        assertArrayEquals(expected, result);
     }
 
     @Test
     public void testFindIndexProducer() {
         IndexProducer producer = TestingHashers.FROM1.indices(getTestShape());
         LayeredBloomFilter filter = setupFindTest();
 
+        int[] expected = {0, 3};
         int[] result = filter.find(producer);
-        assertEquals(2, result.length);
-        assertEquals(0, result[0]);
-        assertEquals(3, result[1]);
+        assertArrayEquals(expected, result);
 
+        expected = new int[] {1, 3};
         producer = TestingHashers.FROM11.indices(getTestShape());
         result = filter.find(producer);
-        assertEquals(2, result.length);
-        assertEquals(1, result[0]);
-        assertEquals(3, result[1]);
+        assertArrayEquals(expected, result);
     }
 
     /**

src/test/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilterTest.java

@@ -41,16 +41,4 @@ public void testMergeShortBitMapProducer() {
         assertTrue(filter.merge(producer));
         assertEquals(1, filter.cardinality());
     }
-
-    @Test
-    public void testCardinalityAndIsEmpty() {
-        testCardinalityAndIsEmpty(createEmptyFilter(getTestShape()));
-
-        // test the case where is empty after merge
-        // in this case the internal cardinality == -1
-        BloomFilter bf = createEmptyFilter(getTestShape());
-        bf.merge(IndexProducer.fromIndexArray());
-        assertTrue(bf.isEmpty());
-    }
-
 }

src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java

@@ -18,7 +18,8 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
-import org.junit.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 public class WrappedBloomFilterTest extends AbstractBloomFilterTest<WrappedBloomFilter> {
 
@@ -28,15 +29,17 @@ protected WrappedBloomFilter createEmptyFilter(Shape shape) {
         };
     }
 
-    @Test
-    public void testCharacteristics() {
+    @ParameterizedTest
+    @ValueSource(ints = {0, 1, 34})
+    public void testCharacteristics(int characteristics) {
         Shape shape = getTestShape();
-        BloomFilter inner = new DefaultBloomFilterTest.SparseDefaultBloomFilter(shape);
-        WrappedBloomFilter underTest = createEmptyFilter(getTestShape());
-        assertEquals(inner.characteristics(), underTest.characteristics());
-
-        inner = new DefaultBloomFilterTest.NonSparseDefaultBloomFilter(shape);
-        underTest = createEmptyFilter(getTestShape());
-        assertEquals(inner.characteristics(), underTest.characteristics());
+        BloomFilter inner = new DefaultBloomFilterTest.SparseDefaultBloomFilter(shape) {
+            @Override
+            public int characteristics() {
+                return characteristics;
+            }
+        };
+        WrappedBloomFilter underTest = new WrappedBloomFilter(inner) {};
+        assertEquals(characteristics, underTest.characteristics());
     }
 }