public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikolay Shaplov <[email protected]>
To: Andres Freund <[email protected]>
To: Daniel Gustafsson <[email protected]>
Cc: Dagfinn Ilmari Mannsåker <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: TAP output format in pg_regress
Date: Sat, 26 Nov 2022 22:37:08 +0300
Message-ID: <4313529.Z4tMiuZmk9@thinkpad-pgpro> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
В письме от пятница, 25 ноября 2022 г. 00:20:01 MSK пользователь Daniel
Gustafsson написал:
+ /*
+ * The width of the testname field when printing to ensure vertical
alignment
+ * of test runtimes. Thius number is somewhat arbitrarily chosen to match
the
+ * older pre-TAP output format.
+ */
"Thius" seems to be a typo :-)
-----
+ #define bail_noatexit(...) bail_out(true, __VA_ARGS__)
BTW what does "noat" stands for? I thought it is typo too :-) and originally
meant to be "not".
-----
- snprintf(buf, sizeof(buf),
- _(" All %d tests passed. "),
- success_count);
- else if (fail_count == 0) /* fail_count=0, fail_ignore_count>0 */
- snprintf(buf, sizeof(buf),
- _(" %d of %d tests passed, %d failed test(s) ignored. "),
- success_count,
- success_count + fail_ignore_count,
- fail_ignore_count);
- else if (fail_ignore_count == 0) /* fail_count>0 && fail_ignore_count=0
*/
- snprintf(buf, sizeof(buf),
- _(" %d of %d tests failed. "),
- fail_count,
- success_count + fail_count);
+ note(_("All %d tests passed.\n"), success_count);
+ /* fail_count=0, fail_ignore_count>0 */
+ else if (fail_count == 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=0 */
+ else if (fail_ignore_count == 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, elses
and may be indent offsets of lines that did not change as they were to have
nicer diff? Would make understanding this changeset more easy... Or this is
work of pg_indent that spoils it?
----
While looking at the my output I am getting wrong offset for
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
select_distinct_on namespace select_implicit case prepared_xacts subselect
transactions portals select_distinct union arrays update hash_index join
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...
----
As for the rest: I see no other problems in the code, and consider it should
be passed to commiter (or may be more experienced reviewer)
> > On 24 Nov 2022, at 20:32, Andres Freund <[email protected]> wrote:
> >
> > On November 24, 2022 11:07:43 AM PST, Daniel Gustafsson <[email protected]>
wrote:
> >>> On 24 Nov 2022, at 18:07, Nikolay Shaplov <[email protected]> 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.
> >>
> >> -bail_out(bool non_rec, const char *fmt,...)
> >> +bail(void (*exit_func)(int), const char *fmt,...)
> >>
> >> 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.
> >
> > I'd just rename _bail to bail_noatexit().
>
> That's probably the best option, done in the attached along with the comment
> fixup to mention the recursion issue.
>
> >>> 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'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.>
> > 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 them
> > 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..).
>
> --
> Daniel Gustafsson https://vmware.com/
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
Attachments:
[application/pgp-signature] signature.asc (488B, ../4313529.Z4tMiuZmk9@thinkpad-pgpro/2-signature.asc)
download
view thread (40+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: TAP output format in pg_regress
In-Reply-To: <4313529.Z4tMiuZmk9@thinkpad-pgpro>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox