Subject: Re: TAP output format in pg_regress

From: vignesh C <[email-masked]>
Date: Tue, 3 Jan 2023 16:01:29 +0530

On Tue, 29 Nov 2022 at 00:57, Daniel Gustafsson <[email-masked]> wrote:
>
> > On 28 Nov 2022, at 20:02, Nikolay Shaplov <[email-masked]> wrote:
>
> > From my reviewer's point of view patch is ready for commit.
> >
> > Thank you for your patience :-)
>
> Thanks for review.
>
> The attached tweaks a few comments and attempts to address the compiler warning
> error in the CFBot CI.  Not sure I entirely agree with the compiler there but
> here is an attempt to work around it at least (by always copying the va_list
> for the logfile). It also adds a missing va_end for the logfile va_list.
>
> I hope this is close to a final version of this patch (commitmessage needs a
> bit of work though).
>
> > PS Should I change commitfest status?
>
> Sure, go ahead.
>

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
92957ed98c5c565362ce665266132a7f08f6b0c0 ===
=== applying patch
./v14-0001-Change-pg_regress-output-format-to-be-TAP-compli.patch
patching file meson.build
Hunk #1 FAILED at 2968.
1 out of 1 hunk FAILED -- saving rejects to file meson.build.rej

[1] - http://cfbot.cputube.org/patch_41_3837.log

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Fri, 6 Jan 2023 11:20:09 +0530

> 1 out of 1 hunk FAILED -- saving rejects to file meson.build.rej

Attached a rebased patch on top of HEAD to try and see if we can close
this patch in this commitfest.

Regards,
Vignesh
From 005bcf13037577a8682e3a6365b1b52bb2e48492 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <[email-masked]>
Date: Fri, 6 Jan 2023 11:09:10 +0530
Subject: [PATCH v15] Change pg_regress output format to be TAP compliant

This converts pg_regress output format to emit TAP complient output
while keeping it as human readable as possible for use without TAP
test harnesses. As verbose harness related information isn't really
supported by TAP this also reduces the verbosity of pg_regress runs
which makes scrolling through log output in buildfarm/CI runs a bit
easier as well.

TAP format testing is also enabled in Meson as of this.

Discussion: https://postgr.es/m/[email-masked]
---
 meson.build                   |   1 +
 src/test/regress/pg_regress.c | 575 ++++++++++++++++++----------------
 2 files changed, 314 insertions(+), 262 deletions(-)

diff --git a/meson.build b/meson.build
index 45fb9dd616..7195937d17 100644
--- a/meson.build
+++ b/meson.build
@@ -3024,6 +3024,7 @@ foreach test_dir : tests
       env.prepend('PATH', temp_install_bindir, test_dir['bd'])
 
       test_kwargs = {
+        'protocol': 'tap',
         'priority': 10,
         'timeout': 1000,
         'depends': test_deps + t.get('deps', []),
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 40e6c231a3..a05f423c75 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -68,6 +68,26 @@ const char *basic_diff_opts = "-w";
 const char *pretty_diff_opts = "-w -U3";
 #endif
 
+/* For parallel tests, testnames are indented when printed for grouping */
+#define PARALLEL_INDENT 7
+/*
+ * The width of the testname field when printing to ensure vertical alignment
+ * of test runtimes. This number is somewhat arbitrarily chosen to match the
+ * older pre-TAP output format.
+ */
+#define TESTNAME_WIDTH 36
+
+typedef enum TAPtype
+{
+	DIAG = 0,
+	BAIL,
+	NOTE,
+	DETAIL,
+	TEST_STATUS,
+	PLAN,
+	NONE
+}			TAPtype;
+
 /* options settable from command line */
 _stringlist *dblist = NULL;
 bool		debug = false;
@@ -103,6 +123,7 @@ static const char *sockdir;
 static const char *temp_sockdir;
 static char sockself[MAXPGPATH];
 static char socklock[MAXPGPATH];
+static StringInfo failed_tests = NULL;
 
 static _resultmap *resultmap = NULL;
 
@@ -116,12 +137,29 @@ static int	fail_ignore_count = 0;
 static bool directory_exists(const char *dir);
 static void make_directory(const char *dir);
 
-static void header(const char *fmt,...) pg_attribute_printf(1, 2);
-static void status(const char *fmt,...) pg_attribute_printf(1, 2);
+static void test_status_print(bool ok, const char *testname, double runtime, bool parallel, bool ignore);
+static void test_status_ok(const char *testname, double runtime, bool parallel);
+static void test_status_failed(const char *testname, bool ignore, double runtime, bool parallel);
+static void bail_out(bool noatexit, const char *fmt,...) pg_attribute_printf(2, 3);
+static void emit_tap_output(TAPtype type, const char *fmt,...) pg_attribute_printf(2, 3);
+static void emit_tap_output_v(TAPtype type, const char *fmt, va_list argp) pg_attribute_printf(2, 0);
+
 static StringInfo psql_start_command(void);
 static void psql_add_command(StringInfo buf, const char *query,...) pg_attribute_printf(2, 3);
 static void psql_end_command(StringInfo buf, const char *database);
 
+/*
+ * Convenience macros for printing TAP output with a more shorthand syntax
+ * aimed at making the code more readable.
+ */
+#define plan(x)				emit_tap_output(PLAN, "1..%i\n", (x))
+#define note(...)			emit_tap_output(NOTE, __VA_ARGS__)
+#define note_detail(...)	emit_tap_output(DETAIL, __VA_ARGS__)
+#define diag(...)			emit_tap_output(DIAG, __VA_ARGS__)
+#define note_end()			emit_tap_output(NONE, "\n");
+#define bail_noatexit(...)	bail_out(true, __VA_ARGS__)
+#define bail(...)			bail_out(false, __VA_ARGS__)
+
 /*
  * allow core files if possible.
  */
@@ -134,9 +172,7 @@ unlimit_core_size(void)
 	getrlimit(RLIMIT_CORE, &lim);
 	if (lim.rlim_max == 0)
 	{
-		fprintf(stderr,
-				_("%s: could not set core size: disallowed by hard limit\n"),
-				progname);
+		diag(_("could not set core size: disallowed by hard limit\n"));
 		return;
 	}
 	else if (lim.rlim_max == RLIM_INFINITY || lim.rlim_cur < lim.rlim_max)
@@ -202,53 +238,153 @@ split_to_stringlist(const char *s, const char *delim, _stringlist **listhead)
 }
 
 /*
- * Print a progress banner on stdout.
+ * Bailing out is for unrecoverable errors which prevents further testing to
+ * occur and after which the test run should be aborted. By passing noatexit
+ * as true the process will terminate with _exit(2) and skipping registered
+ * exit handlers, thus avoid any risk of bottomless recursion calls to exit.
  */
 static void
