public inbox for [email protected]  

help / color / mirror / Atom feed

From: Greg Burd <[email protected]>
To: Masahiko Sawada <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: [PATCH] Add tests for Bitmapset
Date: Thu, 11 Sep 2025 06:56:07 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAD21AoDijQoXik+U8hf0QptPvNcpjCjfzO_Rp=eCLYSL1D6OOQ@mail.gmail.com>
References: <[email protected]>
	<aLpENihhKL4vOxeX@nathan>
	<[email protected]>
	<aLsvPdSHJdRCEk7v@nathan>
	<[email protected]>
	<CAD21AoDijQoXik+U8hf0QptPvNcpjCjfzO_Rp=eCLYSL1D6OOQ@mail.gmail.com>


On Thu, Sep 11, 2025, at 2:48 AM, Masahiko Sawada wrote:
> On Mon, Sep 8, 2025 at 11:21 AM Burd, Greg <[email protected]> wrote:
>>
>>
>> > On Sep 5, 2025, at 2:43 PM, Nathan Bossart <[email protected]> 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

view thread (81+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Add tests for Bitmapset
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox