Re: [PATCH] Add tests for Bitmapset - Mailing list pgsql-hackers

On Thu, Sep 11, 2025, at 2:48 AM, Masahiko Sawada wrote:
> On Mon, Sep 8, 2025 at 11:21 AM Burd, Greg <greg@burd.me> wrote:
>>
>>
>> > On Sep 5, 2025, at 2:43 PM, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> >
>> > On Fri, Sep 05, 2025 at 10:48:21AM -0400, Burd, Greg wrote:
>> >> I looked at both radix tree and binary heap and how they use random sets when
>> >> testing.  Binary heap uses it to create different random sets of numbers to
>> >> use across multiple tests while radix tree has a single function that focuses
>> >> on randomized data.  I decided not to add randomization into the tests of
>> >> Bitmapset simply because I like avoiding non-deterministic behavior.  But in
>> >> tests I guess that can be helpful finding future unknown corner cases.  I'm
>> >> on the fence as to the value, your call.  :)
>> >
>> > I'm not too concerned about it.  We've lived without a dedicated test suite
>> > for Bitmapset for a very long time, so any amount of test coverage is an
>> > improvement.  Like you said, adding some randomization might be helpful for
>> > finding weird bugs we wouldn't have thought to test.  And, given the many,
>> > many machines that run the tests, IMHO it'd only help build even more
>> > confidence in the code.  If my suggestion inspires you to update the patch,
>> > great, but I'm fine with proceeding with what you already wrote, too.
>>
>> Nathan, thanks for considering the patch.  Honestly, I'm fine with it as is.
>> We can revisit later if needed.  This does what I'd intended, test and document
>> in code the API and implementation making future changes to that more
>> transparent.
>
> I appreciate your work on this. While I agree that adding more tests
> to  bitmapset.c is a good idea, I'm concerned about the minimal
> improvement in test coverage despite the addition of new test cases
> (only three lines of code are newly covered). Apart from adding some
> randomness to the tests we've discussed, given that we're implementing
> a dedicated test module for bitmapset.c, I would expect to see a more
> increase in test coverage.

Good point and thanks for taking the time to reply.  The only thing it identified
was a small issue fixed in b463288 so I'd not expect coverage to increase much.

I'll take a stab at adding some randomness to the tests and check the delta in
coverage.  Thanks for the nudge.  :)

>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com

Just for reference I started this not to increase coverage, which is a good
goal just not the one I had.  I was reviewing the API and considering some
changes based on other work I've done.  Now that I see how deeply baked in
this code is I think that's unlikely.  Maybe something else distinct for
bitmaps over 64-bit space at some point will be useful.  I wrote this code
just to capture the API in test form.

best,

-greg