Thread: pgsql: Add basic TAP tests for psql's tab-completion logic.
Add basic TAP tests for psql's tab-completion logic. Up to now, psql's tab-complete.c has had exactly no regression test coverage. This patch is an experimental attempt to add some. This needs Perl's IO::Pty module, which isn't installed everywhere, so the test script just skips all tests if that's not present. There may be other portability gotchas too, so I await buildfarm results with interest. So far this just covers a few very basic keyword-completion and query-driven-completion scenarios, which should be enough to let us get a feel for whether this is practical at all from a portability standpoint. If it is, there's lots more that can be done. Discussion: https://postgr.es/m/10967.1577562752@sss.pgh.pa.us Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/7c015045b9141cc30272930ea88cfa5df47240b7 Modified Files -------------- configure | 2 + configure.in | 1 + src/Makefile.global.in | 1 + src/bin/psql/.gitignore | 2 +- src/bin/psql/Makefile | 10 +++ src/bin/psql/t/010_tab_completion.pl | 122 +++++++++++++++++++++++++++++++++++ src/test/perl/PostgresNode.pm | 67 +++++++++++++++++++ 7 files changed, 204 insertions(+), 1 deletion(-)
On Thu, Jan 02, 2020 at 08:02:29PM +0000, Tom Lane wrote:
> Add basic TAP tests for psql's tab-completion logic.
>
> Up to now, psql's tab-complete.c has had exactly no regression test
> coverage. This patch is an experimental attempt to add some.
>
> This needs Perl's IO::Pty module, which isn't installed everywhere,
> so the test script just skips all tests if that's not present.
> There may be other portability gotchas too, so I await buildfarm
> results with interest.
Reading through the commit logs, I am not a fan of this part:
+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';
+}
This has the disadvantage to have people never actually notice if the
tests are running or not because this does not generate a dependency
error. Skipping things if libreadline is not around is perfectly fine
IMO, but I think that we should harden things for IO::Pty by removing
this skipping part, and by adding a test in configure.in's
AX_PROG_PERL_MODULES. That would be also more consistent with the
approach we take with other tests.
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> This has the disadvantage to have people never actually notice if the
> tests are running or not because this does not generate a dependency
> error. Skipping things if libreadline is not around is perfectly fine
> IMO, but I think that we should harden things for IO::Pty by removing
> this skipping part, and by adding a test in configure.in's
> AX_PROG_PERL_MODULES. That would be also more consistent with the
> approach we take with other tests.
I do not think that requiring IO::Pty is practical. It's not going
to be present in Windows installations, for starters, because it's
nonfunctional there. I've also found that it fails to compile on
some of my older buildfarm dinosaurs.
In the case of IPC::Run, having a hard dependency is sensible because
the TAP tests pretty much can't do anything at all without it.
However, we don't need IO::Pty except for testing a few psql behaviors,
so it's fine with me if some buildfarm members don't run those tests.
There's precedent, too: see for instance
src/test/recovery/t/017_shm.pl
which is where I stole this coding technique from.
regards, tom lane