Thread: TAP testing for psql's tab completion code

We've often talked about the problem that we have no regression test
coverage for psql's tab completion code.  I got interested in this
issue while messing with the filename completion logic therein [1],
so here is a draft patch that adds some testing for that code.

This is just preliminary investigation, so I've made no attempt
to test tab-complete.c comprehensively (and I'm not sure there
would be much point in covering every last code path in it anyway).
Still, it does get us from zero to 43% coverage of that file
already, and it does good things for the coverage of input.c
and prompt.c as well.

What I think is actually interesting at this stage is portability
of the tests.  There are a number of issues:

* The script requires Perl's IO::Pty module (indirectly, in that IPC::Run
requires that to make pty connections), which isn't installed everywhere.
I just made the script skip if that's not available, so that we're not
moving the goalposts for what has to be installed to run the TAP tests.
Is that the right answer?

* It seems pretty likely that this won't work on Windows, given all the
caveats in the IPC::Run documentation about nonfunctionality of the pty
support there.  Maybe we don't care, seeing that we don't really support
readline on Windows anyway.  For the moment I assumed that the skip
conditions for --without-readline and/or missing-IO::Pty would cover
this, but we might need an explicit check for Windows too.  Or maybe
somebody wants to try to make it work on Windows; but that won't be me.

* What's even more likely to be problematic is small variations in the
output behavior of different readline and libedit versions.  According
to my tests so far, though, all modern versions of them do pass these
test cases.  I noted failures on very old Apple versions of libedit:

1. macOS 10.5's version of libedit seems not to honor
rl_completion_append_character; it never emits the trailing space
we're expecting.  This seems like a plain old libedit bug, especially
since 10.4's version works as expected.

2. Both 10.4 and 10.5 emit the alternative table names in the wrong
order, suggesting that they're not internally sorting the completion
results.  If this proves to be more widespread, we could likely fix
it by adding ORDER BY to the completion queries, but I'm not sure that
it's worth doing if only these dead macOS versions have the issue.
(On the other hand, it seems like bad practice to be issuing queries
that have LIMIT without ORDER BY, so maybe we should fix them anyway.)


I'm strongly tempted to just push this and see what the buildfarm
thinks of it.  If it fails in lots of places, maybe the idea is a
dead end.  If it works, I'd look into extending the tests --- in
particular, I'd like to have some coverage for the filename
completion logic.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/14585.1577486216%40sss.pgh.pa.us

diff --git a/configure b/configure
index 9de5037..a133f66 100755
--- a/configure
+++ b/configure
@@ -706,6 +706,7 @@ with_libxml
 XML2_CONFIG
 UUID_EXTRA_OBJS
 with_uuid
+with_readline
 with_systemd
 with_selinux
 with_openssl
@@ -8000,6 +8001,7 @@ $as_echo "$as_me: WARNING: *** Readline does not work on MinGW --- disabling" >&
 fi


+
 #
 # Prefer libedit
 #
diff --git a/configure.in b/configure.in
index 9c5e5e7..9172622 100644
--- a/configure.in
+++ b/configure.in
@@ -875,6 +875,7 @@ if test "$PORTNAME" = "win32"; then
     with_readline=no
   fi
 fi
+AC_SUBST(with_readline)


 #
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 05b6638..5002c47 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -185,6 +185,7 @@ with_perl    = @with_perl@
 with_python    = @with_python@
 with_tcl    = @with_tcl@
 with_openssl    = @with_openssl@
+with_readline    = @with_readline@
 with_selinux    = @with_selinux@
 with_systemd    = @with_systemd@
 with_gssapi    = @with_gssapi@
diff --git a/src/bin/psql/.gitignore b/src/bin/psql/.gitignore
index c2862b1..10b6dd3 100644
--- a/src/bin/psql/.gitignore
+++ b/src/bin/psql/.gitignore
@@ -1,5 +1,5 @@
 /psqlscanslash.c
 /sql_help.h
 /sql_help.c
-
 /psql
+/tmp_check/
diff --git a/src/bin/psql/Makefile b/src/bin/psql/Makefile
index 42771bc..d08767b 100644
--- a/src/bin/psql/Makefile
+++ b/src/bin/psql/Makefile
@@ -16,6 +16,9 @@ subdir = src/bin/psql
 top_builddir = ../../..
 include $(top_builddir)/src/Makefile.global

