Em qua., 8 de out. de 2025 às 15:48, Greg Burd <[email protected]> escreveu:
> > On Oct 8 2025, at 8:10 am, Ranier Vilela <[email protected]> wrote: > > > Hi. > > > > > >> Em sex., 3 de out. de 2025 às 09:13, Greg Burd <[email protected]> escreveu: > >> > >>> > >>> On Oct 3 2025, at 4:25 am, Daniel Gustafsson <[email protected]> wrote: > >>> > >>>>> On 3 Oct 2025, at 01:36, David Rowley <[email protected]> wrote: > >>>>> > >>>>> On Fri, 3 Oct 2025 at 01:33, Daniel Gustafsson <[email protected]> > 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
