Horton

Thread

Commits

  1. Improve coverage of psql for backslash commands with \if and \elif

  2. Fix comment in psql's describe.c

  1. psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-08-28T14:31:00Z

    Hello devs,

The attached patch improves psql code coverage by adding a specific TAP 
test. The 1709 tests take 4 seconds CPU (6.3 elapsed time) on my laptop.

The infrastructure is updated to require perl module "Expect", allowing to 
test interactive features such as tab completion and prompt changes.

Coverage in "src/bin/psql" jumps from 40.0% of lines and 41.9% of 
functions to 78.4% of lines and 98.1% of functions with "check-world".

-- 
Fabien.

  2. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-09-03T06:06:43Z

    Attached is a rebase after TestLib.pm got a documentation in 6fcc40b1.

> The attached patch improves psql code coverage by adding a specific TAP test. 
> The 1709 tests take 4 seconds CPU (6.3 elapsed time) on my laptop.
>
> The infrastructure is updated to require perl module "Expect", allowing to 
> test interactive features such as tab completion and prompt changes.
>
> Coverage in "src/bin/psql" jumps from 40.0% of lines and 41.9% of functions 
> to 78.4% of lines and 98.1% of functions with "check-world".

-- 
Fabien.

  3. Re: psql - improve test coverage from 41% to 88%

    Michael Paquier <michael@paquier.xyz> — 2019-09-11T06:25:59Z

    On Tue, Sep 03, 2019 at 08:06:43AM +0200, Fabien COELHO wrote:
> Attached is a rebase after TestLib.pm got a documentation in
> 6fcc40b1.

I am not completely sure what to think about this patch, but here are
some high-level comments.

+=item $node->icommand_checks(cmd, ...)
+
+=cut
+
+sub icommand_checks

Surely this can have a better description, like say
PostgresNode::command_checks_all.

Is Expect compatible down to perl 5.8.0 which is the minimum required
for the TAP tests (see src/test/perl/README)?

There are cases where we don't support tab completion, aka no
USE_READLINE.  So tests would need to be skipped.

-   \a \C arg1 \c arg1 arg2 arg3 arg4 \cd arg1 \conninfo
+   \a
+    \C arg1
Why are you changing that?  Your patch does not touch the logic of
psql.  Could it make sense as a separate patch to shape better the
tests?

--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -307,6 +307,7 @@ describeTablespaces(const char *pattern, bool
verbose)
  * a for aggregates
  * n for normal
+ * p for procedure
This is a separate issue, fixed.
--
Michael

  4. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-09-11T20:52:01Z

    Bonjour Michaël,

> +=item $node->icommand_checks(cmd, ...)
> +
> +=cut
> +
> +sub icommand_checks
>
> Surely this can have a better description, like say
> PostgresNode::command_checks_all.

Ok.

> Is Expect compatible down to perl 5.8.0 which is the minimum required
> for the TAP tests (see src/test/perl/README)?

I think so. It looks like this has existed for a very long time (22 
years?), but I cannot test it simply against a perl 5.8.

> There are cases where we don't support tab completion, aka no
> USE_READLINE.  So tests would need to be skipped.

Good catch. I added a skip if it detects that history/readline is 
disabled.

>  -   \a \C arg1 \c arg1 arg2 arg3 arg4 \cd arg1 \conninfo
>  +   \a
>  +    \C arg1
> Why are you changing that?

AFAICR this is because the coverage was not the same:-) Some backslash 
commands just skip silently to the end of the line, so that intermediate 
\commands on the same line are not recognized/processed the same, so I 
moved everything on one line to avoid this.

> Your patch does not touch the logic of psql.  Could it make sense as a 
> separate patch to shape better the tests?

Nope, this is not just reshaping, it is really about improving coverage.

> --- a/src/bin/psql/describe.c
> +++ b/src/bin/psql/describe.c
> @@ -307,6 +307,7 @@ describeTablespaces(const char *pattern, bool
> verbose)
>  * a for aggregates
>  * n for normal
> + * p for procedure
> This is a separate issue, fixed.

Ok.

Attached v3 which fixes the outlined issues.

-- 
Fabien.

  5. Re: psql - improve test coverage from 41% to 88%

    Michael Paquier <michael@paquier.xyz> — 2019-09-12T01:37:24Z

    On Wed, Sep 11, 2019 at 10:52:01PM +0200, Fabien COELHO wrote:
> AFAICR this is because the coverage was not the same:-) Some backslash
> commands just skip silently to the end of the line, so that intermediate
> \commands on the same line are not recognized/processed the same, so I moved
> everything on one line to avoid this.

I see.  So basically this tests for more code paths to ignore
backslash commands and it improves the coverage of \elif.  Applied
after fixing two nits:
- Indentation was incorrect.
- Moved the \elif test closer to the existing one for the false
branch (you can grep #2 to find it).
--
Michael

  6. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-09-12T06:25:50Z

    > On Wed, Sep 11, 2019 at 10:52:01PM +0200, Fabien COELHO wrote:
>> AFAICR this is because the coverage was not the same:-) Some backslash
>> commands just skip silently to the end of the line, so that intermediate
>> \commands on the same line are not recognized/processed the same, so I moved
>> everything on one line to avoid this.
>
> I see.  So basically this tests for more code paths to ignore
> backslash commands and it improves the coverage of \elif.  Applied
> after fixing two nits:
> - Indentation was incorrect.
> - Moved the \elif test closer to the existing one for the false
> branch (you can grep #2 to find it).

Ok. Rebased version added, with some minor changes to improve readability 
(comments, variables).

-- 
Fabien.

  7. Re: psql - improve test coverage from 41% to 88%

    vignesh C <vignesh21@gmail.com> — 2019-09-12T08:07:37Z

    On Thu, Sep 12, 2019 at 11:56 AM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> > On Wed, Sep 11, 2019 at 10:52:01PM +0200, Fabien COELHO wrote:
> >> AFAICR this is because the coverage was not the same:-) Some backslash
> >> commands just skip silently to the end of the line, so that intermediate
> >> \commands on the same line are not recognized/processed the same, so I moved
> >> everything on one line to avoid this.
> >
> > I see.  So basically this tests for more code paths to ignore
> > backslash commands and it improves the coverage of \elif.  Applied
> > after fixing two nits:
> > - Indentation was incorrect.
> > - Moved the \elif test closer to the existing one for the false
> > branch (you can grep #2 to find it).
>
> Ok. Rebased version added, with some minor changes to improve readability
> (comments, variables).
>
>
Few comments:
+sub create_test_file
+{
+ my ($fname, $contents) = @_;
+ my $fn = $node->basedir . '/' . $fname;
+ #ok(not -e $fn, "$fn must not already exists");
+ append_to_file($fn, $contents);
+ return $fn;
+}

Commented line can be removed

+# nope, interacts on tty
+#psql('-W', 0, "foo\n", [ qr{^$} ], [ qr{^$} ], 'psql -W');
+psql('-x', 0, "SELECT 1 AS one, 2 AS two;\n", [ qr{one \| 1.*two \|
2}s ], $EMPTY, 'psql -x');
+# some issue, \0 is not welcome somewhere
+#psql('-A -z', "SELECT 1 AS one, 2 AS two;\n", [ qr{one.two}s,
qr{1.2}s ], $EMPTY, 'psql -z');
+#psql('-A -0', "SELECT 1 AS one, 2 AS two;\n", [ qr{two.1}s ],
$EMPTY, 'psql -0');
+psql('-1', 0, "SELECT 54;\nSELECT 32;\n", [ qr{54}, qr{32} ], $EMPTY,
'psql -1');

Commented lines can be removed

+ [ "\\lo_list\n", [ qr{Large objects} ] ],
+ [ "\\if true\\q\\endif\n", $EMPTY ],
+ # ???
+ #[ "SELECT md5('hello world');\n\\s\n", [ qr{5eb63bbbe0}, qr{SELECT md5} ] ],
+ [ "\\set\n", [ qr{ENCODING = }, qr{VERSION_NUM = } ] ],
+ [ "\\set COMP_KEYWORD_CASE preserve-lower\n\\set COMP_KEYWORD_CASE lower\n" .

#[ "Select"] commented line can be removed
??? can be changed to some suitable heading

+psql('', 0, "\\s /dev/null\n", $EMPTY, $EMPTY, 'psql \s null');
+
+# tab-complation
+ipsql('-P pager', 0, 5,
+  [ # commands

tab-complation to be changed to tab-completion

+ # but the coverage works as expected.
+ #[ "CREATE \t", qr/i(MATERIALIZED VIEW.*postgres=\# )?/s ],
+ #[ "\\r\n", qr/Query buffer reset.*postgres=\# /s ],
+ [ "CREATE \t\\r\n", qr/Query buffer reset.*postgres=\# /s ],
+ #[ "DROP \t", qr/(UNLOGGED.*postgres=\# )?/s ],
+ #[ "\\r\n", qr/Query buffer reset.*postgres=\# /s ],
+ [ "DROP \t\\r\n", qr/Query buffer reset.*postgres=\# /s ],
+ #[ "ALTER \t", qr/(TABLESPACE.*postgres=\# )/s ],
+ #[ "\\r\n", qr/Query buffer reset.*postgres=\# /s ],

Commented lines can be removed, some more are present below these lines also.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  8. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-09-12T08:45:06Z

    >> Ok. Rebased version added, with some minor changes to improve readability
>> (comments, variables).
>
> Few comments: [...]
>
> Commented line can be removed
> Commented lines can be removed
> ??? can be changed to some suitable heading
> tab-complation to be changed to tab-completion
> Commented lines can be removed, some more are present below these lines also.

Thanks for this review.

The lines were really tests I did that had some issues because of the way 
the Expect module works, and are not useful for inclusion in the code 
base.

Here is a v5.

-- 
Fabien Coelho - CRI, MINES ParisTech

  9. Re: psql - improve test coverage from 41% to 88%

    vignesh C <vignesh21@gmail.com> — 2019-09-12T10:48:36Z

    On Thu, Sep 12, 2019 at 2:15 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>
> >> Ok. Rebased version added, with some minor changes to improve readability
> >> (comments, variables).
> >
> > Few comments: [...]
> >
> > Commented line can be removed
> > Commented lines can be removed
> > ??? can be changed to some suitable heading
> > tab-complation to be changed to tab-completion
> > Commented lines can be removed, some more are present below these lines also.
>
> Thanks for this review.
>
> The lines were really tests I did that had some issues because of the way
> the Expect module works, and are not useful for inclusion in the code
> base.
>
> Here is a v5.
Few more in icommand_checks subroutine:

+
+ #$ps->slave->stty(qw(raw -echo));
+ $ps->slave->stty(qw(raw));
+ my $n = 0;
+ for my $test (@$inout)
+ {
+ #warn "test: @$test";
+ my ($in, @out) = @$test;
+ $n++;
+ #warn "in: $in";
+ #warn "out: @out";
+ $ps->send($in);

Few unwanted code can be removed.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  10. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-09-12T11:19:28Z

    >> Here is a v5.

> Few more in icommand_checks subroutine:
> Few unwanted code can be removed.

Indeed, more debug and test code.

Attached v6 fixes these, and I checked for remaining scrubs without 
finding any.

-- 
Fabien.

  11. Re: psql - improve test coverage from 41% to 88%

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-12T15:14:16Z

    I think the TestLib.pm changes should be done separately, not together
with the rest of the hacking in this patch.

Mostly, because I think they're going to cause trouble.  Adding a
parameter in the middle of the list may cause trouble for third-party
users of TestLib.  I propose that we make the routines a bit smarter to
cope with the API change: use named parameters instead.  And in order to
do that without having to change existing users of command_check, make
it so that the routine checks whether the parameter is a hashref, and
behave differently.  So when called as in the existing callsites (five
scalar paramters) it behaves as currently.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: psql - improve test coverage from 41% to 88%

    Michael Paquier <michael@paquier.xyz> — 2019-09-13T03:17:46Z

    On Thu, Sep 12, 2019 at 12:14:16PM -0300, Alvaro Herrera wrote:
> Mostly, because I think they're going to cause trouble.  Adding a
> parameter in the middle of the list may cause trouble for third-party
> users of TestLib.  I propose that we make the routines a bit smarter to
> cope with the API change: use named parameters instead.  And in order to
> do that without having to change existing users of command_check, make
> it so that the routine checks whether the parameter is a hashref, and
> behave differently.  So when called as in the existing callsites (five
> scalar parameters) it behaves as currently.

+1.
--
Michael

  13. Re: psql - improve test coverage from 41% to 88%

    vignesh C <vignesh21@gmail.com> — 2019-09-17T11:18:06Z

    On Thu, Sep 12, 2019 at 4:49 PM Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
>
> >> Here is a v5.
>
> > Few more in icommand_checks subroutine:
> > Few unwanted code can be removed.
>
> Indeed, more debug and test code.
>
> Attached v6 fixes these, and I checked for remaining scrubs without
> finding any.
>
Few comments:
+ [ 'START TRANSACTION', [ qr{ISOLATION LEVEL}, qr{(?!BEGIN)} ] ],
+ [ 'TABLE', [ qr{ONLY} ] ], # hmmm...
+ [ 'TRUNCATE', [ qr{CONTINUE IDENTITY} ] ],
+ [ 'UNLISTEN', [ ] ],

We can remove # hmmm... if not required

+ [ 'UPDATE', [ qr{RETURNING} ] ],
+ [ 'VACUUM', [ qr{FREEZE} ] ],
+ [ 'VALUES', [ qr{ORDER BY} ] ],
+ [ 'WITH', [ qr{RECURSIVE} ] ], # SELECT duplicate?
+);

We can remove # SELECT duplicate? if not required

+
+psql('--log-file=/dev/null', 0, "SELECT 5432 AS pg\n",
+ [ qr/\b5432\b/ ], $EMPTY, 'psql -L null');
+
+psql('', 0, "\\copy public.regress_psql_tap_1_t1(data) FROM PROGRAM
'echo moe'\n",
+ [ qr/COPY 1\b/ ], $EMPTY, 'psql copy echo');
+psql('', 0, "\\copy public.regress_psql_tap_1_t1(data) TO PROGRAM 'cat'\n",
+ [ qr/COPY 1\b/ ], $EMPTY, 'psql copy cat'); # :-)
+
+END_UNIX_ZONE:

We can remove # :-) if not required


Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

  14. Re: psql - improve test coverage from 41% to 88%

    Michael Paquier <michael@paquier.xyz> — 2019-11-27T05:03:21Z

    Hi Fabien,

On Tue, Sep 17, 2019 at 04:48:06PM +0530, vignesh C wrote:
> Few comments:
> + [ 'START TRANSACTION', [ qr{ISOLATION LEVEL}, qr{(?!BEGIN)} ] ],
> + [ 'TABLE', [ qr{ONLY} ] ], # hmmm...
> + [ 'TRUNCATE', [ qr{CONTINUE IDENTITY} ] ],
> + [ 'UNLISTEN', [ ] ],
> 
> We can remove # hmmm... if not required
> 
> + [ 'UPDATE', [ qr{RETURNING} ] ],
> + [ 'VACUUM', [ qr{FREEZE} ] ],
> + [ 'VALUES', [ qr{ORDER BY} ] ],
> + [ 'WITH', [ qr{RECURSIVE} ] ], # SELECT duplicate?
> +);
> 
> We can remove # SELECT duplicate? if not required
> 
> +
> +psql('--log-file=/dev/null', 0, "SELECT 5432 AS pg\n",
> + [ qr/\b5432\b/ ], $EMPTY, 'psql -L null');
> +
> +psql('', 0, "\\copy public.regress_psql_tap_1_t1(data) FROM PROGRAM
> 'echo moe'\n",
> + [ qr/COPY 1\b/ ], $EMPTY, 'psql copy echo');
> +psql('', 0, "\\copy public.regress_psql_tap_1_t1(data) TO PROGRAM 'cat'\n",
> + [ qr/COPY 1\b/ ], $EMPTY, 'psql copy cat'); # :-)
> +
> +END_UNIX_ZONE:
> 
> We can remove # :-) if not required

Please note that you have received comments on this patch a couple of
weeks ago.  The patch was still marked as "needs review", which was
incorrect, and it does not apply.  Perhaps you did not notice it, so I
am moving it to next CF, waiting on author for a rebase and for
replies on those comments.
--
Michael

  15. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-11-27T09:14:16Z

    Bonjour Michaël,

> Please note that you have received comments on this patch a couple of
> weeks ago.  The patch was still marked as "needs review", which was
> incorrect, and it does not apply.  Perhaps you did not notice it, so I
> am moving it to next CF, waiting on author for a rebase and for
> replies on those comments.

Indeed, I did not notice.

Attached a rebase, which also removes the 3 comments pointed out by 
Vignesh.

-- 
Fabien.

  16. Re: psql - improve test coverage from 41% to 88%

    Michael Paquier <michael@paquier.xyz> — 2019-11-28T04:01:10Z

    On Wed, Nov 27, 2019 at 10:14:16AM +0100, Fabien COELHO wrote:
> Indeed, I did not notice.

Thanks, Fabien!
--
Michael

  17. Re: psql - improve test coverage from 41% to 88%

    David Steele <david@pgmasters.net> — 2020-03-24T14:47:00Z

    Hi Fabien,

On 11/27/19 11:01 PM, Michael Paquier wrote:
> On Wed, Nov 27, 2019 at 10:14:16AM +0100, Fabien COELHO wrote:
>> Indeed, I did not notice.


This patch no longer applies: http://cfbot.cputube.org/patch_27_2262.log

CF entry has been updated to Waiting on Author.

Regards,
-- 
-David
david@pgmasters.net

  18. Re: psql - improve test coverage from 41% to 88%

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-05T11:38:33Z

    > On 24 Mar 2020, at 15:47, David Steele <david@pgmasters.net> wrote:

> This patch no longer applies: http://cfbot.cputube.org/patch_27_2262.log
> 
> CF entry has been updated to Waiting on Author.

This patch hasn't been updated and still doesn't apply, do you intend to rebase
it during this commitfest or should we move it to returned with feedback?  It
can always be re-opened at a later date.

cheers ./daniel

  19. Re: psql - improve test coverage from 41% to 88%

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-30T21:47:23Z

    > On 5 Jul 2020, at 13:38, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 24 Mar 2020, at 15:47, David Steele <david@pgmasters.net> wrote:
> 
>> This patch no longer applies: http://cfbot.cputube.org/patch_27_2262.log
>> 
>> CF entry has been updated to Waiting on Author.
> 
> This patch hasn't been updated and still doesn't apply, do you intend to rebase
> it during this commitfest or should we move it to returned with feedback?  It
> can always be re-opened at a later date.

As the thread has stalled, I've marked this Returned with Feedback.

cheers ./daniel

  20. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-08-01T07:06:39Z

    Hello,

>>> This patch no longer applies: http://cfbot.cputube.org/patch_27_2262.log
>>>
>>> CF entry has been updated to Waiting on Author.
>>
>> This patch hasn't been updated and still doesn't apply, do you intend to rebase
>> it during this commitfest or should we move it to returned with feedback?  It
>> can always be re-opened at a later date.
>
> As the thread has stalled, I've marked this Returned with Feedback.

Hmmm.

AFAICR the feedback is that the Expect perl module is not welcome, which 
seems to suggest that it would have to be re-implemented somehow. This is 
not my dev philosophy, I won't do that, so I'm sorry to say that psql 
coverage will remain pretty abysmal.

Sigh.

-- 
Fabien.

  21. Re: psql - improve test coverage from 41% to 88%

    Daniel Gustafsson <daniel@yesql.se> — 2020-08-01T20:42:32Z

    > On 1 Aug 2020, at 09:06, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> 
> Hello,
> 
>>>> This patch no longer applies: http://cfbot.cputube.org/patch_27_2262.log
>>>> 
>>>> CF entry has been updated to Waiting on Author.
>>> 
>>> This patch hasn't been updated and still doesn't apply, do you intend to rebase
>>> it during this commitfest or should we move it to returned with feedback?  It
>>> can always be re-opened at a later date.
>> 
>> As the thread has stalled, I've marked this Returned with Feedback.
> 
> Hmmm.
> 
> AFAICR the feedback is that the Expect perl module is not welcome, which seems to suggest that it would have to be re-implemented somehow. This is not my dev philosophy, I won't do that, so I'm sorry to say that psql coverage will remain pretty abysmal.

Re-reading this thread, I see no complaints about introducing a dependency on
Expect.  The feedback returned in this case is that the patch hasn't applied
since March, and that the patch is more than welcome to be re-entered in the
next CF once it does.

cheers ./daniel

  22. Re: psql - improve test coverage from 41% to 88%

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-01T21:27:53Z

    Daniel Gustafsson <daniel@yesql.se> writes:
> On 1 Aug 2020, at 09:06, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> AFAICR the feedback is that the Expect perl module is not welcome, which seems to suggest that it would have to be re-implemented somehow. This is not my dev philosophy, I won't do that, so I'm sorry to say that psql coverage will remain pretty abysmal.

> Re-reading this thread, I see no complaints about introducing a dependency on
> Expect.  The feedback returned in this case is that the patch hasn't applied
> since March, and that the patch is more than welcome to be re-entered in the
> next CF once it does.

Personally, I'd object to introducing a hard dependency on Expect, as
there are no doubt a lot of developers and buildfarm members who don't
have that installed.  But I see no reason we couldn't skip some tests
if it's lacking, as we're already doing with IO::Pty in
010_tab_completion.pl.

That does raise the question of whether Expect makes things enough
easier than raw IO::Pty that it's worth adding that dependency (and
hence foregoing the tests on some machines).  But I'm prepared to be
convinced on that point.

			regards, tom lane

  23. Re: psql - improve test coverage from 41% to 88%

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-08-02T15:10:23Z

    On 8/1/20 5:27 PM, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 1 Aug 2020, at 09:06, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>>> AFAICR the feedback is that the Expect perl module is not welcome, which seems to suggest that it would have to be re-implemented somehow. This is not my dev philosophy, I won't do that, so I'm sorry to say that psql coverage will remain pretty abysmal.
>> Re-reading this thread, I see no complaints about introducing a dependency on
>> Expect.  The feedback returned in this case is that the patch hasn't applied
>> since March, and that the patch is more than welcome to be re-entered in the
>> next CF once it does.
> Personally, I'd object to introducing a hard dependency on Expect, as
> there are no doubt a lot of developers and buildfarm members who don't
> have that installed.  But I see no reason we couldn't skip some tests
> if it's lacking, as we're already doing with IO::Pty in
> 010_tab_completion.pl.
>
> That does raise the question of whether Expect makes things enough
> easier than raw IO::Pty that it's worth adding that dependency (and
> hence foregoing the tests on some machines).  But I'm prepared to be
> convinced on that point.
>
> 			


+1. Also note that the Windows animals don't and probably will never
support Expect, since Windows doesn't have PTYs. Expect.pm is in fact a
pure perl module that sits on top of IO::Pty, which in turn sits on top
of IO::Tty. So if you have those Expect.pm probably isn't a huge
stretch. But let's not add a dependency if we can avoid it. And if we do
add one it will need to be a soft one like the case you mentioned.


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  24. Re: psql - improve test coverage from 41% to 88%

    Michael Paquier <michael@paquier.xyz> — 2020-08-03T02:06:19Z

    On Sun, Aug 02, 2020 at 11:10:23AM -0400, Andrew Dunstan wrote:
> +1. Also note that the Windows animals don't and probably will never
> support Expect, since Windows doesn't have PTYs. Expect.pm is in fact a
> pure perl module that sits on top of IO::Pty, which in turn sits on top
> of IO::Tty. So if you have those Expect.pm probably isn't a huge
> stretch. But let's not add a dependency if we can avoid it. And if we do
> add one it will need to be a soft one like the case you mentioned.

Even with that, do we really care about some code coverage specific to
Windows for tab-complete.c?  Also, how complicated does the proposed
patch become if we remove the dependency to Expect.pm and just rely on
IO::Pty?
--
Michael

  25. Re: psql - improve test coverage from 41% to 88%

    Fabien COELHO <coelho@cri.ensmp.fr> — 2020-08-03T19:34:47Z

    > Re-reading this thread, I see no complaints about introducing a 
> dependency on Expect.

Indeed, Tom's complaint was on another thread, possibly when interactive 
tests "src/bin/psql/t/010_tab_completion.pl" were added.

ISTM that one of the issue was that some farm animal would be broken.

I'm quite lost about Expect portability discussion wrt windows, it is 
unclear to me whether it is expected to work there or not.

As I stated, I do not like re-inventing the wheel, probably quite badly, 
when someone else already did a good job.

-- 
Fabien.

  26. Re: psql - improve test coverage from 41% to 88%

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-08-03T22:44:03Z

    On 8/3/20 3:34 PM, Fabien COELHO wrote:
>
>
> I'm quite lost about Expect portability discussion wrt windows, it is
> unclear to me whether it is expected to work there or not.



Sorry if I was unclear. Expect will not work on Windows. Nor will use of
IO::Pty  or IO::Tty, which are what Expect uses under the hood. So use
of any of that needs to be done just as it is done on
010_tab_completion.pl, i.e.


    eval { require IO::Pty; };
    if ($@)
    {
        plan skip_all => 'IO::Pty is needed to run this test';
    }


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services