+# make this available to TAP test scripts
+export with_readline
+
 REFDOCDIR= $(top_srcdir)/doc/src/sgml/ref

 override CPPFLAGS := -I. -I$(srcdir) -I$(libpq_srcdir) $(CPPFLAGS)
@@ -73,8 +76,15 @@ uninstall:

 clean distclean:
     rm -f psql$(X) $(OBJS) lex.backup
+    rm -rf tmp_check

 # files removed here are supposed to be in the distribution tarball,
 # so do not clean them in the clean/distclean rules
 maintainer-clean: distclean
     rm -f sql_help.h sql_help.c psqlscanslash.c
+
+check:
+    $(prove_check)
+
+installcheck:
+    $(prove_installcheck)
diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
new file mode 100644
index 0000000..1c7610f
--- /dev/null
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -0,0 +1,122 @@
+use strict;
+use warnings;
+
+use PostgresNode;
+use TestLib;
+use Test::More;
+use IPC::Run qw(pump finish timer);
+
+if ($ENV{with_readline} ne 'yes')
+{
+    plan skip_all => 'readline is not supported by this build';
+}
+
+# If we don't have IO::Pty, forget it, because IPC::Run depends on that
+# to support pty connections
+eval { require IO::Pty; };
+if ($@)
+{
+    plan skip_all => 'IO::Pty is needed to run this test';
+}
+
+# start a new server
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+# set up a few database objects
+$node->safe_psql('postgres',
+        "CREATE TABLE tab1 (f1 int, f2 text);\n"
+      . "CREATE TABLE mytab123 (f1 int, f2 text);\n"
+      . "CREATE TABLE mytab246 (f1 int, f2 text);\n");
+
+# Developers would not appreciate this test adding a bunch of junk to
+# their ~/.psql_history, so be sure to redirect history into a temp file.
+# We might as well put it in the test log directory, so that buildfarm runs
+# capture the result for possible debugging purposes.
+my $historyfile = "${TestLib::log_path}/010_psql_history.txt";
+$ENV{PSQL_HISTORY} = $historyfile;
+
+# fire up an interactive psql session
+my $in  = '';
+my $out = '';
+
+my $timer = timer(5);
+
+my $h = $node->interactive_psql('postgres', \$in, \$out, $timer);
+
+ok($out =~ /psql/, "print startup banner");
+
+# Simple test case: type something and see if psql responds as expected
+sub check_completion
+{
+    my ($send, $pattern, $annotation) = @_;
+
+    # reset output collector
+    $out = "";
+    # restart per-command timer
+    $timer->start(5);
+    # send the data to be sent
+    $in .= $send;
+    # wait ...
+    pump $h until ($out =~ m/$pattern/ || $timer->is_expired);
+    my $okay = ($out =~ m/$pattern/ && !$timer->is_expired);
+    ok($okay, $annotation);
+    # for debugging, log actual output if it didn't match
+    note 'Actual output was "' . $out . "\"\n" if !$okay;
+}
+
+# Clear query buffer to start over
+# (won't work if we are inside a string literal!)
+sub clear_query
+{
+    check_completion("\\r\n", "postgres=# ", "\\r works");
+}
+
+# check basic command completion: SEL<tab> produces SELECT<space>
+check_completion("SEL\t", "SELECT ", "complete SEL<tab> to SELECT");
+
+clear_query();
+
+# check case variation is honored
+check_completion("sel\t", "select ", "complete sel<tab> to select");
+
+# check basic table name completion
+check_completion("* from t\t", "\\* from tab1 ", "complete t<tab> to tab1");
+
+clear_query();
+
+# check table name completion with multiple alternatives
+# note: readline might print a bell before the completion
+check_completion(
+    "select * from my\t",
+    "select \\* from my\a?tab",
+    "complete my<tab> to mytab when there are multiple choices");
+
+# some versions of readline/libedit require two tabs here, some only need one
+check_completion("\t\t", "mytab123 +mytab246",
+    "offer multiple table choices");
+
+check_completion("2\t", "246 ",
+    "finish completion of one of multiple table choices");
+
+clear_query();
+
+# check case-sensitive keyword replacement
+# XXX the output here might vary across readline versions
+check_completion(
+    "\\DRD\t",
+    "\\DRD\b\b\bdrds ",
+    "complete \\DRD<tab> to \\drds");
+
+clear_query();
+
+# send psql an explicit \q to shut it down, else pty won't close properly
+$timer->start(5);
+$in .= "\\q\n";
+finish $h or die "psql returned $?";
+$timer->reset;
+
+# done
+$node->stop;
+done_testing();
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 270bd6c..2e0cf4a 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1534,6 +1534,73 @@ sub psql

 =pod

