Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oyFhM-0000xc-IA for pgsql-hackers@arkaria.postgresql.org; Thu, 24 Nov 2022 17:07:40 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oyFhL-0006C6-7q for pgsql-hackers@arkaria.postgresql.org; Thu, 24 Nov 2022 17:07:39 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oyFhK-0006Aq-Uj for pgsql-hackers@lists.postgresql.org; Thu, 24 Nov 2022 17:07:38 +0000 Received: from mail.nataraj.su ([88.198.198.57]) by magus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oyFhE-0005DM-P6 for pgsql-hackers@lists.postgresql.org; Thu, 24 Nov 2022 17:07:38 +0000 Received: from thinkpad-pgpro.localnet (unknown [91.203.188.92]) by mail.nataraj.su (Postfix) with ESMTPSA id 8E9FC9E22D; Thu, 24 Nov 2022 17:07:30 +0000 (UTC) From: Nikolay Shaplov To: Andres Freund , Daniel Gustafsson Cc: Dagfinn Ilmari =?ISO-8859-1?Q?Manns=E5ker?= , PostgreSQL Developers , Andrew Dunstan , Tom Lane , Peter Eisentraut Subject: Re: TAP output format in pg_regress Date: Thu, 24 Nov 2022 20:07:29 +0300 Message-ID: <1787166.pFkNfHHBAF@thinkpad-pgpro> Organization: Postgres Professional In-Reply-To: References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com> <20221122235636.4frx7hjterq6bmls@awork3.anarazel.de> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2874975.GvFihrIVtr"; micalg="pgp-sha512"; protocol="application/pgp-signature" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --nextPart2874975.GvFihrIVtr Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii"; protected-headers="v1" From: Nikolay Shaplov To: Andres Freund , Daniel Gustafsson Cc: Dagfinn Ilmari =?ISO-8859-1?Q?Manns=E5ker?= , PostgreSQL Developers , Andrew Dunstan , Tom Lane , Peter Eisentraut Subject: Re: TAP output format in pg_regress Date: Thu, 24 Nov 2022 20:07:29 +0300 Message-ID: <1787166.pFkNfHHBAF@thinkpad-pgpro> Organization: Postgres Professional In-Reply-To: References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com> <20221122235636.4frx7hjterq6bmls@awork3.anarazel.de> You guys are really fast... I only think about problem, it is already mentioned here... Most issues I've noticed are already fixed before I was able to say something. Nevertheless... /* * Bailing out is for unrecoverable errors which prevents further testing to * occur and after which the test run should be aborted. By passing non_rec * as true the process will exit with _exit(2) and skipping registered exit * handlers. */ static void bail_out(bool non_rec, const char *fmt,...) { In original code, where _exit were called, there were mention about recursion (whatever it is) as a reason for using _exit() instead of exit(). Now this mention is gone: - _exit(2); /* not exit(), that could be recursive */ + /* Not using the normal bail() as we want _exit */ + _bail(_("could not stop postmaster: exit code was %d\n"), r); The only remaining part of recursion is _rec suffix. I guess we should keep mention of recursion in comments, and for me _rec stands for "record", not "recursion", I will not guess original meaning by _rec suffix. I would suggest to change it to _recurs or _recursive to keep things clear ----- +#define _bail(...) bail_out(true, __VA_ARGS__) +#define bail(...) bail_out(false, __VA_ARGS__) I will never guess what the difference between bail, _bail and bail_out. We need really good explanation here, or better to use self-explanatory names here... I would start bail_out with _ as it is "internal", not used in the code. And for _bail I would try to find some name that shows it's nature. Like bail_in_recursion or something. ----- + if (ok || ignore) { - va_start(ap, fmt); - vfprintf(logfile, fmt, ap); - va_end(ap); + emit_tap_output(TEST_STATUS, "ok %-5i %s%-*s %8.0f ms%s\n", + testnumber, + (parallel ? PARALLEL_INDENT : ""), + padding, testname, + runtime, + (ignore ? " # SKIP (ignored)" : "")); + } + else + { + emit_tap_output(TEST_STATUS, "not ok %-5i %s%-*s %8.0f ms\n", + testnumber, + (parallel ? PARALLEL_INDENT : ""), + padding, testname, + runtime); } This magic spell "...%-5i %s%-*s %8.0f ms\n" is too dark to repeat it even two times. I understand problems with spaces... But may be it would be better somehow narrow it to one ugly print... Print "ok %-5i "|"not ok %-5i" to buffer first, and then have one "%s%-*s %8.0f ms%s\n" print or something like that... I am not strongly insisting on that, but I feel strong urge to remove code duplication here... -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su --nextPart2874975.GvFihrIVtr Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEE+sk3ebqQKlezKOi8PMbfuIHAGpgFAmN/pNEACgkQPMbfuIHA GpiuFAgAg7RuRME7rqQ+K0T4rtwHYFmQU/pePXl/z4CUXWnzL2X28Xxk6l+vrHYs 4dx30bNf+YEPqLESCOu2f2aCsWyAxOvo0MIaMufuxGHnIjFK55WD67FWuAS2Fnu5 kxQ05KSGnPr0g8Wle20N3ElVPcacDv1t9lbCh4Z0NEV6ccEBcU0J5osIGllYG38E WZNJAdTgzUmWy4L8t22EVDd9JjdRrILRxoXDVOPHoimi7TotXtGgMs8b7ctXlv/U Lgfffzi3LuOlgvMxmWKbcuEo/pUW+c5g0oJm+SO4uW6xYFVAlUnyexOEQuH+dSQA Ue3D/mepIRGrrKNtgA5qv/tUl22Uww== =8dJ0 -----END PGP SIGNATURE----- --nextPart2874975.GvFihrIVtr--