On Sat, Oct 5, 2024 at 11:46 AM Claude Warren <cla...@xenei.com> wrote:
> Your Fixture solution is close to correct.
>
> @Override
> > public WrappedBloomFilter copy() {
> > return new Fixture(getWrapped());
> > }
>
>
> should return new Fixture(getWrapped().copy());
>
Thank you for your review Claude. These changes are now in git master.
Gary
>
> On Sat, Oct 5, 2024 at 4:42 PM Claude Warren <cla...@xenei.com> wrote:
>
> > Ignore my previous message. I don't know where my mind was.
> >
> > On Sat, Oct 5, 2024 at 4:26 PM Claude Warren <cla...@xenei.com> wrote:
> >
> >> WrappedBloomFilter is not a WrappedBloomFilter,
> >>
> >>
> >> This is true in most useful cases for the test `
> >> assertEquals(bf1.getClass(), copy.getClass());
> >>
> >> In a proposed KIP (Kafka Improvement Proposal) there is a Bloom filter
> >> that is decorated with additional data (The timestamp for when it was
> >> created). This class is called TimestampedBloomFilter and it extends
> >> WrappedBloomFilter.
> >>
> >> When TimestampedBloomFilter is passed to the WrappedBloomFilterTest
> >> classes and the "copy" method is called, the result is not a
> >> WrappedBloomFilter but a TimestampedBloomFilter.
> >>
> >> If the TimestampedBloomFilterTest extends the WrappedBloomFilterTest the
> >> test fails.
> >>
> >>
> >>
> >>
> >> On Sat, Oct 5, 2024 at 3:21 PM Gary Gregory <garydgreg...@gmail.com>
> >> wrote:
> >>
> >>> The WrappedBloomFilterTest currently says [0][1] that a copy of a
> >>> WrappedBloomFilter is not a WrappedBloomFilter, which does not make
> sense
> >>> to me as non-SME.
> >>>
> >>> Should the test be adjusted? It looks like the test is written in a
> >>> contrived way due to WrappedBloomFilter being abstract. I would make
> more
> >>> sense to me to have private static WrappedBloomFilter in the test and
> use
> >>> it, like this:
> >>>
> >>> private static class Fixture extends WrappedBloomFilter {
> >>>
> >>> public Fixture(BloomFilter wrapped) {
> >>> super(wrapped);
> >>> }
> >>>
> >>> @Override
> >>> public WrappedBloomFilter copy() {
> >>> return new Fixture(getWrapped());
> >>> }
> >>>
> >>> }
> >>>
> >>> @Override
> >>> protected WrappedBloomFilter createEmptyFilter(final Shape shape) {
> >>> return new Fixture(new
> >>> DefaultBloomFilterTest.SparseDefaultBloomFilter(shape));
> >>> }
> >>>
> >>> @ParameterizedTest
> >>> @ValueSource(ints = {0, 1, 34})
> >>> public void testCharacteristics(final int characteristics) {
> >>> final Shape shape = getTestShape();
> >>> final BloomFilter inner = new
> >>> DefaultBloomFilterTest.SparseDefaultBloomFilter(shape) {
> >>> @Override
> >>> public int characteristics() {
> >>> return characteristics;
> >>> }
> >>> };
> >>> final WrappedBloomFilter underTest = new Fixture(inner);
> >>> assertEquals(characteristics, underTest.characteristics());
> >>> }
> >>>
> >>> WDYT?
> >>>
> >>> Let's skip discussing the signature of the copy() method, I'll write a
> >>> separate email later about that.
> >>>
> >>> TY,
> >>> Gary
> >>> [0]
> >>>
> >>>
> https://github.com/apache/commons-collections/blob/730d972cdebb13dd3a896eb5b90ebc9e1f594d5b/src/test/java/org/apache/commons/collections4/bloomfilter/WrappedBloomFilterTest.java#L26-L56
> >>> [1] The above link in WrappedBloomFilterTest is:
> >>>
> >>> @Override
> >>> protected WrappedBloomFilter createEmptyFilter(final Shape shape) {
> >>> return new WrappedBloomFilter(new
> >>> DefaultBloomFilterTest.SparseDefaultBloomFilter(shape)) {
> >>> @Override
> >>> public BloomFilter copy() {
> >>> final BloomFilter result = new
> >>> DefaultBloomFilterTest.SparseDefaultBloomFilter(shape);
> >>> result.merge(getWrapped());
> >>> return result;
> >>> }
> >>> };
> >>> }
> >>>
> >>> @ParameterizedTest
> >>> @ValueSource(ints = {0, 1, 34})
> >>> public void testCharacteristics(final int characteristics) {
> >>> final Shape shape = getTestShape();
> >>> final BloomFilter inner = new
> >>> DefaultBloomFilterTest.SparseDefaultBloomFilter(shape) {
> >>> @Override
> >>> public int characteristics() {
> >>> return characteristics;
> >>> }
> >>> };
> >>> final WrappedBloomFilter underTest = new
> >>> WrappedBloomFilter(inner) {
> >>> @Override
> >>> public BloomFilter copy() {
> >>> final BloomFilter result = new
> >>> DefaultBloomFilterTest.SparseDefaultBloomFilter(shape);
> >>> result.merge(getWrapped());
> >>> return result;
> >>> }
> >>> };
> >>> assertEquals(characteristics, underTest.characteristics());
> >>> }
> >>>
> >>
> >>
> >> --
> >> LinkedIn: http://www.linkedin.com/in/claudewarren
> >>
> >
> >
> > --
> > LinkedIn: http://www.linkedin.com/in/claudewarren
> >
>
>
> --
> LinkedIn: http://www.linkedin.com/in/claudewarren
>