+=item $node->interactive_psql($dbname, \$stdin, \$stdout, $timer, %params) => harness
+
+Invoke B<psql> on B<$dbname> and return an IPC::Run harness object,
+which the caller may use to send interactive input to B<psql>.
+The process's stdin is sourced from the $stdin scalar reference,
+and its stdout and stderr go to the $stdout scalar reference.
+ptys are used so that psql thinks it's being called interactively.
+
+The specified timer object is attached to the harness, as well.
+It's caller's responsibility to select the timeout length, and to
+restart the timer after each command if the timeout is per-command.
+
+psql is invoked in tuples-only unaligned mode with reading of B<.psqlrc>
+disabled.  That may be overridden by passing extra psql parameters.
+
+Dies on failure to invoke psql, or if psql fails to connect.
+Errors occurring later are the caller's problem.
+
+Be sure to "finish" the harness when done with it.
+
+The only extra parameter currently accepted is
+
+=over
+
+=item extra_params => ['--single-transaction']
+
+If given, it must be an array reference containing additional parameters to B<psql>.
+
+=back
+
+This requires IO::Pty in addition to IPC::Run.
+
+=cut
+
+sub interactive_psql
+{
+    my ($self, $dbname, $stdin, $stdout, $timer, %params) = @_;
+
+    my @psql_params = ('psql', '-XAt', '-d', $self->connstr($dbname));
+
+    push @psql_params, @{ $params{extra_params} }
+      if defined $params{extra_params};
+
+    # Ensure there is no data waiting to be sent:
+    $$stdin = "" if ref($stdin);
+    # IPC::Run would otherwise append to existing contents:
+    $$stdout = "" if ref($stdout);
+
+    my $harness = IPC::Run::start \@psql_params,
+      '<pty<', $stdin, '>pty>', $stdout, $timer;
+
+    # Pump until we see psql's help banner.  This ensures that callers
+    # won't write anything to the pty before it's ready, avoiding an
+    # implementation issue in IPC::Run.  Also, it means that psql
+    # connection failures are caught here, relieving callers of
+    # the need to handle those.  (Right now, we have no particularly
+    # good handling for errors anyway, but that might be added later.)
+    pump $harness
+      until $$stdout =~ /Type "help" for help/ || $timer->is_expired;
+
+    die "psql startup timed out" if $timer->is_expired;
+
+    return $harness;
+}
+
+=pod
+
 =item $node->poll_query_until($dbname, $query [, $expected ])

 Run B<$query> repeatedly, until it returns the B<$expected> result

Hello Tom,

