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 1ozEpT-00059c-HD for pgsql-hackers@arkaria.postgresql.org; Sun, 27 Nov 2022 10:24:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ozEoS-0008MJ-NH for pgsql-hackers@arkaria.postgresql.org; Sun, 27 Nov 2022 10:23:04 +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 1ozEoS-0008JS-Ce for pgsql-hackers@lists.postgresql.org; Sun, 27 Nov 2022 10:23:04 +0000 Received: from mail.nataraj.su ([88.198.198.57]) by magus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ozEoQ-0007IB-8i for pgsql-hackers@lists.postgresql.org; Sun, 27 Nov 2022 10:23:03 +0000 Received: from thinkpad-pgpro.localnet (unknown [91.203.188.92]) by mail.nataraj.su (Postfix) with ESMTPSA id C0AC39CA1E; Sun, 27 Nov 2022 10:22:59 +0000 (UTC) From: Nikolay Shaplov To: Daniel Gustafsson Cc: Andres Freund , 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: Sun, 27 Nov 2022 13:22:58 +0300 Message-ID: <8151488.aLmcGXPk4V@thinkpad-pgpro> Organization: Postgres Professional In-Reply-To: References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com> <4313529.Z4tMiuZmk9@thinkpad-pgpro> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart27710351.Amf9mX20Rh"; micalg="pgp-sha512"; protocol="application/pgp-signature" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --nextPart27710351.Amf9mX20Rh Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8"; protected-headers="v1" From: Nikolay Shaplov To: Daniel Gustafsson Cc: Andres Freund , 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: Sun, 27 Nov 2022 13:22:58 +0300 Message-ID: <8151488.aLmcGXPk4V@thinkpad-pgpro> Organization: Postgres Professional In-Reply-To: References: <5e6bc6a6-0a4e-109d-2b7d-c286dc58d609@enterprisedb.com> <4313529.Z4tMiuZmk9@thinkpad-pgpro> =D0=92 =D0=BF=D0=B8=D1=81=D1=8C=D0=BC=D0=B5 =D0=BE=D1=82 =D1=81=D1=83=D0=B1= =D0=B1=D0=BE=D1=82=D0=B0, 26 =D0=BD=D0=BE=D1=8F=D0=B1=D1=80=D1=8F 2022 =D0= =B3. 23:35:45 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: > > + #define bail_noatexit(...) bail_out(true, __VA_ARGS__) > >=20 > > BTW what does "noat" stands for? I thought it is typo too :-) and > > originally meant to be "not". >=20 > Calling _exit() will cause exit handler functions registered with atexit() > to not be invoked, no noatexit was intentional spelling. I've read some mans: The function _exit() terminates the calling process "immediately". I guess, "immediately" is a good word here that very precisely describes wh= at=20 is happening here.=20 I wold suggest to use word immediate instead of noatexit. This will do the= =20 code much more sensible for me.=20 =2D------ /* * Bailing out is for unrecoverable errors which prevents further testing to * occur and after which the test run should be aborted. By passing immedia= te * as true the process will terminate process with _exit() instead of exit(= ). * This will allow to skip registered exit handlers, thus avoid possible * infinite recursive calls while exiting. */ static void bail_out(bool immediate, const char *fmt,...) { va_list ap; va_start(ap, fmt); emit_tap_output_v(BAIL, fmt, ap); va_end(ap); if (immediate) _exit(2); exit(2); } #define bail_immediate(...) bail_out(true, __VA_ARGS__) #define bail(...) bail_out(false, __VA_ARGS__) =2D------ I've also rewritten the comment, the way I would understand it better, if I= =20 read it for the first time. I am not sure about my English, but key feature= s=20 there are:=20 =2D "terminate process" instead of "exit". Too many exist in the sentence,= =20 better to use synonyms wherever is possible. =2D "_exit() instead of exit()" more accurate description of what is happen= ing=20 here =2D Split this sentence into two. First sentence: what does it do. Second=20 sentence: why it does so. =2D Added "infinite" word before "recursion". Recursion is not a bad thing,= =20 infinite recursion is. This explicitly state what we are trying to avoid. =3D=3D=3D=3D=3D=3D > The diff algorithm decided that this was the compact way of displaying the > unified diff, probably because too many lines in proximity changed. When line is not changed it is never added to a change block by diff... I=20 guess this part should be looking like that: =2D snprintf(buf, sizeof(buf), =2D _(" All %d tests passed. "), + note(_("All %d tests passed.\n"),=20 success_count); else if (fail_count =3D=3D 0) /* fail_count=3D0, fail_ignore_c= ount>0 */ =2D snprintf(buf, sizeof(buf), =2D _(" %d of %d tests passed, %d failed tes= t(s)=20 ignored. "), + note(_("%d of %d tests passed, %d failed test(s) ignored.\n= "), success_count, success_count + fail_ignore_count, fail_ignore_count); else if (fail_ignore_count =3D=3D 0) /* fail_count>0 &&=20 fail_ignore_count=3D0 */ =2D snprintf(buf, sizeof(buf), =2D _(" %d of %d tests failed. "), + diag(_("%d of %d tests failed.\n"), fail_count, success_count + fail_count); Working with my students I usually insist them to provide such patches. =20 > While > avoiding moving the comments to before the line might mitigate that somew= hat > I prefer this greatly to a slightly easier to read diff. I do not quite understand what are you trying to achieve here, what stands= =20 behind "prefer", but if it is important for you, I will no longer insist. =3D=3D=3D=3D=3D=20 > No, they are aligned in such a way because they are running outside of a > parallel group. Note that it's not part of the "parallel group" note > preceeding the tests: > In the previous format it's a bit clearer, and maybe we should adopt that > for > TAP as well? > That would if so make the output something like the below. Personally I > think the "test" prefix adds little value since everything printed are te= st > suites, and we are already today using indentation for grouping parallel > tests. So this extra offset indicates that test is being included into parallel=20 group? Guess it not really obvious... I am not sure I have a clear idea what can be done here. May be it some ideal I would give each group a name. Like ok 8 types.int2 45 ms ok 9 types.int4 73 ms ok 10 types.int8 91 ms ok 11 types.oid 47 ms ok 12 types.float4 88 ms ok 13 types.float8 139 ms ok 14 types.bit 165 ms ok 15 types.numeric 1065 ms but this does not seems very realistic in the current code base and the sco= pe=20 of this path. Theoretically TAP 14 has subtests and this parallel tests looks like=20 subtests... but TAP 14 is not supported by modern harnesses.. So I have no idea... May be leaving it as it is for now, is good enough... =2D-=20 Nikolay Shaplov aka Nataraj =46uzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su --nextPart27710351.Amf9mX20Rh 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+sk3ebqQKlezKOi8PMbfuIHAGpgFAmODOoIACgkQPMbfuIHA GpjSUAgAvWG1MIM0Jhp3kT2Y6svFj9Phtho8W+74rFeF6k1ewPuDBZ9C3sw1OraN KyqEFGHuVyjwQh+MtY2v9EAt/rCr0W0nDTELpRK/GJHvxrpLHrLYEfHJp9lxWpD1 NG4UmrKRWP0Axt/VmhG7pfkO2/2b0MoArvALWlIZ+ahEgAUj/IS0fP/9ajBfvHLn GpEgxMIZoXkvFgcHkyhSB1lD9pMXuxTxiPlRfZKo1px6IdlBjqzU83B5fcfLgN7t cSHPBdYBRjRRcA6VUwzxrC6DREh5mkLMQvkox/mrgdohXrtYDCrq1qQ1c5A8AM6w jN2oAALG9kC1hJEjptWKnQ23ovMvVg== =URgj -----END PGP SIGNATURE----- --nextPart27710351.Amf9mX20Rh--