Re: [PATCH] Add tests for Bitmapset

Em qua., 8 de out. de 2025 às 15:48, Greg Burd <greg(at)burd(dot)me> escreveu:

>
> On Oct 8 2025, at 8:10 am, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> > Hi.
> >
> >
> >> Em sex., 3 de out. de 2025 às 09:13, Greg Burd <greg(at)burd(dot)me> escreveu:
> >>
> >>>
> >>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >>>
> >>>>> On 3 Oct 2025, at 01:36, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> >>>>>
> >>>>> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson <daniel(at)yesql(dot)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.
>
Great. Thank you for their efforts.

Tests pass here too (Windows).

LGTM.

best regards,
Ranier Vilela