> We've often talked about the problem that we have no regression test
> coverage for psql's tab completion code.  I got interested in this
> issue while messing with the filename completion logic therein [1],
> so here is a draft patch that adds some testing for that code.
>
> This is just preliminary investigation, so I've made no attempt
> to test tab-complete.c comprehensively (and I'm not sure there
> would be much point in covering every last code path in it anyway).
> Still, it does get us from zero to 43% coverage of that file
> already, and it does good things for the coverage of input.c
> and prompt.c as well.
>
> What I think is actually interesting at this stage is portability
> of the tests.  There are a number of issues:
>
> * The script requires Perl's IO::Pty module (indirectly, in that IPC::Run
> requires that to make pty connections), which isn't installed everywhere.
> I just made the script skip if that's not available, so that we're not
> moving the goalposts for what has to be installed to run the TAP tests.
> Is that the right answer?
>
> * It seems pretty likely that this won't work on Windows, given all the
> caveats in the IPC::Run documentation about nonfunctionality of the pty
> support there.  Maybe we don't care, seeing that we don't really support
> readline on Windows anyway.  For the moment I assumed that the skip
> conditions for --without-readline and/or missing-IO::Pty would cover
> this, but we might need an explicit check for Windows too.  Or maybe
> somebody wants to try to make it work on Windows; but that won't be me.
>
> * What's even more likely to be problematic is small variations in the
> output behavior of different readline and libedit versions.  According
> to my tests so far, though, all modern versions of them do pass these
> test cases.  I noted failures on very old Apple versions of libedit:
>
> 1. macOS 10.5's version of libedit seems not to honor
> rl_completion_append_character; it never emits the trailing space
> we're expecting.  This seems like a plain old libedit bug, especially
> since 10.4's version works as expected.
>
> 2. Both 10.4 and 10.5 emit the alternative table names in the wrong
> order, suggesting that they're not internally sorting the completion
> results.  If this proves to be more widespread, we could likely fix
> it by adding ORDER BY to the completion queries, but I'm not sure that
> it's worth doing if only these dead macOS versions have the issue.
> (On the other hand, it seems like bad practice to be issuing queries
> that have LIMIT without ORDER BY, so maybe we should fix them anyway.)
>
>
> I'm strongly tempted to just push this and see what the buildfarm
> thinks of it.  If it fails in lots of places, maybe the idea is a
> dead end.  If it works, I'd look into extending the tests --- in
> particular, I'd like to have some coverage for the filename
> completion logic.
>
> Thoughts?

After you raised the issue, I submitted something last August, which did 
not attract much attention.

   https://commitfest.postgresql.org/26/2262/

It covers some tab-completion stuff. It uses Expect for the interactive 
stuff (tab completion, \h, ...).

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> We've often talked about the problem that we have no regression test
>> coverage for psql's tab completion code.  I got interested in this
>> issue while messing with the filename completion logic therein [1],
>> so here is a draft patch that adds some testing for that code.

> After you raised the issue, I submitted something last August, which did 
> not attract much attention.
>    https://commitfest.postgresql.org/26/2262/
> It covers some tab-completion stuff. It uses Expect for the interactive 
> stuff (tab completion, \h, ...).

Now that you mention it, I seem to recall looking at that and not being
happy with the additional dependency on Expect.  Expect is *not* a
standard module; on the machines I have handy, the only one in which it
appears in the default Perl installation is macOS.  (Huh, what's Apple
doing out ahead of the pack?)  I'm pretty sure that Expect also relies on
IO::Pty, so it's a strictly worse dependency than what I've got here.

Can we recast what you did into something like this patch's methods?

            regards, tom lane

Hello Tom,

>>> We've often talked about the problem that we have no regression test
>>> coverage for psql's tab completion code.  I got interested in this
>>> issue while messing with the filename completion logic therein [1],
>>> so here is a draft patch that adds some testing for that code.
>
>> After you raised the issue, I submitted something last August, which did
>> not attract much attention.
>>    https://commitfest.postgresql.org/26/2262/
>> It covers some tab-completion stuff. It uses Expect for the interactive
>> stuff (tab completion, \h, ...).
>
> Now that you mention it, I seem to recall looking at that and not being
> happy with the additional dependency on Expect.

Possibly. You did not say it out very loud.

> Expect is *not* a standard module;

Somehow. It is an old one, though.

> on the machines I have handy, the only one in which it appears in the 
> default Perl installation is macOS.  (Huh, what's Apple doing out ahead 
> of the pack?)  I'm pretty sure that Expect also relies on IO::Pty,

Indeed, it does.

> so it's a strictly worse dependency than what I've got here.

If you have to install IO::Pty anyway, ISTM you can also install Expect.

IO::Pty documentation says that it is "mainly used by Expect", which is a 
clue that IO::Pty is not much better than Expect as a dependency.

For installation, "apt install libexpect-perl" did the trick for me. "cpan 
install Expect" should work as well on most setup.

I guess it is possible to check whether Expect is available and to skip 
the corresponding tests if not.

> Can we recast what you did into something like this patch's methods?

Basically it means reimplementing some expect functionality in the script, 
including new bugs. Modules were invented to avert that, so I cannot say 
I'm happy with the prospect of re-inventing the wheel. Note that Expect is 
a pure-perl 1600-LOC module.

Anyway, I'll have a look. At least I used a very limited subset of Expect 
capabilities which should help matters along.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> on the machines I have handy, the only one in which [Expect] appears in the
>> default Perl installation is macOS.  (Huh, what's Apple doing out ahead
>> of the pack?)  I'm pretty sure that Expect also relies on IO::Pty,

