From a71b55e83ca3738f3c60b16185a2d36d9a5b50b6 Mon Sep 17 00:00:00 2001 From: Claude Warren Date: Fri, 27 Oct 2023 17:40:13 +0200 Subject: [PATCH] Updated tests as per review --- .../AbstractBloomFilterProducerTest.java | 12 +++ .../bloomfilter/AbstractBloomFilterTest.java | 9 ++ .../bloomfilter/CountingPredicateTest.java | 94 ++++++++++++++++--- .../bloomfilter/LayerManagerTest.java | 45 ++++----- .../bloomfilter/LayeredBloomFilterTest.java | 30 +++--- .../bloomfilter/SimpleBloomFilterTest.java | 12 --- .../bloomfilter/WrappedBloomFilterTest.java | 23 +++-- 7 files changed, 146 insertions(+), 79 deletions(-) diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterProducerTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterProducerTest.java index b7ad6bca4c..3d445add96 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterProducerTest.java +++ b/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. + * + * @return the testing shape. + */ + protected Shape getTestShape() { + return shape; + } + @BeforeEach public void setup() { one.clear(); diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java index b325df5e1b..b9389af6dd 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java +++ b/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. */ diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java index 9b1a1e44d1..d92a34f448 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java +++ b/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> expected = new ArrayList<>(); + private List> result = new ArrayList<>(); + private Integer[] ary = {Integer.valueOf(1), Integer.valueOf(2)}; + + private BiPredicate makeFunc(BiPredicate 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 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 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 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 cp = new CountingPredicate<>(ary, (x, y) -> y == null); + public void testPredicateLonger() { + expected.clear(); + expected.add(Pair.of(1, 3)); + result.clear(); + CountingPredicate 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); } } diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java index 248964012c..c7023b1703 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java +++ b/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 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 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 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> underTest = LayerManager.Cleanup.onMaxSize(maxSize); LinkedList 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)); } diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java index 8041a30076..11164eca06 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java +++ b/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,17 +96,15 @@ 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 @@ -116,16 +112,14 @@ 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); } /** diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilterTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilterTest.java index 9fb8555e8a..f58828afcf 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilterTest.java +++ b/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()); - } - } diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java index b1b5980537..eca4a21a3b 100644 --- a/src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java +++ b/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 { @@ -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()); } }