-header(const char *fmt,...)
+bail_out(bool noatexit, const char *fmt,...)
 {
-	char		tmp[64];
 	va_list		ap;
 
 	va_start(ap, fmt);
-	vsnprintf(tmp, sizeof(tmp), fmt, ap);
+	emit_tap_output_v(BAIL, fmt, ap);
 	va_end(ap);
 
-	fprintf(stdout, "============== %-38s ==============\n", tmp);
-	fflush(stdout);
+	if (noatexit)
+		_exit(2);
+
+	exit(2);
 }
 
-/*
- * Print "doing something ..." --- supplied text should not end with newline
- */
 static void
-status(const char *fmt,...)
+test_status_print(bool ok, const char *testname, double runtime, bool parallel, bool ignore)
 {
-	va_list		ap;
+	int			testnumber;
+	int			padding;
 
-	va_start(ap, fmt);
-	vfprintf(stdout, fmt, ap);
-	fflush(stdout);
-	va_end(ap);
+	testnumber = fail_count + fail_ignore_count + success_count;
+	padding = TESTNAME_WIDTH;
 
-	if (logfile)
+	/*
+	 * Calculate the width for the testname field required to align runtimes
+	 * vertically.
+	 */
+	if (parallel)
+		padding -= PARALLEL_INDENT;
+
+	/*
+	 * There is no NLS translation here as "(not) ok" and "SKIP" are protocol.
+	 * Testnumbers are padded to 5 characters to ensure that testnames align
+	 * vertically (assuming at most 9999 tests).  Testnames are indented 8
+	 * spaces in case they run as part of a parallel group. The position for
+	 * the runtime is offset based on that indentation.
+	 */
+	emit_tap_output(TEST_STATUS, "%sok %-5i%*s %*s%-*s %8.0f ms%s\n",
+					(ok ? "" : "not "),
+					testnumber,
+	/* If ok, indent with four spaces matching "not " */
+					(ok ? (int) strlen("not ") : 0), "",
+	/* If parallel, indent to indicate grouping */
+					(parallel ? PARALLEL_INDENT : 0), "",
+	/* Testnames are padded to align runtimes */
+					padding, testname,
+					runtime,
+	/* Add an (igngored) comment on the SKIP line to clarify */
+					(ignore ? " # SKIP (ignored)" : ""));
+}
+
+static void
+test_status_ok(const char *testname, double runtime, bool parallel)
+{
+	success_count++;
+
+	test_status_print(true, testname, runtime, parallel, false);
+}
+
+static void
+test_status_failed(const char *testname, bool ignore, double runtime, bool parallel)
+{
+	/*
+	 * Save failed tests in a buffer such that we can print a summary at the
+	 * end with diag() to ensure it's shown even under test harnesses.
+	 */
+	if (!failed_tests)
+		failed_tests = makeStringInfo();
+	else
+		appendStringInfoChar(failed_tests, ',');
+
+	appendStringInfo(failed_tests, " %s", testname);
+
+	if (ignore)
 	{
-		va_start(ap, fmt);
-		vfprintf(logfile, fmt, ap);
-		va_end(ap);
+		fail_ignore_count++;
+		appendStringInfoString(failed_tests, " (ignored)");
 	}
+	else
+		fail_count++;
+
+	test_status_print(false, testname, runtime, parallel, ignore);
+}
+
+
+static void
+emit_tap_output(TAPtype type, const char *fmt,...)
+{
+	va_list		argp;
+
+	va_start(argp, fmt);
+	emit_tap_output_v(type, fmt, argp);
+	va_end(argp);
 }
 
-/*
- * Done "doing something ..."
- */
 static void
-status_end(void)
+emit_tap_output_v(TAPtype type, const char *fmt, va_list argp)
 {
-	fprintf(stdout, "\n");
-	fflush(stdout);
+	va_list		argp_logfile;
+	FILE	   *fp;
+
+	/* Make a copy of the va args for printing to the logfile */
+	va_copy(argp_logfile, argp);
+
+	/*
+	 * Diagnostic output will be hidden by prove unless printed to stderr. The
+	 * Bail message is also printed to stderr to aid debugging under a harness
+	 * which might otherwise not emit such an important message.
+	 */
+	if (type == DIAG || type == BAIL)
+		fp = stderr;
+	else
+		fp = stdout;
+
+	/*
+	 * Non-protocol output such as diagnostics or notes must be prefixed by a
+	 * '#' character. We print the Bail message like this too.
+	 */
+	if (type == NOTE || type == DIAG || type == BAIL)
+	{
+		fprintf(fp, "# ");
+		if (logfile)
+			fprintf(logfile, "# ");
+	}
+	vfprintf(fp, fmt, argp);
 	if (logfile)
-		fprintf(logfile, "\n");
+		vfprintf(logfile, fmt, argp_logfile);
+
+	/*
+	 * If this was a Bail message, the bail protocol message must go to stdout
+	 * separately.
+	 */
+	if (type == BAIL)
+	{
+		fprintf(stdout, "Bail Out!\n");
+		if (logfile)
+			fprintf(logfile, "Bail Out!\n");
+	}
+
+	va_end(argp_logfile);
+	fflush(NULL);
 }
 
 /*
@@ -272,9 +408,9 @@ stop_postmaster(void)
 		r = system(buf);
 		if (r != 0)
 		{
-			fprintf(stderr, _("\n%s: could not stop postmaster: exit code was %d\n"),
-					progname, r);
-			_exit(2);			/* not exit(), that could be recursive */
+			/* Not using the normal bail() as we want _exit */
+			bail_noatexit(_("could not stop postmaster: exit code was %d\n"),
+						  r);
 		}
 
 		postmaster_running = false;
@@ -332,9 +468,8 @@ make_temp_sockdir(void)
 	temp_sockdir = mkdtemp(template);
 	if (temp_sockdir == NULL)
 	{
-		fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"),
-				progname, template, strerror(errno));
-		exit(2);
+		bail(_("could not create directory \"%s\": %s\n"),
+			 template, strerror(errno));
 	}
 
 	/* Stage file names for remove_temp().  Unsafe in a signal handler. */
@@ -456,9 +591,8 @@ load_resultmap(void)
 		/* OK if it doesn't exist, else complain */
 		if (errno == ENOENT)
 			return;
-		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
-				progname, buf, strerror(errno));
-		exit(2);
+		bail(_("could not open file \"%s\" for reading: %s\n"),
+			 buf, strerror(errno));
 	}
 
 	while (fgets(buf, sizeof(buf), f))
@@ -477,26 +611,20 @@ load_resultmap(void)
 		file_type = strchr(buf, ':');
 		if (!file_type)
 		{
-			fprintf(stderr, _("incorrectly formatted resultmap entry: %s\n"),
-					buf);
-			exit(2);
+			bail(_("incorrectly formatted resultmap entry: %s\n"), buf);
 		}
 		*file_type++ = '\0';
 
 		platform = strchr(file_type, ':');
 		if (!platform)
 		{
-			fprintf(stderr, _("incorrectly formatted resultmap entry: %s\n"),
-					buf);
-			exit(2);
+			bail(_("incorrectly formatted resultmap entry: %s\n"), buf);
 		}
 		*platform++ = '\0';
 		expected = strchr(platform, '=');
 		if (!expected)
 		{
-			fprintf(stderr, _("incorrectly formatted resultmap entry: %s\n"),
-					buf);
-			exit(2);
+			bail(_("incorrectly formatted resultmap entry: %s\n"), buf);
 		}
 		*expected++ = '\0';
 
@@ -742,13 +870,13 @@ initialize_environment(void)
 		}
 
 		if (pghost && pgport)
-			printf(_("(using postmaster on %s, port %s)\n"), pghost, pgport);
+			note(_("(using postmaster on %s, port %s)\n"), pghost, pgport);
 		if (pghost && !pgport)
-			printf(_("(using postmaster on %s, default port)\n"), pghost);
+			note(_("(using postmaster on %s, default port)\n"), pghost);
 		if (!pghost && pgport)
-			printf(_("(using postmaster on Unix socket, port %s)\n"), pgport);
+			note(_("(using postmaster on Unix socket, port %s)\n"), pgport);
 		if (!pghost && !pgport)
-			printf(_("(using postmaster on Unix socket, default port)\n"));
+			note(_("(using postmaster on Unix socket, default port)\n"));
 	}
 
 	load_resultmap();
@@ -797,35 +925,27 @@ current_windows_user(const char **acct, const char **dom)
 
 	if (!OpenProcessToken(GetCurrentProcess(), TOKEN_READ, &token))
 	{
-		fprintf(stderr,
-				_("%s: could not open process token: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
+		bail(_("could not open process token: error code %lu"),
+			 GetLastError());
 	}
 
 	if (!GetTokenInformation(token, TokenUser, NULL, 0, &retlen) && GetLastError() != 122)
 	{
-		fprintf(stderr,
-				_("%s: could not get token information buffer size: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
+		bail(_("could not get token information buffer size: error code %lu"),
+			 GetLastError());
 	}
 	tokenuser = pg_malloc(retlen);
 	if (!GetTokenInformation(token, TokenUser, tokenuser, retlen, &retlen))
 	{
-		fprintf(stderr,
-				_("%s: could not get token information: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
+		bail(_("could not get token information: error code %lu\n"),
+			 GetLastError());
 	}
 
 	if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
 						  domainname, &domainnamesize, &accountnameuse))
 	{
-		fprintf(stderr,
-				_("%s: could not look up account SID: error code %lu\n"),
-				progname, GetLastError());
-		exit(2);
+		bail(_("could not look up account SID: error code %lu\n"),
+			 GetLastError());
 	}
 
 	free(tokenuser);
@@ -974,7 +1094,7 @@ psql_start_command(void)
 	StringInfo	buf = makeStringInfo();
 
 	appendStringInfo(buf,
-					 "\"%s%spsql\" -X",
+					 "\"%s%spsql\" -X -q",
 					 bindir ? bindir : "",
 					 bindir ? "/" : "");
 	return buf;
@@ -1030,8 +1150,7 @@ psql_end_command(StringInfo buf, const char *database)
 	if (system(buf->data) != 0)
 	{
 		/* psql probably already reported the error */
-		fprintf(stderr, _("command failed: %s\n"), buf->data);
-		exit(2);
+		bail(_("command failed: %s\n"), buf->data);
 	}
 
 	/* Clean up */
@@ -1072,9 +1191,7 @@ spawn_process(const char *cmdline)
 	pid = fork();
 	if (pid == -1)
 	{
-		fprintf(stderr, _("%s: could not fork: %s\n"),
-				progname, strerror(errno));
-		exit(2);
+		bail(_("could not fork: %s\n"), strerror(errno));
 	}
 	if (pid == 0)
 	{
@@ -1089,9 +1206,10 @@ spawn_process(const char *cmdline)
 
 		cmdline2 = psprintf("exec %s", cmdline);
 		execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
-		fprintf(stderr, _("%s: could not exec \"%s\": %s\n"),
-				progname, shellprog, strerror(errno));
-		_exit(1);				/* not exit() here... */
+		/* Not using the normal bail() here as we want _exit */
+		bail_noatexit(_("could not exec \"%s\": %s\n"),
+					  shellprog,
+					  strerror(errno));
 	}
 	/* in parent */
 	return pid;
@@ -1129,8 +1247,8 @@ file_size(const char *file)
 
 	if (!f)
 	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
-				progname, file, strerror(errno));
+		diag(_("could not open file \"%s\" for reading: %s\n"),
+			 file, strerror(errno));
 		return -1;
 	}
 	fseek(f, 0, SEEK_END);
@@ -1151,8 +1269,8 @@ file_line_count(const char *file)
 
 	if (!f)
 	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
-				progname, file, strerror(errno));
+		diag(_("could not open file \"%s\" for reading: %s\n"),
+			 file, strerror(errno));
 		return -1;
 	}
 	while ((c = fgetc(f)) != EOF)
@@ -1193,9 +1311,8 @@ make_directory(const char *dir)
 {
 	if (mkdir(dir, S_IRWXU | S_IRWXG | S_IRWXO) < 0)
 	{
-		fprintf(stderr, _("%s: could not create directory \"%s\": %s\n"),
-				progname, dir, strerror(errno));
-		exit(2);
+		bail(_("could not create directory \"%s\": %s\n"),
+			 dir, strerror(errno));
 	}
 }
 
@@ -1245,8 +1362,7 @@ run_diff(const char *cmd, const char *filename)
 	r = system(cmd);
 	if (!WIFEXITED(r) || WEXITSTATUS(r) > 1)
 	{
-		fprintf(stderr, _("diff command failed with status %d: %s\n"), r, cmd);
-		exit(2);
+		bail(_("diff command failed with status %d: %s\n"), r, cmd);
 	}
 #ifdef WIN32
 
@@ -1256,8 +1372,7 @@ run_diff(const char *cmd, const char *filename)
 	 */
 	if (WEXITSTATUS(r) == 1 && file_size(filename) <= 0)
 	{
-		fprintf(stderr, _("diff command not found: %s\n"), cmd);
-		exit(2);
+		bail(_("diff command not found: %s\n"), cmd);
 	}
 #endif
 
@@ -1328,9 +1443,8 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul
 		alt_expectfile = get_alternative_expectfile(expectfile, i);
 		if (!alt_expectfile)
 		{
-			fprintf(stderr, _("Unable to check secondary comparison files: %s\n"),
-					strerror(errno));
-			exit(2);
+			bail(_("Unable to check secondary comparison files: %s\n"),
+				 strerror(errno));
 		}
 
 		if (!file_exists(alt_expectfile))
@@ -1445,9 +1559,8 @@ wait_for_tests(PID_TYPE * pids, int *statuses, instr_time *stoptimes,
 
 		if (p == INVALID_PID)
 		{
-			fprintf(stderr, _("failed to wait for subprocesses: %s\n"),
-					strerror(errno));
-			exit(2);
+			bail(_("failed to wait for subprocesses: %s\n"),
+				 strerror(errno));
 		}
 #else
 		DWORD		exit_status;
@@ -1456,9 +1569,8 @@ wait_for_tests(PID_TYPE * pids, int *statuses, instr_time *stoptimes,
 		r = WaitForMultipleObjects(tests_left, active_pids, FALSE, INFINITE);
 		if (r < WAIT_OBJECT_0 || r >= WAIT_OBJECT_0 + tests_left)
 		{
-			fprintf(stderr, _("failed to wait for subprocesses: error code %lu\n"),
-					GetLastError());
-			exit(2);
+			bail(_("failed to wait for subprocesses: error code %lu\n"),
+				 GetLastError());
 		}
 		p = active_pids[r - WAIT_OBJECT_0];
 		/* compact the active_pids array */
@@ -1477,7 +1589,7 @@ wait_for_tests(PID_TYPE * pids, int *statuses, instr_time *stoptimes,
 				statuses[i] = (int) exit_status;
 				INSTR_TIME_SET_CURRENT(stoptimes[i]);
 				if (names)
-					status(" %s", names[i]);
+					note_detail(" %s", names[i]);
 				tests_left--;
 				break;
 			}
@@ -1496,21 +1608,21 @@ static void
 log_child_failure(int exitstatus)
 {
 	if (WIFEXITED(exitstatus))
-		status(_(" (test process exited with exit code %d)"),
-			   WEXITSTATUS(exitstatus));
+		diag(_("(test process exited with exit code %d)\n"),
+			 WEXITSTATUS(exitstatus));
 	else if (WIFSIGNALED(exitstatus))
 	{
 #if defined(WIN32)
-		status(_(" (test process was terminated by exception 0x%X)"),
-			   WTERMSIG(exitstatus));
+		diag(_("(test process was terminated by exception 0x%X)\n"),
+			 WTERMSIG(exitstatus));
 #else
-		status(_(" (test process was terminated by signal %d: %s)"),
-			   WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));
+		diag(_("(test process was terminated by signal %d: %s)\n"),
+			 WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));
 #endif
 	}
 	else
-		status(_(" (test process exited with unrecognized status %d)"),
-			   exitstatus);
+		diag(_("(test process exited with unrecognized status %d)\n"),
+			 exitstatus);
 }
 
 /*
@@ -1542,9 +1654,8 @@ run_schedule(const char *schedule, test_start_function startfunc,
 	scf = fopen(schedule, "r");
 	if (!scf)
 	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
-				progname, schedule, strerror(errno));
-		exit(2);
+		bail(_("could not open file \"%s\" for reading: %s\n"),
+			 schedule, strerror(errno));
 	}
 
 	while (fgets(scbuf, sizeof(scbuf), scf))
@@ -1582,9 +1693,8 @@ run_schedule(const char *schedule, test_start_function startfunc,
 		}
 		else
 		{
-			fprintf(stderr, _("syntax error in schedule file \"%s\" line %d: %s\n"),
-					schedule, line_num, scbuf);
-			exit(2);
+			bail(_("syntax error in schedule file \"%s\" line %d: %s\n"),
+				 schedule, line_num, scbuf);
 		}
 
 		num_tests = 0;
@@ -1600,9 +1710,8 @@ run_schedule(const char *schedule, test_start_function startfunc,
 
 					if (num_tests >= MAX_PARALLEL_TESTS)
 					{
-						fprintf(stderr, _("too many parallel tests (more than %d) in schedule file \"%s\" line %d: %s\n"),
-								MAX_PARALLEL_TESTS, schedule, line_num, scbuf);
-						exit(2);
+						bail(_("too many parallel tests (more than %d) in schedule file \"%s\" line %d: %s\n"),
+							 MAX_PARALLEL_TESTS, schedule, line_num, scbuf);
 					}
 					sav = *c;
 					*c = '\0';
@@ -1624,14 +1733,12 @@ run_schedule(const char *schedule, test_start_function startfunc,
 
 		if (num_tests == 0)
 		{
-			fprintf(stderr, _("syntax error in schedule file \"%s\" line %d: %s\n"),
-					schedule, line_num, scbuf);
-			exit(2);
+			bail(_("syntax error in schedule file \"%s\" line %d: %s\n"),
+				 schedule, line_num, scbuf);
 		}
 
 		if (num_tests == 1)
 		{
-			status(_("test %-28s ... "), tests[0]);
 			pids[0] = (startfunc) (tests[0], &resultfiles[0], &expectfiles[0], &tags[0]);
 			INSTR_TIME_SET_CURRENT(starttimes[0]);
 			wait_for_tests(pids, statuses, stoptimes, NULL, 1);
@@ -1639,16 +1746,15 @@ run_schedule(const char *schedule, test_start_function startfunc,
 		}
 		else if (max_concurrent_tests > 0 && max_concurrent_tests < num_tests)
 		{
-			fprintf(stderr, _("too many parallel tests (more than %d) in schedule file \"%s\" line %d: %s\n"),
-					max_concurrent_tests, schedule, line_num, scbuf);
-			exit(2);
+			bail(_("too many parallel tests (more than %d) in schedule file \"%s\" line %d: %s\n"),
+				 max_concurrent_tests, schedule, line_num, scbuf);
 		}
 		else if (max_connections > 0 && max_connections < num_tests)
 		{
 			int			oldest = 0;
 
-			status(_("parallel group (%d tests, in groups of %d): "),
-				   num_tests, max_connections);
+			note(_("parallel group (%d tests, in groups of %d): "),
+				 num_tests, max_connections);
 			for (i = 0; i < num_tests; i++)
 			{
 				if (i - oldest >= max_connections)
@@ -1664,18 +1770,18 @@ run_schedule(const char *schedule, test_start_function startfunc,
 			wait_for_tests(pids + oldest, statuses + oldest,
 						   stoptimes + oldest,
 						   tests + oldest, i - oldest);
-			status_end();
+			note_end();
 		}
 		else
 		{
-			status(_("parallel group (%d tests): "), num_tests);
+			note(_("parallel group (%d tests): "), num_tests);
 			for (i = 0; i < num_tests; i++)
 			{
 				pids[i] = (startfunc) (tests[i], &resultfiles[i], &expectfiles[i], &tags[i]);
 				INSTR_TIME_SET_CURRENT(starttimes[i]);
 			}
 			wait_for_tests(pids, statuses, stoptimes, tests, num_tests);
-			status_end();
+			note_end();
 		}
 
 		/* Check results for all tests */