> Indeed, it does.

>> so it's a strictly worse dependency than what I've got here.

> If you have to install IO::Pty anyway, ISTM you can also install Expect.

My point is precisely that buildfarm owners *won't* have to install
IO::Pty; it comes in a default Perl install almost everywhere.
I'm afraid that's not true of Expect.

Now in both cases we could avoid raising the bar by allowing the
script to "skip" if the module isn't there.  But I think we'd end up
with less coverage if we do that with Expect than with IO::Pty.

> IO::Pty documentation says that it is "mainly used by Expect", which is a
> clue that IO::Pty is not much better than Expect as a dependency.

You're just guessing, not looking at facts on the ground.  I have looked
at RHEL, Fedora, Debian, FreeBSD, NetBSD, and OpenBSD.  The only one
in which IO::Pty isn't in the standard Perl install is OpenBSD.

Well, actually, it's possible that on some of these boxes it was pulled
in by the IPC::Run package, as I have that installed on all of them.
But the point remains the same: almost nowhere will IO::Pty be a new
dependency for a buildfarm owner, whereas Expect will be a new dependency
almost everywhere.

(One reason I'm interested to push this sooner not later is to find
out what fraction of the TAP-test-running buildfarm critters do have
IO::Pty already.  If it turns out not to be almost all of them, then
my assumptions are wrong and we could revisit this discussion.)

> For installation, "apt install libexpect-perl" did the trick for me. "cpan
> install Expect" should work as well on most setup.

I'm well aware of the mechanisms for installing nonstandard Perl modules,
thanks.  It's a pain, as a general rule.  The fact that the buildfarm
requires IPC::Run is a large barrier to entry, and I don't want to double
the pain by adding a second far-off-the-beaten-track dependency.

            regards, tom lane

Hello Tom,

>> If you have to install IO::Pty anyway, ISTM you can also install Expect.
>
> My point is precisely that buildfarm owners *won't* have to install
> IO::Pty; it comes in a default Perl install almost everywhere.
> I'm afraid that's not true of Expect.

Hmmm. That is a good argument.

> Now in both cases we could avoid raising the bar by allowing the
> script to "skip" if the module isn't there.

Yep.

>> IO::Pty documentation says that it is "mainly used by Expect", which is a
>> clue that IO::Pty is not much better than Expect as a dependency.
>
> You're just guessing, not looking at facts on the ground. [...]

I'm not guessing what the documentation says:-) But for the consequences, 
indeed I was guessing.

> Well, actually, it's possible that on some of these boxes it was pulled 
> in by the IPC::Run package,

Ah, you are guessing right, IPC::Run requires IO::Pty, so it should be 
available everywhere the buildfarm scripts already run. Maybe.

I've looked at your PoC implementation:

I'm not fan of relying on the configure stuff ("with_readline"), in my 
Expect version I tested if history capabilities are available from psql 
itself.

I did not paid attention not to overwrite the psql history file, though.

For the psql coverage patch, I was more ambitious and needed less 
assumption about the configuration, I only forced -X.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> I've looked at your PoC implementation:

> I'm not fan of relying on the configure stuff ("with_readline"), in my 
> Expect version I tested if history capabilities are available from psql 
> itself.

No, I disagree with that.  If configure thinks it built with readline,
and then the actual binary acts like it doesn't have readline, that's
a bug that we'd like the tests to detect.  We don't want the test
silently deciding that things are OK if the first thing it tries
doesn't work.  (For comparison, the SSL tests are also enabled by
configure's opinion not some other way -- I was mostly copying how
that works.)

> For the psql coverage patch, I was more ambitious and needed less 
> assumption about the configuration, I only forced -X.

I mainly just duplicated the environment set up by PostgresNode::psql
as much as it seemed reasonable to.  The -At options are kind of
irrelevant for what we're going to test here, probably, but why not
keep the default behavior the same?  I did drop -q since that
suppresses prompting, and we probably want to test prompt.c using
this infrastructure.

            regards, tom lane

>> I'm not fan of relying on the configure stuff ("with_readline"), in my
>> Expect version I tested if history capabilities are available from psql
>> itself.
>
> No, I disagree with that.  If configure thinks it built with readline,
> and then the actual binary acts like it doesn't have readline, that's
> a bug that we'd like the tests to detect.

