aherbert commented on code in PR #331:
URL:
https://github.com/apache/commons-collections/pull/331#discussion_r958353209
##########
src/test/java/org/apache/commons/collections4/properties/EmptyPropertiesTest.java:
##########
@@ -31,16 +31,39 @@
import java.io.IOException;
import java.io.PrintStream;
import java.io.PrintWriter;
+import java.io.UnsupportedEncodingException;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Properties;
-
import org.apache.commons.io.input.NullReader;
import org.apache.commons.lang3.ArrayUtils;
import org.junit.jupiter.api.Test;
public class EmptyPropertiesTest {
+ /**
Review Comment:
You should not be including any changes to this package.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractBloomFilterTest.java:
##########
@@ -93,16 +94,30 @@ protected final Shape getTestShape() {
*
*/
@Test
- public void testConstructWithBadHasher() {
+ public void testMergeWithBadHasher() {
// value too large
+ final BloomFilter f = createEmptyFilter(getTestShape());
assertThrows(IllegalArgumentException.class,
- () -> createFilter(getTestShape(), new
BadHasher(getTestShape().getNumberOfBits())));
+ () -> f.merge(new
BadHasher(getTestShape().getNumberOfBits())));
// negative value
- assertThrows(IllegalArgumentException.class, () ->
createFilter(getTestShape(), new BadHasher(-1)));
+ BloomFilter f2 = createEmptyFilter(getTestShape());
+ assertThrows(IllegalArgumentException.class, () -> f2.merge(new
BadHasher(-1)));
}
@Test
- public void testConstructWitBitMapProducer() {
+ public void testMergeWithHasher() {
+ // value too large
+ final BloomFilter f = createEmptyFilter(getTestShape());
+ f.merge(from1);
+ int[] idx = f.asIndexArray();
+ assertEquals(getTestShape().getNumberOfHashFunctions(), idx.length);
+ for (int i=0; i<idx.length; i++) {
Review Comment:
Whitespace:
```Java
for (int i = 0; i < idx.length; i++) {
assertEquals(i + 1, idx[i]);
```
##########
src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilter.java:
##########
@@ -60,6 +60,11 @@ public interface BloomFilter extends IndexProducer,
BitMapProducer {
*/
Shape getShape();
+ /**
Review Comment:
Why is the clear method being added here? Your PR branch should be rebased
on the current master as this change should not be in this PR.
##########
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);
Review Comment:
Since these methods just do the same as `createEmptyFilter(Shape)` then
`merge(source)` I do not understand why they have to be overridden. The
implementation can be in the Abstract test:
```Java
protected BloomFilter createFilter(final Shape shape, final Hasher hasher) {
BloomFilter bf = createEmptyFilter(shape);
bf.merge(hasher);
return bf;
}
```
Is it because the return argument type is incorrect? Then this can be fixed
with:
```Java
protected SparseBloomFilter createFilter(final Shape shape, final Hasher
hasher) {
return (SparseBloomFilter) super.createFilter(shape, hasher);
}
```
Doing it this way ensures all filters are using the same create and merge
method to populate the new filter.
##########
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:
Why not use `createFilter` here. You have already reinstated the method. You
should roll back the changes throughout the test suite where you previously
dropped the method.
Your current diff seems to have too many changes. I think you should:
- branch from master
- copy all the modified classes in `main/java/...` over the existing classes
- modify only the 'createFilter' methods in the test classes and any other
location that previously used the removed constructors.
- set your upstream to this PR branch in your github and force push the
changes. That should clear all your current git history and just introduce the
desired changes.
##########
pom.xml:
##########
@@ -588,9 +588,9 @@
<commons.jira.pid>12310465</commons.jira.pid>
<!-- The RC version used in the staging repository URL. -->
<commons.rc.version>RC1</commons.rc.version>
- <checkstyle.version>3.1.2</checkstyle.version>
+ <checkstyle.version>3.2.0</checkstyle.version>
Review Comment:
The changes to pom.xml and changes.xml should not be here.
--
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]
