Re: [PATCH] Add tests for Bitmapset

On Sep 22 2025, at 3:57 am, Michael Paquier <michael@paquier.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