@@ -1686,8 +1792,7 @@ run_schedule(const char *schedule, test_start_function startfunc,
 					   *tl;
 			bool		differ = false;
 
-			if (num_tests > 1)
-				status(_("     %-28s ... "), tests[i]);
+			INSTR_TIME_SUBTRACT(stoptimes[i], starttimes[i]);
 
 			/*
 			 * Advance over all three lists simultaneously.
@@ -1707,9 +1812,7 @@ run_schedule(const char *schedule, test_start_function startfunc,
 					(*postfunc) (rl->str);
 				newdiff = results_differ(tests[i], rl->str, el->str);
 				if (newdiff && tl)
-				{
-					printf("%s ", tl->str);
-				}
+					diag(_("tag: %s\n"), tl->str);
 				differ |= newdiff;
 			}
 
@@ -1726,30 +1829,14 @@ run_schedule(const char *schedule, test_start_function startfunc,
 						break;
 					}
 				}
-				if (ignore)
-				{
-					status(_("failed (ignored)"));
-					fail_ignore_count++;
-				}
-				else
-				{
-					status(_("FAILED"));
-					fail_count++;
-				}
+
+				test_status_failed(tests[i], ignore, INSTR_TIME_GET_MILLISEC(stoptimes[i]), (num_tests > 1));
 			}
 			else
-			{
-				status(_("ok    "));	/* align with FAILED */
-				success_count++;
-			}
+				test_status_ok(tests[i], INSTR_TIME_GET_MILLISEC(stoptimes[i]), (num_tests > 1));
 
 			if (statuses[i] != 0)
 				log_child_failure(statuses[i]);
-
-			INSTR_TIME_SUBTRACT(stoptimes[i], starttimes[i]);
-			status(_(" %8.0f ms"), INSTR_TIME_GET_MILLISEC(stoptimes[i]));
-
-			status_end();
 		}
 
 		for (i = 0; i < num_tests; i++)
@@ -1786,7 +1873,6 @@ run_single_test(const char *test, test_start_function startfunc,
 			   *tl;
 	bool		differ = false;
 
-	status(_("test %-28s ... "), test);
 	pid = (startfunc) (test, &resultfiles, &expectfiles, &tags);
 	INSTR_TIME_SET_CURRENT(starttime);
 	wait_for_tests(&pid, &exit_status, &stoptime, NULL, 1);
@@ -1809,30 +1895,19 @@ run_single_test(const char *test, test_start_function startfunc,
 			(*postfunc) (rl->str);
 		newdiff = results_differ(test, rl->str, el->str);
 		if (newdiff && tl)
-		{
-			printf("%s ", tl->str);
-		}
+			diag(_("tag: %s\n"), tl->str);
 		differ |= newdiff;
 	}
 
+	INSTR_TIME_SUBTRACT(stoptime, starttime);
+
 	if (differ)
-	{
-		status(_("FAILED"));
-		fail_count++;
-	}
+		test_status_failed(test, false, INSTR_TIME_GET_MILLISEC(stoptime), false);
 	else
-	{
-		status(_("ok    "));	/* align with FAILED */
-		success_count++;
-	}
+		test_status_ok(test, INSTR_TIME_GET_MILLISEC(stoptime), false);
 
 	if (exit_status != 0)
 		log_child_failure(exit_status);
-
-	INSTR_TIME_SUBTRACT(stoptime, starttime);
-	status(_(" %8.0f ms"), INSTR_TIME_GET_MILLISEC(stoptime));
-
-	status_end();
 }
 
 /*
@@ -1854,9 +1929,8 @@ open_result_files(void)
 	logfile = fopen(logfilename, "w");
 	if (!logfile)
 	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for writing: %s\n"),
-				progname, logfilename, strerror(errno));
-		exit(2);
+		bail(_("%s: could not open file \"%s\" for writing: %s"),
+			 progname, logfilename, strerror(errno));
 	}
 
 	/* create the diffs file as empty */
@@ -1865,9 +1939,8 @@ open_result_files(void)
 	difffile = fopen(difffilename, "w");
 	if (!difffile)
 	{
-		fprintf(stderr, _("%s: could not open file \"%s\" for writing: %s\n"),
-				progname, difffilename, strerror(errno));
-		exit(2);
+		bail(_("%s: could not open file \"%s\" for writing: %s"),
+			 progname, difffilename, strerror(errno));
 	}
 	/* we don't keep the diffs file open continuously */
 	fclose(difffile);
@@ -1883,7 +1956,6 @@ drop_database_if_exists(const char *dbname)
 {
 	StringInfo	buf = psql_start_command();
 
-	header(_("dropping database \"%s\""), dbname);
 	/* Set warning level so we don't see chatter about nonexistent DB */
 	psql_add_command(buf, "SET client_min_messages = warning");
 	psql_add_command(buf, "DROP DATABASE IF EXISTS \"%s\"", dbname);
@@ -1900,7 +1972,6 @@ create_database(const char *dbname)
 	 * We use template0 so that any installation-local cruft in template1 will
 	 * not mess up the tests.
 	 */
-	header(_("creating database \"%s\""), dbname);
 	if (encoding)
 		psql_add_command(buf, "CREATE DATABASE \"%s\" TEMPLATE=template0 ENCODING='%s'%s", dbname, encoding,
 						 (nolocale) ? " LC_COLLATE='C' LC_CTYPE='C'" : "");
@@ -1922,10 +1993,7 @@ create_database(const char *dbname)
 	 * this will work whether or not the extension is preinstalled.
 	 */
 	for (sl = loadextension; sl != NULL; sl = sl->next)
-	{
-		header(_("installing %s"), sl->str);
 		psql_command(dbname, "CREATE EXTENSION IF NOT EXISTS \"%s\"", sl->str);
-	}
 }
 
 static void
