aherbert commented on code in PR #402: URL: https://github.com/apache/commons-collections/pull/402#discussion_r1367387262
########## src/test/java/org/apache/commons/collections4/bloomfilter/CountingPredicateTest.java: ########## @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +public class CountingPredicateTest { + + @Test + public void testAryShort() { + CountingPredicate<Integer> cp = new CountingPredicate<>(new Integer[0], (x, y) -> x == null); + assertTrue(cp.test(Integer.valueOf(1))); + } + + @Test + public void testAryLong() { + Integer[] ary = { Integer.valueOf(1), Integer.valueOf(2) }; + CountingPredicate<Integer> cp = new CountingPredicate<>(ary, (x, y) -> y == null); + assertTrue(cp.forEachRemaining()); + + // test last item not checked + cp = new CountingPredicate<>(ary, (x, y) -> y == Integer.valueOf(2)); Review Comment: I think this is not testing what you want. You never call cp.test. So when you call cp.forEachRemaining your predicate will be called with (1, null) and then (2, null). You should check for this. But you are checking y == 2 which is not targeting any functionality of the CountingPredicate. A similar argument for the test below where you check y == 1. Since y will be null in forEachRemaining this is not testing a completion of the forEachRemaining loop. You should add a test case that when you call cp.test(Z) that the constructor BiPredicate receives (1, Z) then (2, Z). ########## src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterProducerTest.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertFalse; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.function.BiPredicate; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public abstract class AbstractBloomFilterProducerTest { + private Shape shape = Shape.fromKM(17, 72); Review Comment: This should be exposed for implementing classes. See AbstractBloomFilterTest.getTestShape. Here you have hard coded the same shape in implementing tests. ########## src/test/java/org/apache/commons/collections4/bloomfilter/DefaultBloomFilterTest.java: ########## @@ -234,9 +237,12 @@ public AbstractDefaultBloomFilter copy() { } } - static class NonSparseDefaultBloomFilter extends AbstractDefaultBloomFilter { + /** + * A default implementation of a non-sparse Bloom filter. + */ + public static class NonSparseDefaultBloomFilter extends AbstractDefaultBloomFilter { Review Comment: No need to be public ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java: ########## @@ -0,0 +1,321 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.LinkedList; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.apache.commons.collections4.bloomfilter.LayerManager.Cleanup; +import org.apache.commons.collections4.bloomfilter.LayerManager.ExtendCheck; +import org.junit.jupiter.api.Test; + +public class LayeredBloomFilterTest extends AbstractBloomFilterTest<LayeredBloomFilter> { + + @Override + protected LayeredBloomFilter createEmptyFilter(Shape shape) { + return LayeredBloomFilter.fixed(shape, 10); + } + + protected BloomFilter makeFilter(int... values) { + return makeFilter(IndexProducer.fromIndexArray(values)); + } + + protected BloomFilter makeFilter(IndexProducer p) { + BloomFilter bf = new SparseBloomFilter(getTestShape()); + bf.merge(p); + return bf; + } + + protected BloomFilter makeFilter(Hasher h) { + BloomFilter bf = new SparseBloomFilter(getTestShape()); + bf.merge(h); + return bf; + } + + @Test + public void testMultipleFilters() { + LayeredBloomFilter filter = LayeredBloomFilter.fixed(getTestShape(), 10); + filter.merge(TestingHashers.FROM1); + filter.merge(TestingHashers.FROM11); + assertEquals(2, filter.getDepth()); + assertTrue(filter.contains(makeFilter(TestingHashers.FROM1))); + assertTrue(filter.contains(makeFilter(TestingHashers.FROM11))); + BloomFilter t1 = makeFilter(6, 7, 17, 18, 19); + assertFalse(filter.contains(t1)); + assertFalse(filter.copy().contains(t1)); + assertTrue(filter.flatten().contains(t1)); + } + + private LayeredBloomFilter setupFindTest() { + LayeredBloomFilter filter = LayeredBloomFilter.fixed(getTestShape(), 10); + filter.merge(TestingHashers.FROM1); + filter.merge(TestingHashers.FROM11); + filter.merge(new IncrementingHasher(11, 2)); + filter.merge(TestingHashers.populateFromHashersFrom1AndFrom11(new SimpleBloomFilter(getTestShape()))); + return filter; + } + + @Test + public void testFindBloomFilter() { + LayeredBloomFilter filter = setupFindTest(); + int[] result = filter.find(TestingHashers.FROM1); + assertEquals(2, result.length); Review Comment: Should use assertArrayEquals to directly test the layers in which the hasher is found ########## src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java: ########## @@ -0,0 +1,42 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.Test; + +public class WrappedBloomFilterTest extends AbstractBloomFilterTest<WrappedBloomFilter> { + + @Override + protected WrappedBloomFilter createEmptyFilter(Shape shape) { + return new WrappedBloomFilter(new DefaultBloomFilterTest.SparseDefaultBloomFilter(shape)) { + }; + } + + @Test + public void testCharacteristics() { Review Comment: I do not know why but the characteristics method had no coverage when I ran tests locally. I changed this to: ```java @ParameterizedTest @ValueSource(ints = {0, 1, 34}) public void testCharacteristics(int characteristics) { Shape shape = getTestShape(); BloomFilter inner = new DefaultBloomFilterTest.SparseDefaultBloomFilter(shape) { @Override public int characteristics() { return characteristics; } }; WrappedBloomFilter underTest = new WrappedBloomFilter(inner) {}; assertEquals(characteristics, underTest.characteristics()); } ``` Now I see coverage. ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt); + LayerManager layerManager = testingBuilder().build(); + for (int i = 0; i < breakAt - 1; i++) { + assertFalse(underTest.test(layerManager), "at " + i); + layerManager.getTarget().merge(TestingHashers.FROM1); + } + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testAdvanceOnCount() { + testAdvanceOnCount(4); + testAdvanceOnCount(10); + testAdvanceOnCount(2); + testAdvanceOnCount(1); + 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(); + 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()); Review Comment: I was concerned by the use of random hasher. However with the shape as (17, 72) I observe only 3 or 4 hashers required to saturate the shape. Maybe add this as a note with the maxN value (2.9356) in case the shape ever changes in the future so it is obvious how to update the test. ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { Review Comment: This can be a ParameterizedTest with `@ValueSource(ints = {4, 10, 2, 1})`. You should test the IAE to ExtendCheck in a separate ParameterizedTest with not strictly positive counts. ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt); + LayerManager layerManager = testingBuilder().build(); + for (int i = 0; i < breakAt - 1; i++) { + assertFalse(underTest.test(layerManager), "at " + i); + layerManager.getTarget().merge(TestingHashers.FROM1); + } + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testAdvanceOnCount() { + testAdvanceOnCount(4); + testAdvanceOnCount(10); + testAdvanceOnCount(2); + testAdvanceOnCount(1); + 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()); Review Comment: This is the same as `testAdvanceOnSaturation`. I think it should be: ```java int maxN = shape.getNumberOfBits(); Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnSaturation(maxN); ``` ########## src/test/java/org/apache/commons/collections4/bloomfilter/DefaultBloomFilterTest.java: ########## @@ -215,9 +215,12 @@ public int cardinality() { } } - static class SparseDefaultBloomFilter extends AbstractDefaultBloomFilter { + /** + * A default implementation of a Sparse bloom filter. + */ + public static class SparseDefaultBloomFilter extends AbstractDefaultBloomFilter { - SparseDefaultBloomFilter(final Shape shape) { + public SparseDefaultBloomFilter(final Shape shape) { Review Comment: These do not need to be public since they are used only in the package. If I revert this change the test suite still functions. Are you using them externally by importing collections test-jar? ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt); + LayerManager layerManager = testingBuilder().build(); + for (int i = 0; i < breakAt - 1; i++) { + assertFalse(underTest.test(layerManager), "at " + i); + layerManager.getTarget().merge(TestingHashers.FROM1); + } + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testAdvanceOnCount() { + testAdvanceOnCount(4); + testAdvanceOnCount(10); + testAdvanceOnCount(2); + testAdvanceOnCount(1); + 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()); Review Comment: When I use maxN = number of bits I see the use of a random hasher requires 11-30 iterations (sample size = 20). So this is fine but a larger shape could be an issue to fill up. Could we replace this with a series of `IncrementingHasher` starting from 1 and increasing the initial value by (number of hash functions) each loop iteration? ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt); + LayerManager layerManager = testingBuilder().build(); + for (int i = 0; i < breakAt - 1; i++) { + assertFalse(underTest.test(layerManager), "at " + i); + layerManager.getTarget().merge(TestingHashers.FROM1); + } + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testAdvanceOnCount() { + testAdvanceOnCount(4); + testAdvanceOnCount(10); + testAdvanceOnCount(2); + testAdvanceOnCount(1); + 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() { Review Comment: This is exactly the same as `testAdvanceOnCalculatedFull`. I think this should be: ```java double maxN = shape.estimateMaxN(); Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnSaturation(maxN); ``` ########## src/test/java/org/apache/commons/collections4/bloomfilter/SimpleBloomFilterTest.java: ########## @@ -41,4 +41,16 @@ public void testMergeShortBitMapProducer() { assertTrue(filter.merge(producer)); assertEquals(1, filter.cardinality()); } + + @Test + public void testCardinalityAndIsEmpty() { Review Comment: This is just doing what the parent class does. The extra test should be in its own fixture, e.g. ```java @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()); } ``` ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java: ########## @@ -0,0 +1,321 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.LinkedList; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.apache.commons.collections4.bloomfilter.LayerManager.Cleanup; +import org.apache.commons.collections4.bloomfilter.LayerManager.ExtendCheck; +import org.junit.jupiter.api.Test; + +public class LayeredBloomFilterTest extends AbstractBloomFilterTest<LayeredBloomFilter> { + + @Override + protected LayeredBloomFilter createEmptyFilter(Shape shape) { + return LayeredBloomFilter.fixed(shape, 10); + } + + protected BloomFilter makeFilter(int... values) { + return makeFilter(IndexProducer.fromIndexArray(values)); + } + + protected BloomFilter makeFilter(IndexProducer p) { + BloomFilter bf = new SparseBloomFilter(getTestShape()); + bf.merge(p); + return bf; + } + + protected BloomFilter makeFilter(Hasher h) { + BloomFilter bf = new SparseBloomFilter(getTestShape()); + bf.merge(h); + return bf; + } + + @Test + public void testMultipleFilters() { + LayeredBloomFilter filter = LayeredBloomFilter.fixed(getTestShape(), 10); + filter.merge(TestingHashers.FROM1); + filter.merge(TestingHashers.FROM11); + assertEquals(2, filter.getDepth()); + assertTrue(filter.contains(makeFilter(TestingHashers.FROM1))); + assertTrue(filter.contains(makeFilter(TestingHashers.FROM11))); + BloomFilter t1 = makeFilter(6, 7, 17, 18, 19); + assertFalse(filter.contains(t1)); + assertFalse(filter.copy().contains(t1)); + assertTrue(filter.flatten().contains(t1)); + } + + private LayeredBloomFilter setupFindTest() { + LayeredBloomFilter filter = LayeredBloomFilter.fixed(getTestShape(), 10); + filter.merge(TestingHashers.FROM1); + filter.merge(TestingHashers.FROM11); + filter.merge(new IncrementingHasher(11, 2)); + filter.merge(TestingHashers.populateFromHashersFrom1AndFrom11(new SimpleBloomFilter(getTestShape()))); + return filter; + } + + @Test + public void testFindBloomFilter() { + LayeredBloomFilter filter = setupFindTest(); + int[] result = filter.find(TestingHashers.FROM1); + assertEquals(2, result.length); + assertEquals(0, result[0]); + assertEquals(3, result[1]); + result = filter.find(TestingHashers.FROM11); + assertEquals(2, result.length); + assertEquals(1, result[0]); + assertEquals(3, result[1]); + } + + @Test + public void testFindBitMapProducer() { + LayeredBloomFilter filter = setupFindTest(); + + IndexProducer idxProducer = TestingHashers.FROM1.indices(getTestShape()); + BitMapProducer producer = BitMapProducer.fromIndexProducer(idxProducer, getTestShape().getNumberOfBits()); + + int[] result = filter.find(producer); + assertEquals(2, result.length); + assertEquals(0, result[0]); + assertEquals(3, result[1]); + + 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]); + } + + @Test + public void testFindIndexProducer() { + IndexProducer producer = TestingHashers.FROM1.indices(getTestShape()); + LayeredBloomFilter filter = setupFindTest(); + + int[] result = filter.find(producer); + assertEquals(2, result.length); Review Comment: Use assertArrayEquals ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt); + LayerManager layerManager = testingBuilder().build(); + for (int i = 0; i < breakAt - 1; i++) { + assertFalse(underTest.test(layerManager), "at " + i); + layerManager.getTarget().merge(TestingHashers.FROM1); + } + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testAdvanceOnCount() { + testAdvanceOnCount(4); + testAdvanceOnCount(10); + testAdvanceOnCount(2); + testAdvanceOnCount(1); + 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(); + 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()); + } + assertTrue(underTest.test(layerManager)); + assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnSaturation(0)); + assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnSaturation(-1)); + } + + private void testOnMaxSize(int maxSize) { + Consumer<LinkedList<BloomFilter>> underTest = LayerManager.Cleanup.onMaxSize(maxSize); + LinkedList<BloomFilter> list = new LinkedList<>(); + for (int i = 0; i < maxSize; i++) { + assertEquals(i, list.size()); + list.add(new SimpleBloomFilter(shape)); + underTest.accept(list); + } + assertEquals(maxSize, list.size()); + + for (int i = 0; i < maxSize; i++) { + list.add(new SimpleBloomFilter(shape)); + underTest.accept(list); + assertEquals(maxSize, list.size()); + } + } + + @Test + public void testOnMaxSize() { + testOnMaxSize(5); + testOnMaxSize(100); + testOnMaxSize(2); + testOnMaxSize(1); + assertThrows(IllegalArgumentException.class, () -> LayerManager.Cleanup.onMaxSize(0)); + assertThrows(IllegalArgumentException.class, () -> LayerManager.Cleanup.onMaxSize(-1)); + } + + @Test + public void testNoCleanup() { + Consumer<LinkedList<BloomFilter>> underTest = LayerManager.Cleanup.noCleanup(); + LinkedList<BloomFilter> list = new LinkedList<>(); + for (int i = 0; i < 20; i++) { + assertEquals(i, list.size()); + list.add(new SimpleBloomFilter(shape)); + underTest.accept(list); + } + } + + @Test + public void testRemoveEmptyTarget() { + Consumer<LinkedList<BloomFilter>> underTest = LayerManager.Cleanup.removeEmptyTarget(); + LinkedList<BloomFilter> list = new LinkedList<>(); + + // removes an empty filter + BloomFilter bf = new SimpleBloomFilter(shape); + list.add(bf); + assertEquals(bf, list.get(0)); + underTest.accept(list); + assertTrue(list.isEmpty()); + + // does not remove a populated filter. + bf.merge(IndexProducer.fromIndexArray(1)); + list.add(bf); + assertEquals(bf, list.get(0)); + underTest.accept(list); + assertEquals(bf, list.get(0)); + + // does not remove an empty filter followed by a populated filter. + list.clear(); + list.add(new SimpleBloomFilter(shape)); + list.add(bf); + assertEquals(2, list.size()); + underTest.accept(list); + assertEquals(2, list.size()); + + // does not remove multiple empty filters at the end of the list, just the last + // one. + list.clear(); + list.add(bf); + list.add(new SimpleBloomFilter(shape)); + list.add(new SimpleBloomFilter(shape)); + assertEquals(3, list.size()); + underTest.accept(list); + assertEquals(2, list.size()); + assertEquals(bf, list.get(0)); + + } + + @Test + public void testCopy() { + LayerManager underTest = LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)).build(); + underTest.getTarget().merge(TestingHashers.randomHasher()); + underTest.next(); + underTest.getTarget().merge(TestingHashers.randomHasher()); + underTest.next(); + underTest.getTarget().merge(TestingHashers.randomHasher()); + assertEquals(3, underTest.getDepth()); + + LayerManager copy = underTest.copy(); + assertNotSame(underTest, copy); + // object equals not implemented + assertNotEquals(underTest, copy); + + assertEquals(underTest.getDepth(), copy.getDepth()); + assertTrue( + underTest.forEachBloomFilterPair(copy, (x, y) -> Arrays.equals(x.asBitMapArray(), y.asBitMapArray()))); + } + + @Test + public void testBuilder() { + LayerManager.Builder underTest = LayerManager.builder(); + NullPointerException npe = assertThrows(NullPointerException.class, () -> underTest.build()); + assertTrue(npe.getMessage().contains("Supplier must not be null")); + underTest.setSupplier(() -> null).setCleanup(null); + npe = assertThrows(NullPointerException.class, () -> underTest.build()); + assertTrue(npe.getMessage().contains("Cleanup must not be null")); + underTest.setCleanup(x -> { + }).setExtendCheck(null); + npe = assertThrows(NullPointerException.class, () -> underTest.build()); + assertTrue(npe.getMessage().contains("ExtendCheck must not be null")); + + npe = assertThrows(NullPointerException.class, () -> LayerManager.builder().setSupplier(() -> null).build()); + assertTrue(npe.getMessage().contains("filterSupplier returned null.")); + + } + + @Test + public void testClear() { + LayerManager underTest = LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)).build(); + underTest.getTarget().merge(TestingHashers.randomHasher()); + underTest.next(); + underTest.getTarget().merge(TestingHashers.randomHasher()); + underTest.next(); + underTest.getTarget().merge(TestingHashers.randomHasher()); + assertEquals(3, underTest.getDepth()); + underTest.clear(); + assertEquals(1, underTest.getDepth()); + assertEquals(0, underTest.getTarget().cardinality()); + } + + @Test + public void testNextAndGetDepth() { + LayerManager underTest = LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)).build(); + assertEquals(1, underTest.getDepth()); + underTest.getTarget().merge(TestingHashers.randomHasher()); + assertEquals(1, underTest.getDepth()); + underTest.next(); + assertEquals(2, underTest.getDepth()); + } + + @Test + public void testGet() { + LayerManager underTest = LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)).build(); Review Comment: The SimpleBloomFilter could be cached and then returned. You can then assert that the LayerManager either does or does not return a copy: ```java SimpleBloomFilter f = new SimpleBloomFilter(shape); LayerManager underTest = LayerManager.builder().setSupplier(() -> f).build() // ... assertSame(f, underTest.get(0)); ``` ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayeredBloomFilterTest.java: ########## @@ -0,0 +1,321 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.LinkedList; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.apache.commons.collections4.bloomfilter.LayerManager.Cleanup; +import org.apache.commons.collections4.bloomfilter.LayerManager.ExtendCheck; +import org.junit.jupiter.api.Test; + +public class LayeredBloomFilterTest extends AbstractBloomFilterTest<LayeredBloomFilter> { + + @Override + protected LayeredBloomFilter createEmptyFilter(Shape shape) { + return LayeredBloomFilter.fixed(shape, 10); + } + + protected BloomFilter makeFilter(int... values) { + return makeFilter(IndexProducer.fromIndexArray(values)); + } + + protected BloomFilter makeFilter(IndexProducer p) { + BloomFilter bf = new SparseBloomFilter(getTestShape()); + bf.merge(p); + return bf; + } + + protected BloomFilter makeFilter(Hasher h) { + BloomFilter bf = new SparseBloomFilter(getTestShape()); + bf.merge(h); + return bf; + } + + @Test + public void testMultipleFilters() { + LayeredBloomFilter filter = LayeredBloomFilter.fixed(getTestShape(), 10); + filter.merge(TestingHashers.FROM1); + filter.merge(TestingHashers.FROM11); + assertEquals(2, filter.getDepth()); + assertTrue(filter.contains(makeFilter(TestingHashers.FROM1))); + assertTrue(filter.contains(makeFilter(TestingHashers.FROM11))); + BloomFilter t1 = makeFilter(6, 7, 17, 18, 19); + assertFalse(filter.contains(t1)); + assertFalse(filter.copy().contains(t1)); + assertTrue(filter.flatten().contains(t1)); + } + + private LayeredBloomFilter setupFindTest() { + LayeredBloomFilter filter = LayeredBloomFilter.fixed(getTestShape(), 10); + filter.merge(TestingHashers.FROM1); + filter.merge(TestingHashers.FROM11); + filter.merge(new IncrementingHasher(11, 2)); + filter.merge(TestingHashers.populateFromHashersFrom1AndFrom11(new SimpleBloomFilter(getTestShape()))); + return filter; + } + + @Test + public void testFindBloomFilter() { + LayeredBloomFilter filter = setupFindTest(); + int[] result = filter.find(TestingHashers.FROM1); + assertEquals(2, result.length); + assertEquals(0, result[0]); + assertEquals(3, result[1]); + result = filter.find(TestingHashers.FROM11); + assertEquals(2, result.length); + assertEquals(1, result[0]); + assertEquals(3, result[1]); + } + + @Test + public void testFindBitMapProducer() { + LayeredBloomFilter filter = setupFindTest(); + + IndexProducer idxProducer = TestingHashers.FROM1.indices(getTestShape()); + BitMapProducer producer = BitMapProducer.fromIndexProducer(idxProducer, getTestShape().getNumberOfBits()); + + int[] result = filter.find(producer); + assertEquals(2, result.length); Review Comment: Use assertArrayEquals ########## src/test/java/org/apache/commons/collections4/bloomfilter/LayerManagerTest.java: ########## @@ -0,0 +1,305 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.collections4.bloomfilter; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +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.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedList; +import java.util.List; +import java.util.NoSuchElementException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +import org.junit.jupiter.api.Test; + +public class LayerManagerTest { + + private Shape shape = Shape.fromKM(17, 72); + + private LayerManager.Builder testingBuilder() { + return LayerManager.builder().setSupplier(() -> new SimpleBloomFilter(shape)); + } + + @Test + public void testAdvanceOnPopulated() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnPopulated(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + layerManager.getTarget().merge(TestingHashers.FROM1); + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testNeverAdvance() { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.neverAdvance(); + LayerManager layerManager = testingBuilder().build(); + assertFalse(underTest.test(layerManager)); + for (int i = 0; i < 10; i++) { + layerManager.getTarget().merge(TestingHashers.randomHasher()); + assertFalse(underTest.test(layerManager)); + } + } + + private void testAdvanceOnCount(int breakAt) { + Predicate<LayerManager> underTest = LayerManager.ExtendCheck.advanceOnCount(breakAt); + LayerManager layerManager = testingBuilder().build(); + for (int i = 0; i < breakAt - 1; i++) { + assertFalse(underTest.test(layerManager), "at " + i); + layerManager.getTarget().merge(TestingHashers.FROM1); + } + assertTrue(underTest.test(layerManager)); + } + + @Test + public void testAdvanceOnCount() { + testAdvanceOnCount(4); + testAdvanceOnCount(10); + testAdvanceOnCount(2); + testAdvanceOnCount(1); + 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(); + 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()); + } + assertTrue(underTest.test(layerManager)); + assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnSaturation(0)); + assertThrows(IllegalArgumentException.class, () -> LayerManager.ExtendCheck.advanceOnSaturation(-1)); + } + + private void testOnMaxSize(int maxSize) { Review Comment: Should be a ParameterizedTest. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
