Thread

Re: [PATCH] Add tests for Bitmapset

Greg Burd <greg@burd.me> — 2025-10-08T18:48:03Z

On Oct 8 2025, at 8:10 am, Ranier Vilela <ranier.vf@gmail.com> wrote:

> Hi.
>  
>  
>> Em sex., 3 de out. de 2025 às 09:13, Greg Burd <greg@burd.me> escreveu:
>>  
>>>  
>>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>  
>>>>> On 3 Oct 2025, at 01:36, David Rowley <dgrowleyml@gmail.com> wrote:
>>>>>  
>>>>> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson <daniel@yesql.se> wrote:
>>>>>> Another nitpick would be to remove the test for NULL in test_bms_make_singleton
>>>>>> since that is a STRICT function, making the test for NULL
>>>>>> superfluous code:
>>>>>  
>>>>> I see test_random_operations() is also strict. Is it worth getting rid
>>>>> of the SQL NULL checks on the inputs there too? Aka, the attached.
>>>>  
>>>> Indeed, but reading the code I wonder if STRICT was a mistake and
>>>> the intention
>>>> was to allow NULL input?
>>>  
>>> Yes, it was an oversight after I re-worked the random function.
>>>  
>>>> That being said, the function is never called with
>>>> NULL so that's mostly academic thinking.  +1 for removing the NULL
>>>> checks and simplifying the code.
>>>  
>>> I agree, and thank you both for the attention to detail and interest in
>>> this test suite.
>> With the patch attached, there are regression.
>> Is it intentional not to check the return of the function bms_is_member?
>>  
>> diff --strip-trailing-cr -U3
>> C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expected/test_bitmapset.out C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/results/test_bitmapset.out
>> ---
>> C:/dll/postgres_dev/postgres_commit/src/test/modules/test_bitmapset/expected/test_bitmapset.out
>> 2025-10-02 21:17:53.169515700 -0300
>> +++
>> C:/dll/postgres_dev/postgres_commit/build/testrun/test_bitmapset/regress/results/test_bitmapset.out
>> 2025-10-07 21:24:00.534142300 -0300
>> @@ -1570,9 +1570,5 @@
>>  
>>  -- random operations
>>  SELECT test_random_operations(-1, 10000, 81920, 0) > 0 AS result;
>> - result
>> ---------
>> - t
>> -(1 row)
>> -
>> +ERROR:  union missing member 63904
>>  DROP EXTENSION test_bitmapset;
>>  
>> best regards,
>> Ranier Vilela

Thanks Ranier for the report.

You're correct, the tests I wrote overlooked testing the return value
and so were not accomplishing the goal of testing at all.

Adding your small suggested patched turned over another flaw.  The error
message you were seeing was from later in that same function.

> +ERROR:  union missing member 63904

This was emitted by the code in the second portion of the randomized
testing where there is a loop doing add/del/test operations.  That error
message was misleading as it matched the earlier error message near the
change you made.

But, thankfully that pointed to a second (face palm) flaw in that
function (again, I take full credit/blame).  The add/del/test loop
wasn't accumulating the anticipated set of members and so was testing if
the latest random int generated was in the set or not, which makes no
sense.  I've updated that loop to retain the list of members and the
test to check against the retained list of members.  I've updated the
log message to be different from those above to avoid similar confusion
in the future.

This seems to be passing now, apologies for not paying closer attention
to this code earlier on.

best.

-greg