Re: [PATCH] Add tests for Bitmapset

On Mon, Sep 15, 2025 at 03:56:30PM -0400, Greg Burd wrote:
> Reworked as indicated, thanks for pointing out the anti-pattern I'd
> missed.

Hmm. Should we try to get something closer in shape to what we do for
the SLRU tests, with one SQL function mapping to each C function we
are testing? This counts for bms_is_member(), bms_num_members(), add,
del, mul, hash and make_singleton.

+test_bms_del_member_negative(PG_FUNCTION_ARGS)
+{
+ /* This should throw an error for negative member */
+ bms_del_member(NULL, -20);
+ PG_RETURN_VOID();

For example, this case could use a Bitmapset and a number (or an array
of numbers for repeated operations), with NULL being one option for
the input Bitmapset. This makes the tests a bit more representative
of what they do at SQL level, hence there would be no need to
double-check a given line where a failure happens. The Bitmapset
could then be passed around as bytea blobs using \gset, for example.
This should not be a problem as long as they are palloc'd in the
TopMemoryContext. The SQL output for true/false/NULL/non-NULL
generated as output of the test could then be used instead of the
EXPECT_*() macros reported then in the elogs.

Perhaps my view of things is too cute and artistic, but when these
test suites grow over time and differ across branches, having to dig
into a specific line of a specific branch to get what a problem is
about is more error-prone. Particularly annoying when calling one
single function that does a lot of various actions, like the proposed
test_bitmapset().

Side note: `git diff --check` is reporting a bunch of indents with
spaces around the EXPECT macros.
--
Michael