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

Hi,

On 2022-11-17 11:36:13 +0100, Daniel Gustafsson wrote:
> > On 10 Nov 2022, at 11:44, Nikolay Shaplov <dhyan@nataraj.su> wrote:
> > Did not found quick way to include prove TAP harness right into Makefile, so I
> > check dumped output, but it is not really important for now, I guess.
>
> I think we'll start by adding TAP to the meson testrunner and await to see
> where the make buildsystem ends up before doing anything there.

+1

prove IME is of, uh, very questionable quality IME. I do not want to run
pg_regress/isolation tests through that unnecessarily. It'd not gain us much
anyway, afaict. And we don't want it as a hard dependency either.


> I'm not sure if that's the right way to go about configuring regress tests as
> TAP emitting, but it at least shows that meson is properly parsing the output
> AFAICT.

Looks correct to me.


> @@ -742,13 +823,13 @@ initialize_environment(void)
>          }
>
>          if (pghost && pgport)
> -            printf(_("(using postmaster on %s, port %s)\n"), pghost, pgport);
> +            status(_("# (using postmaster on %s, port %s)\n"), pghost, pgport);
>          if (pghost && !pgport)
> -            printf(_("(using postmaster on %s, default port)\n"), pghost);
> +            status(_("# (using postmaster on %s, default port)\n"), pghost);
>          if (!pghost && pgport)
> -            printf(_("(using postmaster on Unix socket, port %s)\n"), pgport);
> +            status(_("# (using postmaster on Unix socket, port %s)\n"), pgport);
>          if (!pghost && !pgport)
> -            printf(_("(using postmaster on Unix socket, default port)\n"));
> +            status(_("# (using postmaster on Unix socket, default port)\n"));
>      }

It doesn't seem quite right to include the '# ' bit in the translated
string. If a translator were to not include it we'd suddenly have incorrect
tap output. That's perhaps not likely, but ... It's also not really necessary
to have it in as many translated strings?

Not that I understand why we even make these strings translatable. It seems
like it'd be a bad idea to translate this kind of output.

But either way, it seems nicer to output the # inside a helper function?


> +    /*
> +     * In order for this information (or any error information) to be shown
> +     * when running pg_regress test suites under prove it needs to be emitted
> +     * stderr instead of stdout.
> +     */
>      if (file_size(difffilename) > 0)
>      {
> -        printf(_("The differences that caused some tests to fail can be viewed in the\n"
> -                 "file \"%s\".  A copy of the test summary that you see\n"
> -                 "above is saved in the file \"%s\".\n\n"),
> +        status(_("\n# The differences that caused some tests to fail can be viewed in the\n"
> +                  "# file \"%s\".  A copy of the test summary that you see\n"
> +                  "# above is saved in the file \"%s\".\n\n"),
>                 difffilename, logfilename);

The comment about needing to print to stderr is correct - but the patch
doesn't do so (anymore)?

The count of failed tests also should go to stderr (but not the report of all
tests having passed).

bail() probably also ought to print the error to stderr, so the most important
detail is immediately visible when looking at the failed test result.

Greetings,

Andres Freund