aherbert commented on pull request #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1011293383
I've not had time for a complete review. It is in a long TODO list. Given that the code is meant to simplify things and there are no compatibility issues due to it being unreleased code then merging this should be fine. All the usual CI checks are green. However this project does not have code coverage listed in the CI checks. [Coveralls](https://coveralls.io/github/apache/commons-collections?branch=master) last received from the master branch in Nov 2021. This does not seem correct. So perhaps the change to github actions (no more travis) is no longer sending coveralls any report data. I've just done a quick local build and the bloom filters package has 94% instructions and 87% branched covered. This is above the 89% and 82% for the whole project. I would like to see some things tidied up. In my experience adding appropriate tests to get coverage is best done now when the developer of the code has the intended purpose fresh in mind. This can add checks to ensure the code paths are functioning as expected. Here are things not covered at all: - One SparseBloomFilter constructor - Shape.toString There is an exception case with no test coverage: - Shape.calculateNumberOfHashFunctions There are some paths in default methods of the BloomFilter interface not covered. This relates to sparse or not sparse filters. @Claudenw Can you look at the coverage and check these code paths. You can do this using: ``` mvn clean test -Dtest=%regex[.*bloomfilter.*] jacoco:report open target/site/jacoco/org.apache.commons.collections4.bloomfilter/index.html ``` -- 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]
