aherbert commented on code in PR #331:
URL:
https://github.com/apache/commons-collections/pull/331#discussion_r958540943
##########
src/test/java/org/apache/commons/collections4/bloomfilter/DefaultBloomFilterTest.java:
##########
@@ -163,28 +123,36 @@ public boolean merge(BloomFilter other) {
String.format("Value in list %s is less than 0",
indices.first()));
}
}
+ }
+ @Override
Review Comment:
Requires an extra blank line
##########
src/test/java/org/apache/commons/collections4/bloomfilter/SparseBloomFilterTest.java:
##########
@@ -33,66 +33,31 @@ protected SparseBloomFilter createEmptyFilter(final Shape
shape) {
@Override
protected SparseBloomFilter createFilter(final Shape shape, final Hasher
hasher) {
- return new SparseBloomFilter(shape, hasher);
+ SparseBloomFilter bf = new SparseBloomFilter(shape);
+ bf.merge(hasher);
+ return bf;
}
@Override
protected SparseBloomFilter createFilter(final Shape shape, final
BitMapProducer producer) {
- return new SparseBloomFilter(shape, producer);
+ SparseBloomFilter bf = new SparseBloomFilter(shape);
+ bf.merge(producer);
+ return bf;
}
@Override
protected SparseBloomFilter createFilter(final Shape shape, final
IndexProducer producer) {
- return new SparseBloomFilter(shape, producer);
+ SparseBloomFilter bf = new SparseBloomFilter(shape);
+ bf.merge(producer);
+ return bf;
}
- private void executeNestedTest(SparseBloomFilterTest nestedTest) {
- nestedTest.testContains();
- nestedTest.testEstimateIntersection();
- nestedTest.testEstimateN();
- nestedTest.testEstimateUnion();
- nestedTest.testIsFull();
- nestedTest.testMerge();
- }
-
- @Test
- public void testConstructors() {
-
- // copy of Sparse
- SparseBloomFilterTest nestedTest = new SparseBloomFilterTest() {
-
- @Override
- protected SparseBloomFilter createEmptyFilter(Shape shape) {
- return new SparseBloomFilter(new SparseBloomFilter(shape));
- }
-
- @Override
- protected SparseBloomFilter createFilter(Shape shape, Hasher
hasher) {
- return new SparseBloomFilter(new SparseBloomFilter(shape,
hasher));
- }
- };
- executeNestedTest(nestedTest);
-
- // copy of Simple
- nestedTest = new SparseBloomFilterTest() {
-
- @Override
- protected SparseBloomFilter createEmptyFilter(Shape shape) {
- return new SparseBloomFilter(new SimpleBloomFilter(shape));
- }
-
- @Override
- protected SparseBloomFilter createFilter(Shape shape, Hasher
hasher) {
- return new SparseBloomFilter(new SimpleBloomFilter(shape,
hasher));
- }
- };
- executeNestedTest(nestedTest);
- }
@Test
public void testBitMapProducerEdgeCases() {
int[] values = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 65, 66, 67, 68, 69, 70, 71
};
- BloomFilter bf = createFilter(getTestShape(),
IndexProducer.fromIndexArray(values));
Review Comment:
This comment has not been addressed as all the `createEmptyFilter` followed
by `merge` does not seem required.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/SetOperationsTest.java:
##########
@@ -49,52 +49,63 @@ private static void assertSymmetricOperation(double
expected, ToDoubleBiFunction
assertEquals(expected, operation.applyAsDouble(filter2, filter1),
"op(filter2, filter1)");
}
+ private BloomFilter createTest(Shape shape, Hasher hasher) {
Review Comment:
These should be more appropriately named, e.g. `createFilter`
##########
src/test/java/org/apache/commons/collections4/bloomfilter/EnhancedDoubleHasherTest.java:
##########
@@ -43,39 +43,39 @@ protected int getHasherSize(Hasher hasher) {
@Test
public void testByteConstructor() {
// single value become increment.
- EnhancedDoubleHasher hasher = new EnhancedDoubleHasher( new byte[] { 1
} );
- assertEquals( 0, hasher.getInitial() );
- assertEquals( 0x01_00_00_00_00_00_00_00L, hasher.getIncrement() );
+ EnhancedDoubleHasher hasher = new EnhancedDoubleHasher(new byte[] { 1
});
Review Comment:
These formatting changes should not be part of this PR. I have applied
formatting to master so you can rebase and should pick up the changes.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java:
##########
@@ -316,20 +345,20 @@ public final void testMerge() {
assertThrows(IllegalArgumentException.class, () -> bf1.merge(new
BadHasher(-1)));
// test error when bloom filter returns values out of range
- final BloomFilter bf5 = new SimpleBloomFilter(
- Shape.fromKM(getTestShape().getNumberOfHashFunctions(), 3 *
Long.SIZE),
- new IncrementingHasher(Long.SIZE * 2, 1));
+ BloomFilter bf5 = new SimpleBloomFilter(
+ Shape.fromKM(getTestShape().getNumberOfHashFunctions(), 3 *
Long.SIZE));
+ bf5.merge(new IncrementingHasher(Long.SIZE * 2, 1));
assertThrows(IllegalArgumentException.class, () -> bf1.merge(bf5));
- final BloomFilter bf6 = new SparseBloomFilter(
- Shape.fromKM(getTestShape().getNumberOfHashFunctions(), 3 *
Long.SIZE),
- new IncrementingHasher(Long.SIZE * 2, 1));
+ BloomFilter bf6 = new SparseBloomFilter(
+ Shape.fromKM(getTestShape().getNumberOfHashFunctions(), 3 *
Long.SIZE));
+ bf6.merge(new IncrementingHasher(Long.SIZE * 2, 1));
assertThrows(IllegalArgumentException.class, () -> bf1.merge(bf6));
}
- private static void assertIndexProducerConstructor(Shape shape, int[]
values, int[] expected) {
+ private void assertIndexProducerConstructor(Shape shape, int[] values,
int[] expected) {
Review Comment:
This method name is now incorrect. Perhaps `assertIndexProducerMerge`
--
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]