@@ -1933,7 +2001,6 @@ drop_role_if_exists(const char *rolename)
 {
 	StringInfo	buf = psql_start_command();
 
-	header(_("dropping role \"%s\""), rolename);
 	/* Set warning level so we don't see chatter about nonexistent role */
 	psql_add_command(buf, "SET client_min_messages = warning");
 	psql_add_command(buf, "DROP ROLE IF EXISTS \"%s\"", rolename);
@@ -1945,7 +2012,6 @@ create_role(const char *rolename, const _stringlist *granted_dbs)
 {
 	StringInfo	buf = psql_start_command();
 
-	header(_("creating role \"%s\""), rolename);
 	psql_add_command(buf, "CREATE ROLE \"%s\" WITH LOGIN", rolename);
 	for (; granted_dbs != NULL; granted_dbs = granted_dbs->next)
 	{
@@ -2167,8 +2233,8 @@ regression_main(int argc, char *argv[],
 				break;
 			default:
 				/* getopt_long already emitted a complaint */
-				fprintf(stderr, _("\nTry \"%s -h\" for more information.\n"),
-						progname);
+				pg_log_error_hint("Try \"%s --help\" for more information.",
+								  progname);
 				exit(2);
 		}
 	}
@@ -2241,17 +2307,13 @@ regression_main(int argc, char *argv[],
 
 		if (directory_exists(temp_instance))
 		{
-			header(_("removing existing temp instance"));
 			if (!rmtree(temp_instance, true))
 			{
-				fprintf(stderr, _("\n%s: could not remove temp instance \"%s\"\n"),
-						progname, temp_instance);
-				exit(2);
+				bail(_("%s: could not remove temp instance \"%s\""),
+					 progname, temp_instance);
 			}
 		}
 
-		header(_("creating temporary instance"));
-
 		/* make the temp instance top directory */
 		make_directory(temp_instance);
 
@@ -2261,7 +2323,6 @@ regression_main(int argc, char *argv[],
 			make_directory(buf);
 
 		/* initdb */
-		header(_("initializing database system"));
 		snprintf(buf, sizeof(buf),
 				 "\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync%s%s > \"%s/log/initdb.log\" 2>&1",
 				 bindir ? bindir : "",
@@ -2273,8 +2334,8 @@ regression_main(int argc, char *argv[],
 		fflush(NULL);
 		if (system(buf))
 		{
-			fprintf(stderr, _("\n%s: initdb failed\nExamine %s/log/initdb.log for the reason.\nCommand was: %s\n"), progname, outputdir, buf);
-			exit(2);
+			bail(_("%s: initdb failed\nExamine %s/log/initdb.log for the reason.\nCommand was: %s"),
+				 progname, outputdir, buf);
 		}
 
 		/*
@@ -2289,8 +2350,8 @@ regression_main(int argc, char *argv[],
 		pg_conf = fopen(buf, "a");
 		if (pg_conf == NULL)
 		{
-			fprintf(stderr, _("\n%s: could not open \"%s\" for adding extra config: %s\n"), progname, buf, strerror(errno));
-			exit(2);
+			bail(_("%s: could not open \"%s\" for adding extra config: %s"),
+				 progname, buf, strerror(errno));
 		}
 		fputs("\n# Configuration added by pg_regress\n\n", pg_conf);
 		fputs("log_autovacuum_min_duration = 0\n", pg_conf);
@@ -2309,8 +2370,8 @@ regression_main(int argc, char *argv[],
 			extra_conf = fopen(temp_config, "r");
 			if (extra_conf == NULL)
 			{
-				fprintf(stderr, _("\n%s: could not open \"%s\" to read extra config: %s\n"), progname, temp_config, strerror(errno));
-				exit(2);
+				bail(_("%s: could not open \"%s\" to read extra config: %s"),
+					 progname, temp_config, strerror(errno));
 			}
 			while (fgets(line_buf, sizeof(line_buf), extra_conf) != NULL)
 				fputs(line_buf, pg_conf);
@@ -2349,14 +2410,14 @@ regression_main(int argc, char *argv[],
 
 				if (port_specified_by_user || i == 15)
 				{
-					fprintf(stderr, _("port %d apparently in use\n"), port);
+					note(_("port %d apparently in use\n"), port);
 					if (!port_specified_by_user)
-						fprintf(stderr, _("%s: could not determine an available port\n"), progname);
-					fprintf(stderr, _("Specify an unused port using the --port option or shut down any conflicting PostgreSQL servers.\n"));
-					exit(2);
+						note(_("could not determine an available port\n"));
+					bail(_("Specify an unused port using the --port option or shut down any conflicting PostgreSQL servers."));
 				}
 
-				fprintf(stderr, _("port %d apparently in use, trying %d\n"), port, port + 1);
+				note(_("port %d apparently in use, trying %d\n"),
+					 port, port + 1);
 				port++;
 				sprintf(s, "%d", port);
 				setenv("PGPORT", s, 1);
@@ -2368,7 +2429,6 @@ regression_main(int argc, char *argv[],
 		/*
 		 * Start the temp postmaster
 		 */
-		header(_("starting postmaster"));
 		snprintf(buf, sizeof(buf),
 				 "\"%s%spostgres\" -D \"%s/data\" -F%s "
 				 "-c \"listen_addresses=%s\" -k \"%s\" "
@@ -2380,11 +2440,7 @@ regression_main(int argc, char *argv[],
 				 outputdir);
 		postmaster_pid = spawn_process(buf);
 		if (postmaster_pid == INVALID_PID)
-		{
-			fprintf(stderr, _("\n%s: could not spawn postmaster: %s\n"),
-					progname, strerror(errno));
-			exit(2);
-		}
+			bail(_("could not spawn postmaster: %s\n"), strerror(errno));
 
 		/*
 		 * Wait till postmaster is able to accept connections; normally this
@@ -2419,16 +2475,16 @@ regression_main(int argc, char *argv[],
 			if (WaitForSingleObject(postmaster_pid, 0) == WAIT_OBJECT_0)
 #endif
 			{
-				fprintf(stderr, _("\n%s: postmaster failed\nExamine %s/log/postmaster.log for the reason\n"), progname, outputdir);
-				exit(2);
+				bail(_("postmaster failed, examine %s/log/postmaster.log for the reason\n"),
+					 outputdir);
 			}
 
 			pg_usleep(1000000L);
 		}
 		if (i >= wait_seconds)
 		{
-			fprintf(stderr, _("\n%s: postmaster did not respond within %d seconds\nExamine %s/log/postmaster.log for the reason\n"),
-					progname, wait_seconds, outputdir);
+			diag(_("postmaster did not respond within %d seconds, examine %s/log/postmaster.log for the reason\n"),
+				 wait_seconds, outputdir);
 
 			/*
 			 * If we get here, the postmaster is probably wedged somewhere in
@@ -2437,17 +2493,14 @@ regression_main(int argc, char *argv[],
 			 * attempts.
 			 */
 #ifndef WIN32
-			if (kill(postmaster_pid, SIGKILL) != 0 &&
-				errno != ESRCH)
-				fprintf(stderr, _("\n%s: could not kill failed postmaster: %s\n"),
-						progname, strerror(errno));
+			if (kill(postmaster_pid, SIGKILL) != 0 && errno != ESRCH)
+				bail(_("could not kill failed postmaster: %s"), strerror(errno));
 #else
 			if (TerminateProcess(postmaster_pid, 255) == 0)
-				fprintf(stderr, _("\n%s: could not kill failed postmaster: error code %lu\n"),
-						progname, GetLastError());
+				bail(_("could not kill failed postmaster: error code %lu"),
+					 GetLastError());
 #endif
-
-			exit(2);
+			bail(_("postmaster failed"));
 		}
 
 		postmaster_running = true;
@@ -2458,8 +2511,8 @@ regression_main(int argc, char *argv[],
 #else
 #define ULONGPID(x) (unsigned long) (x)
 #endif
-		printf(_("running on port %d with PID %lu\n"),
-			   port, ULONGPID(postmaster_pid));
+		note(_("running on port %d with PID %lu\n"),
+			 port, ULONGPID(postmaster_pid));
 	}
 	else
 	{
@@ -2490,8 +2543,6 @@ regression_main(int argc, char *argv[],
 	/*
 	 * Ready to run the tests
 	 */
-	header(_("running regression test queries"));
-
 	for (sl = schedulelist; sl != NULL; sl = sl->next)
 	{
 		run_schedule(sl->str, startfunc, postfunc);
@@ -2507,7 +2558,6 @@ regression_main(int argc, char *argv[],
 	 */
 	if (temp_instance)
 	{
-		header(_("shutting down postmaster"));
 		stop_postmaster();
 	}
 
@@ -2518,62 +2568,63 @@ regression_main(int argc, char *argv[],
 	 */
 	if (temp_instance && fail_count == 0 && fail_ignore_count == 0)
 	{
-		header(_("removing temporary instance"));
 		if (!rmtree(temp_instance, true))
-			fprintf(stderr, _("\n%s: could not remove temp instance \"%s\"\n"),
-					progname, temp_instance);
+			diag("could not remove temp instance \"%s\"\n",
+				 temp_instance);
 	}
 
-	fclose(logfile);
+	/*
+	 * Emit a TAP compliant Plan
+	 */
+	plan((fail_count + fail_ignore_count + success_count));
 
 	/*
 	 * Emit nice-looking summary message
 	 */
 	if (fail_count == 0 && fail_ignore_count == 0)
-		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 */
 	else
-		/* fail_count>0 && fail_ignore_count>0 */
-		snprintf(buf, sizeof(buf),
-				 _(" %d of %d tests failed, %d of these failures ignored. "),
-				 fail_count + fail_ignore_count,
-				 success_count + fail_count + fail_ignore_count,
-				 fail_ignore_count);
-
-	putchar('\n');
-	for (i = strlen(buf); i > 0; i--)
-		putchar('=');
-	printf("\n%s\n", buf);
-	for (i = strlen(buf); i > 0; i--)
-		putchar('=');
-	putchar('\n');
-	putchar('\n');
+		diag(_("%d of %d tests failed, %d of these failures ignored.\n"),
+			 fail_count + fail_ignore_count,
+			 success_count + fail_count + fail_ignore_count,
+			 fail_ignore_count);
+
+	if (fail_count > 0 || fail_ignore_count > 0)
+		diag(_("Failed and ignored tests:%s\n"), failed_tests->data);
 
 	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"),
-			   difffilename, logfilename);
+		diag(_("The differences that caused some tests to fail can be viewed in the file \"%s\".\n"),
+			 difffilename);
+		diag(_("A copy of the test summary that you see above is saved in the file \"%s\".\n"),
+			 logfilename);
 	}
 	else
 	{
 		unlink(difffilename);
 		unlink(logfilename);
+
+		free(difffilename);
+		difffilename = NULL;
+		free(logfilename);
+		logfilename = NULL;
 	}
 
+	fclose(logfile);
+	logfile = NULL;
+
 	if (fail_count != 0)
 		exit(1);
 
-- 
2.34.1

---

From: vignesh C <[email-masked]>
Date: Thu, 19 Jan 2023 16:44:56 +0530

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== applying patch
./v15-0001-Change-pg_regress-output-format-to-be-TAP-compli.patch
patching file meson.build
patching file src/test/regress/pg_regress.c
...
Hunk #58 FAILED at 2584.
2 out of 58 hunks FAILED -- saving rejects to file
src/test/regress/pg_regress.c.rej

Regards,
Vignesh

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 23 Jan 2023 12:42:57 +0100

> On 19 Jan 2023, at 12:14, vignesh C <[email-masked]> wrote:

> The patch does not apply on top of HEAD as in [1], please post a rebased patch:

The attached v16 is a rebase on top of current master which resolves the
conflict which came from the recent commit removing the "ignore" functionality.
It also fixes a few small things pointed out off-list.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 23 Jan 2023 12:43:41 +0100

> On 23 Jan 2023, at 12:42, Daniel Gustafsson <[email-masked]> wrote:
> 

And now with the missing attachment..

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 24 Feb 2023 10:49:13 +0100

Another rebase on top of 337903a16f.  Unless there are conflicting reviews, I
consider this patch to be done and ready for going in during the next CF.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 15 Mar 2023 11:36:02 +0100

> consider this patch to be done and ready for going in during the next CF.

I think this is just about as good as it's going to get, so I think we 
can consider committing this soon.

A few comments along the way:

1) We can remove the gettext markers _() inside calls like note(), 
bail(), etc.  If we really wanted to do translation, we would do that 
inside those functions (similar to errmsg() etc.).

2) There are a few fprintf(stderr, ...) calls left.  Should those be 
changed to something TAP-enabled?

3) Maybe these lines

+++ isolation check in src/test/isolation +++

should be changed to TAP format?  Arguably, if I run "make -s check", 
then everything printed should be valid TAP, right?

4) From the introduction lines

============== creating temporary instance            ==============
============== initializing database system           ==============
============== starting postmaster                    ==============
running on port 61696 with PID 85346
============== creating database "regression"         ==============
============== running regression test queries        ==============

you have kept

# running on port 61696 with PID 85346

which, well, is that the most interesting of those?

The first three lines (creating, initializing, starting) take some 
noticeable amount of time, so they could be kept as a progress 
indicator.  Or just delete all of them?  I suppose some explicit 
indication about temp-instance versus existing instance would be useful.

5) About the output format.  Obviously, this will require some 
retraining of the eye.  But my first impression is that there is a lot 
of whitespace without any guiding lines, so to speak, horizontally or 
vertically.  It makes me a bit dizzy. ;-)

I think instead

# parallel group (2 tests):  event_trigger oidjoins
ok 210              event_trigger                      131 ms
ok 211              oidjoins                           190 ms
ok 212       fast_default                              158 ms
ok 213       tablespace                                319 ms

Something like this would be less dizzy-making:

# parallel group (2 tests):  event_trigger oidjoins
ok 210 - + event_trigger                               131 ms
ok 211 - + oidjoins                                    190 ms
ok 212 - fast_default                                  158 ms
ok 213 - tablespace                                    319 ms

Just an idea, we don't have to get this exactly perfect right now, but 
it's something to think about.

I think if 1-4 are addressed, this can be committed.

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 28 Mar 2023 15:26:06 +0200

> On 15 Mar 2023, at 11:36, Peter Eisentraut <[email-masked]> wrote:
> 
> I think this is just about as good as it's going to get, so I think we can consider committing this soon.
> 
> 1) We can remove the gettext markers _() inside calls like note(), bail(), etc.  If we really wanted to do translation, we would do that inside those functions (similar to errmsg() etc.).

Fixed.

The attached also removes all explicit \n from output and leaves the decision
on when to add a linebreak to the TAP emitting function.  I think this better
match how we typically handle printing of output like this.  It also ensures
that all bail messages follow the same syntax.

> 2) There are a few fprintf(stderr, ...) calls left.  Should those be changed to something TAP-enabled?

Initially the patch kept errors happening before testing started a non-TAP
output, there were leftovers which are now converted.

> should be changed to TAP format?  Arguably, if I run "make -s check", then everything printed should be valid TAP, right?

Fixed.

> The first three lines (creating, initializing, starting) take some noticeable amount of time, so they could be kept as a progress indicator.  Or just delete all of them?  I suppose some explicit indication about temp-instance versus existing instance would be useful.

This was discussed in [email-masked] which
concluded that this was about the only thing of interest, and even at that it
was more of a maybe than a definite yes. 

As this patch stands, it prints the above for a temp install and the host/port
for an existing install, but I don't have ny problems removing that as well.

> 5) About the output format.  Obviously, this will require some retraining of the eye.  But my first impression is that there is a lot of whitespace without any guiding lines, so to speak, horizontally or vertically.  It makes me a bit dizzy. ;-)
> 
> I think instead
> 

