Horton

Thread

Commits

  1. Add basic TAP tests for psql's tab-completion logic.

  1. TAP testing for psql's tab completion code

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-28T19:52:32Z

    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

  2. Re: TAP testing for psql's tab completion code

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-12-28T21:16:52Z

    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.

  3. Re: TAP testing for psql's tab completion code

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-28T22:52:14Z

    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

  4. Re: TAP testing for psql's tab completion code

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-12-29T07:11:07Z

    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.

  5. Re: TAP testing for psql's tab completion code

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-29T17:13:42Z

    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

  6. Re: TAP testing for psql's tab completion code

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-12-29T17:42:36Z

    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.

  7. Re: TAP testing for psql's tab completion code

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-29T17:53:11Z

    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

  8. Re: TAP testing for psql's tab completion code

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-12-29T18:52:41Z

    >> 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.

  9. Re: TAP testing for psql's tab completion code

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-29T19:22:56Z

    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

  10. Re: TAP testing for psql's tab completion code

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-12-30T10:49:28Z

    >> 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.

  11. Re: TAP testing for psql's tab completion code

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-12-30T14:10:40Z

    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

  12. Re: TAP testing for psql's tab completion code

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-12-30T14:40:14Z

    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.