Hmmm. Sure, that's a point.

What about running some tests on an installed version?

>> For the psql coverage patch, I was more ambitious and needed less
>> assumption about the configuration, I only forced -X.
>
> I mainly just duplicated the environment set up by PostgresNode::psql
> as much as it seemed reasonable to.  The -At options are kind of
> irrelevant for what we're going to test here, probably, but why not
> keep the default behavior the same?  I did drop -q since that
> suppresses prompting, and we probably want to test prompt.c using
> this infrastructure.

That is what my patch does: it tests prompts, tab completion, help, 
command options… and I added tests till I covered most psql source.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> No, I disagree with that.  If configure thinks it built with readline,
>> and then the actual binary acts like it doesn't have readline, that's
>> a bug that we'd like the tests to detect.

> Hmmm. Sure, that's a point.

> What about running some tests on an installed version?

I think "make installcheck" has plenty of dependencies already on the
build tree matching the installed version.  For instance, src/pl
will/won't run regression tests on languages it thinks was/weren't built.
If you want to run such tests retroactively, you'd better make sure you
configure your build tree to match the existing installation.

>> I mainly just duplicated the environment set up by PostgresNode::psql
>> as much as it seemed reasonable to.  The -At options are kind of
>> irrelevant for what we're going to test here, probably, but why not
>> keep the default behavior the same?  I did drop -q since that
>> suppresses prompting, and we probably want to test prompt.c using
>> this infrastructure.

> That is what my patch does: it tests prompts, tab completion, help, 
> command options… and I added tests till I covered most psql source.

Well, I think that where possible we ought to test using the existing test
infrastructure -- help, for example, seems like it could perfectly well be
tested in src/test/regress/sql/psql.sql, or we could move stuff out to a
new set of SQL test scripts under src/bin/psql/sql/, if it seems like we
don't need it to be part of the core tests.  But any tests using this new
infrastructure are going to be skipped by some percentage of test
machines, so we shouldn't skip what needn't be skipped.

            regards, tom lane

>> That is what my patch does: it tests prompts, tab completion, help,
>> command options… and I added tests till I covered most psql source.
>
> Well, I think that where possible we ought to test using the existing 
> test infrastructure -- help, for example, seems like it could perfectly 
> well be tested in src/test/regress/sql/psql.sql, or we could move stuff 
> out to a new set of SQL test scripts under src/bin/psql/sql/,

I do not think it is a good idea, because help output is quite large, 
there are many of them, and we should certainly not want it stored 
repeatedly in output files for diffs. I rather trigger the output and only 
check for some related keywords, so that it fits TAP tests reasonably 
well.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> Well, I think that where possible we ought to test using the existing 
>> test infrastructure -- help, for example, seems like it could perfectly 
>> well be tested in src/test/regress/sql/psql.sql, or we could move stuff 
>> out to a new set of SQL test scripts under src/bin/psql/sql/,

> I do not think it is a good idea, because help output is quite large, 
> there are many of them, and we should certainly not want it stored 
> repeatedly in output files for diffs.

Hm, I don't follow --- we are most certainly not going to exercise
\help for every possible SQL keyword, that'd just be silly.

Having said that, the fact that \help now includes a version-dependent
URL in its output is probably enough to break the idea of testing it
with a conventional expected-output test, so maybe TAP is the only
way for that.

            regards, tom lane

Hello Tom,

>> I do not think it is a good idea, because help output is quite large,
>> there are many of them, and we should certainly not want it stored
>> repeatedly in output files for diffs.
>
> Hm, I don't follow --- we are most certainly not going to exercise
> \help for every possible SQL keyword, that'd just be silly.

I am silly.

Price is pretty low, it helps with coverage in "sql_help.c, it checks that 
the help files returns adequate results so that adding new help contents 
does not hide existing stuff. I do not see why we should not do it, in TAP 
tests.

The alternative is that the project tolerates substandard test coverage. 
The "psql" command is currently around 40-44%.

> Having said that, the fact that \help now includes a version-dependent
> URL in its output is probably enough to break the idea of testing it
> with a conventional expected-output test, so maybe TAP is the only
> way for that.

The URL is a good thing, though.

-- 
Fabien.