Re: [PATCH] Add tests for Bitmapset

On Thu, Sep 11, 2025, at 2:48 AM, Masahiko Sawada wrote:
> On Mon, Sep 8, 2025 at 11:21 AM Burd, Greg <greg(at)burd(dot)me> wrote:
>>
>>
>> > On Sep 5, 2025, at 2:43 PM, Nathan Bossart <nathandbossart(at)gmail(dot)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