Re: [PATCH] Add tests for Bitmapset

On Sat, Sep 27, 2025 at 10:03:14AM -0400, Greg Burd wrote:
>
> On Sep 24 2025, at 7:28 pm, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> > On Wed, Sep 24, 2025 at 07:39:59AM -0400, Greg Burd wrote:
> >> Thanks Michael, Tom, for the help getting this into shape and in the tree.
> >
> > By the way, Greg, do you think that we should aim for a state where we
> > are closer to completion?
>
> Hi Michael,
>
> We've come this far, why not go all the way? :)
>
> > We have the module up to the point where we
> > are in pretty good shape, with most things and the infrastructure done
> > but it can be improved a bit more, as well.
>
> Agreed.
>
> > Based on the information provided by the coverage report at
> > https://coverage.postgresql.org/src/backend/nodes/bitmapset.c.gcov.html,
> > we still have the following things:
> > - bms_equal for different word counts
> > - bms_union, bms_nonempty_difference, bms_is_subset and bms_intersect
> > with shorter word counts.
> > - bms_different with different word counts
> > - A couple more cases with bms_subset_compare
> > - bms_member_index and word counts
> > - bms_overlap_list with negative number in input list.
> > - bms_singleton_member ERROR with empty input.
> > - bms_get_singleton_member with NULL input
> > - bms_del_member with word counts
> > - bms_replace_members and repalloc case
> > - bms_add_range, bms_join and bms_del_members, more word count cases
> > - bms_prev_member and the prevbit business
>
> Thanks for the detailed analysis, I tried to address each of these in
> the patch.
>
> > There is not much we can do with the random function, still we could
> > do something about the NULL paths in the internal functions:
> > https://coverage.postgresql.org/src/test/modules/test_bitmapset/test_bitmapset.c.gcov.html
>
> Agreed.
>
> > The coverage of the latter matters less than the coverage of the
> > former, of course.
>
> Agreed.
> test_bitmapset.c
> lines......: 98.9%
> functions..: 100%
> branches...: 82.6%
>
> Here I removed a bit of code I thought was unnecessary and added a few
> test cases. The coverage is better overall, not much to say about it.
>
>
> bitmapset.c
> lines......: 99.8%
> functions..: 100%
> branches...: 89.4%

No need for three tests for the word count case in bms_del_member(),
only one is enough, but I've let them as they you have proposed, as
you wanted to check more positioning with the members. These are
cheap, that's fine.

Added a comment for bms_del_members() for the two cases that trigger
word count changes.

The test changed with test_bms_difference() is indeed long, but not
that bad with 140-ish characters, so left it as-is.

Good catch with bms_del_members() that was missing. There are so many
APIs that it's easy to miss one.

The changes in test_bms_get_singleton_member() don't seem adapted,
because we also want to test if the default value given by the
function caller is changed, bms_get_singleton_member() returning a
boolean status. The patch did not use arg1 at all. Also, if arg0 is
NULL, let's return the input value.

> A few branches were really tricky, my (least) favorite was the
> bms_subset_compare(). To get to the first BMS_DIFFERENT return you have
> to have a test case where the shorter bitmap is long enough to force two
> or more iterations of the loop and in the first iteration the first word
> in a needs to be a subset of b and then for the second word the opposite
> where the second word of b is a subset of a and yet there can be no
> overlapping bits in the two sets and the same number of words in both sets.
>
> test_bms_subset_compare('(b 1 64 65)', '(b 1 2 64)') -> BMS_DIFFERENT
>
> Technically the '(b 64 200)', '(b 1 201)' test case is also
> BMS_DIFFERENT and so redundant, but I was so happy to find the test case
> for this branch I kept them both.

I can see that you have expanded on these quite a bit, impressive.
One thing that was confusing is that some of the tests already
existed, so I've slightly reworded things to reduce the diffs. Per my
measurements, we are at more than 99% in the module and bitmapset.c
when only running make check in the module, which is nice
--
Michael