The current format is chosen to be close to the old format, while also adding
sufficient padding that it won't yield ragged columns.  The wide padding is
needed to cope with long names in the isolation and ecgp test suites and not so
much regress suite.

In the attached I've dialled back the padding a little bit to make it a bit
more compact, but I doubt it's enough.

> Just an idea, we don't have to get this exactly perfect right now, but it's something to think about.

I'm quite convinced that this will be revisited once this lands and is in front
of developers.

I think the attached is a good candidate for going in, but I would like to see it
for another spin in the CF bot first.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 28 Mar 2023 15:56:56 +0200

> On 28 Mar 2023, at 15:26, Daniel Gustafsson <[email-masked]> wrote:

> for another spin in the CF bot first.

Another candidate due to a thinko which raised a compiler warning.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 29 Mar 2023 09:08:05 +0200

> Another candidate due to a thinko which raised a compiler warning.

This is incorrect:

-echo "+++ tap install-check in $(subdir) +++" && \
+echo "\# +++ tap install-check in $(subdir) +++" && \

It actually prints the backslash.

But this appears to be correct:

  pg_regress_check = \
-    echo "+++ regress check in $(subdir) +++" && \
+    echo "\# +++ regress check in $(subdir) +++" && \

(Maybe because it's in a variable definition?)

I'm confused why all the messages at the end lost their period ("All 
tests passed", "A copy of the test summary ...", etc.).  As long as they 
are written as sentences, they should have a period.

There is a comment "Testnames are indented 8 spaces" but you made that 
into a macro now, which is currently defined to 3.

There is an unexplained #if 0.

The additions of

+       free(difffilename);
+       difffilename = NULL;
+       free(logfilename);
+       logfilename = NULL;

in the success case are not clear.  The program is going to end soon anyway.

On the indentation of the test names:  If you look at the output of 
meson test verbose mode (e.g., meson test -C _build --suite regress 
--verbose), it reproduces the indentation in a weirdly backwards way, e.g.,

▶ 1/1 stats                                  981 ms OK
▶ 1/1 event_trigger                          122 ms OK
▶ 1/1 oidjoins                               172 ms OK
▶ 1/1 fast_default                              137 ms OK
▶ 1/1 tablespace                                285 ms OK

Not sure by which logic it arrives at that, but you can clearly see 3 
additional spaces for the single tests.

One thing I have noticed while playing around with this is that it's 
quite hard to tell casually whether a test run has failed or is failing, 
if you just keep half an eye on it in another Window.  The display of 
"ok"/"not ok" as well as the summaries at the end are much less 
prominent than the previous "ok"/"FAILED" and the summary box at the 
end.  I'm not sure what to do about that, or if it's just something to 
get used to.

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 29 Mar 2023 21:14:23 +0200

> On 29 Mar 2023, at 09:08, Peter Eisentraut <[email-masked]> wrote:
> 

Copy paste error, fixed.

> I'm confused why all the messages at the end lost their period ("All tests passed", "A copy of the test summary ...", etc.).  As long as they are written as sentences, they should have a period.

Fixed.  I also made sure that all messages printing the output directory wrap
it in quotes like how we print paths generally.

> There is a comment "Testnames are indented 8 spaces" but you made that into a macro now, which is currently defined to 3.

As explained below, this is removed.  I've also removed a comment on NLS which
no longer makes sense.

> There is an unexplained #if 0.

Ugh, I clearly should've stayed on the couch yesterday.

> in the success case are not clear.  The program is going to end soon anyway.

Fair enough, removed.

> On the indentation of the test names:  If you look at the output of meson test verbose mode (e.g., meson test -C _build --suite regress --verbose), it reproduces the indentation in a weirdly backwards way, e.g.,
> 
> ▶ 1/1 stats                                  981 ms OK
> Not sure by which logic it arrives at that, but you can clearly see 3 additional spaces for the single tests.

I'm not sure what meson does actually.  It seems to strip the leading padding
and line up the testname, but then add the stripped padding on the right side?
Removing the padding for parallel tests solves it, so I have in the attached
moved to indicating parallel tests with a leading '+' and single tests with
'-'.  Not sure if this is clear enough, but it's not worse than padding IMO.

> One thing I have noticed while playing around with this is that it's quite hard to tell casually whether a test run has failed or is failing, if you just keep half an eye on it in another Window.  The display of "ok"/"not ok" as well as the summaries at the end are much less prominent than the previous "ok"/"FAILED" and the summary box at the end.  I'm not sure what to do about that, or if it's just something to get used to.

meson already presents the results in a box, so if we bring back the === box it
gets quite verbose there.  To some extent I think it is something to get used
to, but if there is discontent with what it looks like reported when more
hackers gets exposed to this we need to fix it.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 29 Mar 2023 22:08:48 +0200

> On 29 Mar 2023, at 21:14, Daniel Gustafsson <[email-masked]> wrote:

> Ugh, I clearly should've stayed on the couch yesterday.

Maybe today as well, just as I had sent this I realized there is mention of the
output format in the docs that I had missed.  The attached changes that as well
to match the new format.  (The section in question doesn't mention using meson,
but I have a feeling that's tackled in one of the meson docs threads.)

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 31 Mar 2023 13:12:05 +0200

> On 29 Mar 2023, at 22:08, Daniel Gustafsson <[email-masked]> wrote:
> 

Pushed to master now with a few tweaks to comments and docs.  Thanks for all
the reviews, I hope the new format will make meson builds easier/better without
compromising the human readable aspect of traditional builds.

--
Daniel Gustafsson