psql - improve test coverage from 41% to 88%

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Daniel Gustafsson
> On 5 Jul 2020, at 13:38, Daniel Gustafsson <[hidden email]> wrote:
>
>> On 24 Mar 2020, at 15:47, David Steele <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

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

Daniel Gustafsson
> On 1 Aug 2020, at 09:06, Fabien COELHO <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
Daniel Gustafsson <[hidden email]> writes:
> On 1 Aug 2020, at 09:06, Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Andrew Dunstan-8

On 8/1/20 5:27 PM, Tom Lane wrote:

> Daniel Gustafsson <[hidden email]> writes:
>> On 1 Aug 2020, at 09:06, Fabien COELHO <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Fabien COELHO-3
In reply to this post by Daniel Gustafsson

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


Reply | Threaded
Open this post in threaded view
|

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

Andrew Dunstan-8

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



12