Re: [PATCH] Add tests for Bitmapset

On Sep 22 2025, at 3:57 am, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Fri, Sep 19, 2025 at 02:48:43PM -0400, Greg Burd wrote:
>> I think I've incorporated your feedback which did indeed make the patch
>> more readable/crisp and maintainable, thank you.
>>
>> coverage: HEAD v7 v8
>> lines......: 87.1 90.5 92.2

Michael,

Thank you for your time and help.

> bitmap_match() had some duplicated tests.
>
> bms_replace_members() was not covered, and a function was added to the
> module. I get up to 93.4% for the lines, once added.
>
> +PG_FUNCTION_INFO_V1(test_bms_from_array);
> +PG_FUNCTION_INFO_V1(test_bms_to_array);
> These two were still declared, are not required.
>
> There were a couple of functions that acted as dead code in the
> module, covered in the tree. As we are trying to cover the gap I
> think that we should just do the whole thing. Here are the functions:
> - is_empty
> - subset_compare
> - get_singleton_member
> - nonempty_difference
> - member_index
> - join. The test C function had a mistake, because either input may
> be modified or updated by bms_join(). The code was freeing the right
> input, leading to errors in the node output function when tested.

All great ideas, apologies for the oversights.

> Mixing the CTEs and the UNION ALL would have been nice if the queries
> in the UNIONs relied on more than one input value, but none of them
> did that, so I have removed this part and made the tests leaner. That
> feels easier for the eye.

Agreed, thank you.

> Testing on equality for the hash functions should be OK, so I have
> kept these, including the NULL/0 cases.

Sounds good to me.

> A few incorrect things related to the style, fixed on the way, like an
> incorrect copyright notice in meson.build, the top of test_bitmapset.c
> was largely incorrect. It seems like these were copy-pasted from
> elsewhere.

Yes, copied from the radixtree test module.

> Another thing was testing with -DREALLOCATE_BITMAPSETS, which I've
> done.

That seems like a good idea, I thought about doing that as well.

> A couple of code paths related to the test C functions don't
> test for NULL input, which is still OK as these don't impact the
> internal tests or the backend coverage, so I've left these out.

Fair.

> The result I had was good enough, so applied. The CI was OK, the
> buildfarm may have a different opinion.
> --
> Michael

Thanks for the help getting this done, I really appreciate it. So far
the buildfarm seems okay. :)

best.

-greg