Re: TAP output format in pg_regress - Mailing list pgsql-hackers

В письме от суббота, 26 ноября 2022 г. 23:35:45 MSK пользователь Daniel
Gustafsson написал:
> > + #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".
>
> 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 what
is happening here.

I wold suggest to use word immediate instead of noatexit. This will do the
code much more sensible for me.

-------
/*
 * Bailing out is for unrecoverable errors which prevents further testing to
 * occur and after which the test run should be aborted. By passing immediate
 * 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__)
-------

I've also rewritten the comment, the way I would understand it better, if I
read it for the first time. I am not sure about my English, but key features
there are:

- "terminate process" instead of "exit". Too many exist in the sentence,
better to use synonyms wherever is possible.
- "_exit() instead of exit()" more accurate description of what is happening
here
- Split this sentence into two. First sentence: what does it do. Second
sentence: why it does so.
- Added "infinite" word before "recursion". Recursion is not a bad thing,
infinite recursion is. This explicitly state what we are trying to avoid.

======

> 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
guess this part should be looking like that:

-               snprintf(buf, sizeof(buf),
-                                _(" All %d tests passed. "),
+                  note(_("All %d tests passed.\n"),
                                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. "),
+               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 == 0)        /* fail_count>0 &&
fail_ignore_count=0 */
-               snprintf(buf, sizeof(buf),
-                                _(" %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.

> While
> avoiding moving the comments to before the line might mitigate that somewhat
> 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
behind "prefer", but if it is important for you, I will no longer insist.

=====
> 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 test
> suites, and we are already today using indentation for grouping parallel
> tests.

So this extra offset indicates that test is being included into parallel
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 scope
of this path.

Theoretically TAP 14 has subtests and this parallel tests looks like
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...

--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su