aherbert commented on code in PR #335:
URL:
https://github.com/apache/commons-collections/pull/335#discussion_r997963696
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractIndexProducerTest.java:
##########
@@ -134,7 +167,6 @@ public final void testConsistency() {
@Test
public final void testBehaviourAsIndexArray() {
int flags = getBehaviour();
- Assumptions.assumeTrue((flags & (AS_ARRAY_ORDERED |
AS_ARRAY_DISTINCT)) != 0);
Review Comment:
These Assumptions will skip the test if there is no known behaviour.
Otherwise the test asserts nothing so there is no point running it.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBitCountProducerTest.java:
##########
@@ -16,35 +16,29 @@
*/
package org.apache.commons.collections4.bloomfilter;
+import static org.junit.Assert.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
org.apache.commons.collections4.bloomfilter.BitCountProducer.BitCountConsumer;
import org.junit.jupiter.api.Test;
public abstract class AbstractBitCountProducerTest extends
AbstractIndexProducerTest {
Review Comment:
I am not seeing the version of AbstractBitCountProducerTest that I provided.
However you have noted in the comment that you 'moved and added code'. So I am
not sure what you updated. I think all you need to do is paste in the test
methods from the version I provided. In your current version the test asserts
that the order from getExpectedBitCount is matched, including any duplicate
indices. In my test class we separate the aggregation of the bit counts
(testForEachCount) and the test for uniqueness and ordering
(testBehaviourForEachCount) to respect the behaviour flag. There is also an
early exit test. This thus makes the BitCount producer test more closely match
the IndexProducer test.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractIndexProducerTest.java:
##########
@@ -19,13 +19,18 @@
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.BitSet;
+import java.util.List;
import java.util.function.IntPredicate;
import org.junit.jupiter.api.Assertions;
-import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Test;
+/**
+ * Test for IndexProducer.
+ *
Review Comment:
Remove extra blank line
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractIndexProducerTest.java:
##########
@@ -93,8 +98,36 @@ int[] toArray() {
*/
protected abstract int getBehaviour();
+ /**
+ * Creates an array of expected indices.
+ * @return an array of expected indices.
+ */
+ protected abstract int[] getExpectedIndices();
Review Comment:
Note that this method requires assumptions. The expected indices are
dependent on the producer returned from createProducer, which is ultimately
dependent on the shape of the Bloom filter.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractIndexProducerTest.java:
##########
@@ -93,8 +98,36 @@ int[] toArray() {
*/
protected abstract int getBehaviour();
+ /**
+ * Creates an array of expected indices.
+ * @return an array of expected indices.
+ */
+ protected abstract int[] getExpectedIndices();
+
+ @Test
+ public final void testAsIndexArrayValues() {
+ List<Integer> lst = new ArrayList<>();
Review Comment:
This test is asserting the distinctness of indices using lst.contains (which
is inefficient). If you only wish to test distinctness then use a BitSet not a
List<Integer>. Use of a list (over a set) would be to test the ordering.
Given we have a test for distinctness and ordering then I think this test
should be:
```Java
@Test
public final void testAsIndexArrayValues() {
BitSet bs = new BitSet();
Arrays.stream(createProducer().asIndexArray()).forEach(bs::set);
for (int i : getExpectedIndices()) {
assertTrue(bs.get(i), () -> "Missing " + i);
}
}
```
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractIndexProducerTest.java:
##########
@@ -43,7 +48,7 @@ public abstract class AbstractIndexProducerTest {
/**
* An expandable list of int values.
*/
- private static class IntList {
+ protected static class IntList {
Review Comment:
This can be private. It is not used in any other class.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBitCountProducerTest.java:
##########
@@ -16,35 +16,29 @@
*/
package org.apache.commons.collections4.bloomfilter;
+import static org.junit.Assert.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
org.apache.commons.collections4.bloomfilter.BitCountProducer.BitCountConsumer;
import org.junit.jupiter.api.Test;
public abstract class AbstractBitCountProducerTest extends
AbstractIndexProducerTest {
-
+ /**
+ * A testing BitCountConsumer that always returns true.
+ */
+ private static final BitCountConsumer TRUE_CONSUMER = (i, j) -> true;
/**
* A testing BitCountConsumer that always returns false.
*/
- public static BitCountConsumer FALSE_CONSUMER = new BitCountConsumer() {
-
- @Override
- public boolean test(int index, int count) {
- return false;
- }
- };
+ private static final BitCountConsumer FALSE_CONSUMER = (i, j) -> false;
/**
- * A testing BitCountConsumer that always returns true.
+ * Creates an array of integer pairs comprising the index and the expected
count for the index.
+ * @return an array of integer pairs comprising the index and the expected
count for the index.
*/
- public static BitCountConsumer TRUE_CONSUMER = new BitCountConsumer() {
-
- @Override
- public boolean test(int index, int count) {
- return true;
- }
- };
+ protected abstract int[][] getExpectedBitCount();
Review Comment:
This method may not need to be abstract. In most cases where ordering and
duplicates are not mandated then this implementation will be fine:
```Java
protected int[][] getExpectedBitCount() {
return Arrays.stream(getExpectedIndices()).mapToObj(x -> new int[]
{x, 1}).toArray(int[][]::new);
}
```
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractIndexProducerTest.java:
##########
@@ -149,7 +181,6 @@ public final void testBehaviourAsIndexArray() {
@Test
public final void testBehaviourForEach() {
int flags = getBehaviour();
- Assumptions.assumeTrue((flags & (FOR_EACH_ORDERED |
FOR_EACH_DISTINCT)) != 0);
Review Comment:
Reinstate the Assumptions
##########
src/test/java/org/apache/commons/collections4/bloomfilter/BitCountProducerFromIntArrayTest.java:
##########
@@ -0,0 +1,49 @@
+/*
+ * 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;
+
+public class BitCountProducerFromIntArrayTest extends
AbstractBitCountProducerTest {
+
+ int[] data = {6, 8, 1, 2, 4, 4, 5};
+
Review Comment:
Remove extra line
##########
src/test/java/org/apache/commons/collections4/bloomfilter/IndexProducerFromIntArrayTest.java:
##########
@@ -18,19 +18,32 @@
public class IndexProducerFromIntArrayTest extends AbstractIndexProducerTest {
+ int[] data = { 6, 8, 1, 2, 4, 4, 5 };
+ int[] expected = {1, 2, 4, 5, 6, 8};
+
@Override
protected IndexProducer createEmptyProducer() {
return IndexProducer.fromIndexArray(new int[0]);
}
@Override
protected IndexProducer createProducer() {
- return IndexProducer.fromIndexArray(new int[] { 6, 8, 1, 2, 4, 4, 5 });
+ return IndexProducer.fromIndexArray(data);
}
@Override
protected int getBehaviour() {
// Delegates to the default asIndexArray which is distinct and ordered
return AS_ARRAY_DISTINCT | AS_ARRAY_ORDERED;
}
+
+ @Override
+ protected int[] getExpectedIndices() {
+ return expected;
+ }
+
+// @Override
Review Comment:
Remove commented out code
--
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]
