Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 16 Sep 2025 15:04:03 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Greg Burd <greg@burd.me>
Cc: Robert Haas <robertmhaas@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Nathan Bossart <nathandbossart@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: [PATCH] Add tests for Bitmapset
Message-ID: <aMj90zO8B1EdF8CE@paquier.xyz>
References: <0FE0B26D-B94A-4BA7-BCA4-338C020609D9@greg.burd.me>
 <1CA731DE-B1BD-45C2-95B5-7E83FFE0E35C@greg.burd.me>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Nfg+wjLtD+3D/Zbj"
Content-Disposition: inline
In-Reply-To: <1CA731DE-B1BD-45C2-95B5-7E83FFE0E35C@greg.burd.me>
Archived-At: 
 <https://www.postgresql.org/message-id/aMj90zO8B1EdF8CE%40paquier.xyz>
Precedence: bulk


--Nfg+wjLtD+3D/Zbj
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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=20
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

--Nfg+wjLtD+3D/Zbj
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIyBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmjI/dMACgkQnvQgOdby
QH2SAQ/4ybTlc1jj+LTQVhqQ0NAG817O8C7IArqDxhRFiqXdNr+tnNJHsMYnLwdD
8NMEMbLjutYk6uT6q0Zv8w6TKgiKdBxjwsmqxyGxCtzJYf51HYlxXiupfJU2IUf2
HjK+H/ygVASWI5R5ew0dDlSMfep+x1nSLgn7jrwhxs6NoyLqQ7s8PkGXTmtJG9Dl
uQBDQM63WFGuAqMRZDTmcPGlar+f3ou8gsZKdwp2C2a8dAzcGoa1g7IEa5nTJQrU
OHq4IznfY72q1QS2uYlLaHUPSOyYlCY1gSZdw5Iojy/eR6s4PVm+BNfRTnkoyvXz
A7NTzofokeYFuJ5nAGpfQHYAeFu8oPdEjKpGj+jJDbj5mBueVK90rEnuys2yn91T
xsqTuTmpT3CXE3zRtvw+MimEe4kir00xjyaLCQQ6hT2FhDFHDjHHpzIIhDWF8iP9
7IQfoTpmvbabLhkK1xHjmaDjp4TfJ2OZl7QUJbDcmDQunwDKRFkZmJl5SdSVc/Rs
PLhjGWldzkhZDQwCf4UVsVVNTJVoIR6g+mkUKvPeAgMuMkAl2KoE2AThzbfJGbgS
lt0mfJE4ToxGtm3PAql8nqTiE6wni8yCq8l+dy+O5yDy3vbhbAzC9DNEC/rlUYyh
OiFnAYW0MdwCiXXmXtqxVcVMS0YE5skv2BHCnF7eN0Z9g3tV2A==
=fiBb
-----END PGP SIGNATURE-----

--Nfg+wjLtD+3D/Zbj--