psql - improve test coverage from 41% to 88%

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

psql - improve test coverage from 41% to 88%

Fabien COELHO-3

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.

psql-tap-1.patch (73K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Fabien COELHO-3

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.

psql-tap-2.patch (73K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

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

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

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.

psql-tap-3.patch (73K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

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

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

psql-tap-4.patch (67K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

vignesh C
On Thu, Sep 12, 2019 at 11:56 AM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fabien COELHO-3

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

psql-tap-5.patch (65K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

vignesh C
On Thu, Sep 12, 2019 at 2:15 PM Fabien COELHO <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fabien COELHO-3

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

psql-tap-6.patch (65K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

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

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

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

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

Alvaro Herrera-9
In reply to this post by Fabien COELHO-3
On 2019-Sep-13, Fabien COELHO wrote:

> Hello Alvaro,
>
> > 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.
>
> That is also what I thought, however, see below.

I see.  But you seem to have skipped my suggestion without considering
it.

I think the current API of these functions where they just receive a
plain array of arguments, and all callers have to be patched in unison,
is not very convenient.  Also, I *think* your new icommand_checks method
is the same as command_checks_all, except that you also have the "init"
part.  So you're duplicating code because the original doesn't have
functionality you need?  But why do that, if you could have *one*
function that does both things?  If some callers don't have the "init"
part, just omit it from the parameters.

(Whether it's implemented using Expect or not should not matter.  Either
Expect works everywhere, and we can use it, or it doesn't and we can't.)

--
Álvaro Herrera                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%

Fabien COELHO-3

Hello Alvaro,

>>> 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.
>>
>> That is also what I thought, however, see below.
>
> I see.  But you seem to have skipped my suggestion without considering
> it.

I did understand it, but as Tom did not want simple hocus-pocus, ISTM that
dynamically checking the argument type would not be considered a very good
idea either.

> I think the current API of these functions where they just receive a
> plain array of arguments, and all callers have to be patched in unison,
> is not very convenient.

I agree, but the no diff solution was rejected. I can bring one back, but
going against Tom's views has not proven a good move in the past.

> Also, I *think* your new icommand_checks method is the same as
> command_checks_all, except that you also have the "init" part.

Nope, it is an interactive version based on Expect, which sends input and
waits for output, the process is quite different from a simple one shot no
timeout exec version.

> So you're duplicating code because the original doesn't have
> functionality you need?

Yes, I'm creating a interactive validation variant.

> But why do that, if you could have *one* function that does both things?
> If some callers don't have the "init" part, just omit it from the
> parameters.

Although it could be abstracted somehow, I do not think that having one
function behaving so differently under the hood is a good idea. It is not
just the question of the init part.

> (Whether it's implemented using Expect or not should not matter.  Either
> Expect works everywhere, and we can use it, or it doesn't and we can't.)

For me the question is not about Expect dependency, it is more about how
the test behaves.

--
Fabien.