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 1oz0zF-00067c-Gv for pgsql-hackers@arkaria.postgresql.org; Sat, 26 Nov 2022 19:37:17 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oz0zD-0002cY-Fn for pgsql-hackers@arkaria.postgresql.org; Sat, 26 Nov 2022 19:37:15 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oz0zD-0002cO-25 for pgsql-hackers@lists.postgresql.org; Sat, 26 Nov 2022 19:37:15 +0000 Received: from mail.nataraj.su ([88.198.198.57]) by makus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oz0z9-00037j-Vr for pgsql-hackers@lists.postgresql.org; Sat, 26 Nov 2022 19:37:13 +0000 Received: from thinkpad-pgpro.localnet (unknown [91.203.188.92]) by mail.nataraj.su (Postfix) with ESMTPSA id 90AB09D94D; Sat, 26 Nov 2022 19:37:09 +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: Sat, 26 Nov 2022 22:37:08 +0300 Message-ID: <4313529.Z4tMiuZmk9@thinkpad-pgpro> Organization: Postgres Professional In-Reply-To: <467AAA52-CD0E-4B33-ACA4-6AEA1384E473@yesql.se> References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com> <467AAA52-CD0E-4B33-ACA4-6AEA1384E473@yesql.se> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4209374.tBYCRv8kRr"; micalg="pgp-sha512"; protocol="application/pgp-signature" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --nextPart4209374.tBYCRv8kRr Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8"; 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: Sat, 26 Nov 2022 22:37:08 +0300 Message-ID: <4313529.Z4tMiuZmk9@thinkpad-pgpro> Organization: Postgres Professional In-Reply-To: <467AAA52-CD0E-4B33-ACA4-6AEA1384E473@yesql.se> References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com> <467AAA52-CD0E-4B33-ACA4-6AEA1384E473@yesql.se> =D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D0=BF=D1=8F=D1=82= =D0=BD=D0=B8=D1=86=D0=B0, 25 =D0=BD=D0=BE=D1=8F=D0=B1=D1=80=D1=8F 2022 =D0= =B3. 00:20:01 MSK =D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=82=D0= =B5=D0=BB=D1=8C Daniel=20 Gustafsson =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB: + /* = =20 + * The width of the testname field when printing to ensure vertical=20 alignment =20 + * of test runtimes. Thius number is somewhat arbitrarily chosen to match= =20 the =20 + * older pre-TAP output format. = =20 + */ "Thius" seems to be a typo :-) =2D---- + #define bail_noatexit(...) bail_out(true, __VA_ARGS__) BTW what does "noat" stands for? I thought it is typo too :-) and originall= y=20 meant to be "not".=20 =2D---- =2D snprintf(buf, sizeof(buf), =2D _(" All %d tests passed. "), =2D success_count); =2D else if (fail_count =3D=3D 0) /* fail_count=3D0, fail_ignore_count>= 0 */ =2D snprintf(buf, sizeof(buf), =2D _(" %d of %d tests passed, %d failed test(s) ignored. "), =2D success_count, =2D success_count + fail_ignore_count, =2D fail_ignore_count); =2D else if (fail_ignore_count =3D=3D 0) /* fail_count>0 && fail_ignor= e_count=3D0=20 */ =2D snprintf(buf, sizeof(buf), =2D _(" %d of %d tests failed. "), =2D fail_count, =2D success_count + fail_count); + note(_("All %d tests passed.\n"), success_count); + /* fail_count=3D0, fail_ignore_count>0 */ + else if (fail_count =3D=3D 0) + note(_("%d of %d tests passed, %d failed test(s) ignored.\n"), + success_count, + success_count + fail_ignore_count, + fail_ignore_count); + /* fail_count>0 && fail_ignore_count=3D0 */ + else if (fail_ignore_count =3D=3D 0) + diag(_("%d of %d tests failed.\n"), + fail_count, + success_count + fail_count); + /* fail_count>0 && fail_ignore_count>0 */ Just out of overaccuracy: Logic here have not changed. Can we keep ifs, el= ses=20 and may be indent offsets of lines that did not change as they were to have= =20 nicer diff? Would make understanding this changeset more easy... Or this is= =20 work of pg_indent that spoils it? =2D--- While looking at the my output I am getting wrong offset for=20 sanity_check: ok 84 hash_func 121 ms ok 85 errors 68 ms ok 86 infinite_recurse 233 ms ok 87 sanity_check 144 ms # parallel group (20 tests): select_into delete random select_having=20 select_distinct_on namespace select_implicit case prepared_xacts subselect= =20 transactions portals select_distinct union arrays update hash_index join=20 aggregates btree_index ok 88 select_into 134 ms ok 89 select_distinct 812 ms (also for select_parallel write_parallel vacuum_parallel and fast_default) I guess the intention was to align them too... =2D--- As for the rest: I see no other problems in the code, and consider it shoul= d=20 be passed to commiter (or may be more experienced reviewer) > > On 24 Nov 2022, at 20:32, Andres Freund wrote: > >=20 > > On November 24, 2022 11:07:43 AM PST, Daniel Gustafsson =20 wrote: > >>> On 24 Nov 2022, at 18:07, Nikolay Shaplov wrote: > >> One option could be to redefine bail() to take the exit function as a > >> parameter and have the caller pass the preferred exit handler. > >>=20 > >> -bail_out(bool non_rec, const char *fmt,...) > >> +bail(void (*exit_func)(int), const char *fmt,...) > >>=20 > >> The callsites would then look like the below, which puts a reference to > >> the > >> actual exit handler used in the code where it is called. > >=20 > > I'd just rename _bail to bail_noatexit(). >=20 > That's probably the best option, done in the attached along with the comm= ent > fixup to mention the recursion issue. >=20 > >>> 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= =20 > >>> "|"not ok %-5i" to buffer first, and then have one "%s%-*s %8.0f > >>> ms%s\n" print or something like that... > >>=20 > >> I'm not convinced that this printf format is that hard to read (which = may > >> well be attributed to Stockholm Syndrome), and I do think that breaking > >> it up and adding more code to print the line will make it less readable > >> instead.>=20 > > I don't think it's terrible either. I do think it'd also be ok to switch > > between ok / not ok within a single printf, making it easier to keep th= em > > in sync. > I made it into a single printf to see what it would look like, with some > additional comments to make it more readable (I'm not a fan of where > pgindent moves those but..). >=20 > -- > Daniel Gustafsson https://vmware.com/ =2D-=20 Nikolay Shaplov aka Nataraj =46uzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su --nextPart4209374.tBYCRv8kRr 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+sk3ebqQKlezKOi8PMbfuIHAGpgFAmOCauQACgkQPMbfuIHA GpiJGQf/ZI1nHilYpVGOU+Br3deqV6KkeLk/O2StGOYug8EZVJA5A8j1F5AmNPbv ne50j2xTxcOUeF5irMqCiWA38atOIvjJ8seWiUTkepSlKhkCH151VaEjzkeONxRA 8I4YZaRn2siNYfwZICCH85x+zuZEIYLb/iqOOGx0lsXa4oNNljIZy+k43tapsROM mvSOogrJGJ88NZF9/ptECsZgzoPVcoQTy5nSs7x9swt3mCO1wAYmta0B97VhNGYV jN5A2Y01CPbFfJmFyFftwmXNr7bJQg5RLo5EOO7A2Ee3Ed0rDlCfxySuvScKuG5j 5hA0mfmAwMsUODZoUTCYJZSlBAxbYA== =GoUv -----END PGP SIGNATURE----- --nextPart4209374.tBYCRv8kRr--