Thread

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-25T02:51:00Z

* Stephen Frost (sfrost@snowman.net) wrote:
>     Add regression testing for psql backslash commands
>     
>     This patch adds rather extensive regression testing
>     of the psql backslash commands.  Hopefully this will
>     minimize issues such as the one which cropped up
>     recently with \h segfaulting.  Note that we don't
>     currently explicit check all the \h options and that
>     pretty much any catalog changes will mean that this
>     needs to also be updated.  Still, it's a start, we can
>     reduce the set of tests if that makes sense or they
>     become a problem.

And..  it's way too big to send to the list.  The patch is available
here:

http://snowman.net/~sfrost/psql-regress-help.patch

Of course, if people want to suggest tests that just shouldn't be
included, I can go through and strip things out.

	Thanks,

		Stephen

Re: Regression testing for psql

Robert Haas <robertmhaas@gmail.com> — 2010-05-25T03:38:25Z

On Mon, May 24, 2010 at 10:51 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
>>     Add regression testing for psql backslash commands
>>
>>     This patch adds rather extensive regression testing
>>     of the psql backslash commands.  Hopefully this will
>>     minimize issues such as the one which cropped up
>>     recently with \h segfaulting.  Note that we don't
>>     currently explicit check all the \h options and that
>>     pretty much any catalog changes will mean that this
>>     needs to also be updated.  Still, it's a start, we can
>>     reduce the set of tests if that makes sense or they
>>     become a problem.
>
> And..  it's way too big to send to the list.  The patch is available
> here:
>
> http://snowman.net/~sfrost/psql-regress-help.patch
>
> Of course, if people want to suggest tests that just shouldn't be
> included, I can go through and strip things out.

Well...  I'm a little reluctant to believe that we should have 3.3M of
tests for the entire backend and 5M of tests just for psql.  Then,
too, there's the fact that many of these tests fail on my machine
because my username is not sfrost, and/or because of row-ordering
differences on backslash commands without enough ORDER BY to fully
determine the output order.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-25T10:23:53Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> > Of course, if people want to suggest tests that just shouldn't be
> > included, I can go through and strip things out.
> 
> Well...  I'm a little reluctant to believe that we should have 3.3M of
> tests for the entire backend and 5M of tests just for psql.  Then,
> too, there's the fact that many of these tests fail on my machine
> because my username is not sfrost, and/or because of row-ordering
> differences on backslash commands without enough ORDER BY to fully
> determine the output order.

Yeah, you know, I had fully intended to go grepping through the output
last night to check for things like that, but my wife decided I needed
sleep instead. :)  Sorry about that.  Still, it's more of a general
proposal than something I think should be committed as-is.  Should we
try to deal with those kinds of differences, or just eliminate the tests
which are dependent on username, etc?  It definitely strikes me that
there's a fair bit of code in psql we're not exercising in some fashion
in the regression suite... :/

	Thanks,

		Stephen

Re: Regression testing for psql

Peter Eisentraut <peter_e@gmx.net> — 2010-05-26T10:01:43Z

On tis, 2010-05-25 at 06:23 -0400, Stephen Frost wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > > Of course, if people want to suggest tests that just shouldn't be
> > > included, I can go through and strip things out.
> > 
> > Well...  I'm a little reluctant to believe that we should have 3.3M of
> > tests for the entire backend and 5M of tests just for psql.  Then,
> > too, there's the fact that many of these tests fail on my machine
> > because my username is not sfrost, and/or because of row-ordering
> > differences on backslash commands without enough ORDER BY to fully
> > determine the output order.
> 
> Yeah, you know, I had fully intended to go grepping through the output
> last night to check for things like that, but my wife decided I needed
> sleep instead. :)  Sorry about that.  Still, it's more of a general
> proposal than something I think should be committed as-is.  Should we
> try to deal with those kinds of differences, or just eliminate the tests
> which are dependent on username, etc?  It definitely strikes me that
> there's a fair bit of code in psql we're not exercising in some fashion
> in the regression suite... :/

Maybe pg_regress is not the right framework to test that sort of thing.

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-26T15:09:41Z

* Peter Eisentraut (peter_e@gmx.net) wrote:
> Maybe pg_regress is not the right framework to test that sort of thing.

Perhaps, but if not, then what?  And how can we avoid writing a bunch of
new code that would then need to be checked itself..?

	Thanks,

		Stephen

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-26T19:19:59Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> Then, too, there's the fact that many of these tests fail on my
> machine because my username is not sfrost, 

I've updated the patch to address this, it's again at:
http://snowman.net/~sfrost/psql-regress-help.patch

If the size is still an issue, I could just go through and remove the
commands which return lots of records (that would also remove most of
the info about the catalog, minimizing the effect on this set of tests
when people change the catalog..).  Downside there, of course, is that
we're not testing as many cases.  Still, better something than nothing.
:)

> and/or because of row-ordering differences on backslash commands
> without enough ORDER BY to fully determine the output order.

I don't believe this was ever actually an issue.  At least, I've run it
a number of times locally without issue.  If anyone is still getting
differences when run against 9.0 HEAD, please let me know.

commit e301c873740816c5f3fd5437dcc559c880b8f404
Author: Stephen Frost <sfrost@snowman.net>
Date:   Wed May 26 15:02:28 2010 -0400

    Add regression tests for psql backslash commands

    This patch adds rather extensive regression testing
    of the psql backslash commands.  Hopefully this will
    minimize issues such as the one which cropped up
    recently with \h segfaulting.  Note that we don't
    currently explicit check all the \h options and that
    many catalog changes will mean that this needs to also
    be updated.  Still, it's a start, we can reduce the
    set of tests if that makes sense or they become a
    problem.  Also, any tests which would include the
    owner of the database have been excluded.

    Patch here:
    http://snowman.net/~sfrost/psql-regress-help.patch

	Thanks,

		Stephen

Re: Regression testing for psql

Alvaro Herrera <alvherre@commandprompt.com> — 2010-05-26T19:52:29Z

Excerpts from Stephen Frost's message of mié may 26 15:19:59 -0400 2010:
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > Then, too, there's the fact that many of these tests fail on my
> > machine because my username is not sfrost, 
> 
> I've updated the patch to address this, it's again at:
> http://snowman.net/~sfrost/psql-regress-help.patch

Isn't this kind of test a pain to maintain?  If somebody add a new SQL
command, it will affect the entire \h output and she'll have to either
apply the changes without checking them, or manually check the complete
list.  I have only to add a new function to make the test fail ...

Also, having to exclude tests that mention the database owner means that
you're only testing a fraction of the commands, so any possible problem
has a large chance of going undetected.  I mean, if we're going to test
this kind of thing, shouldn't we be using something that allows us to
ignore the db owner name?  A simple wildcard in place of the owner name
would suffice ... or do we need a regex for some other reason?

The \h output normally depends on terminal width.  Have you handled that
somehow?

(And if we want something like this, I think we should not have a single
huge file for the complete test, but a set of smaller files.  I'd even
put the bunch in src/bin/psql/regress rather than the main regress dir.)

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-26T23:57:26Z

* alvherre (alvherre@commandprompt.com) wrote:
> Excerpts from Stephen Frost's message of mié may 26 15:19:59 -0400 2010:
> > * Robert Haas (robertmhaas@gmail.com) wrote:
> > > Then, too, there's the fact that many of these tests fail on my
> > > machine because my username is not sfrost, 
> > 
> > I've updated the patch to address this, it's again at:
> > http://snowman.net/~sfrost/psql-regress-help.patch
> 
> Isn't this kind of test a pain to maintain?  If somebody add a new SQL
> command, it will affect the entire \h output and she'll have to either
> apply the changes without checking them, or manually check the complete
> list.  I have only to add a new function to make the test fail ...

Erm, last I checked, we already provide a diff output for the changes to
the regression output.  That doesn't help when you add a new column to a
catalog, but that's a far cry from just adding some new SQL syntax or
adding a function.

> Also, having to exclude tests that mention the database owner means that
> you're only testing a fraction of the commands, so any possible problem
> has a large chance of going undetected.  I mean, if we're going to test
> this kind of thing, shouldn't we be using something that allows us to
> ignore the db owner name?  A simple wildcard in place of the owner name
> would suffice ... or do we need a regex for some other reason?

Yes, being able to use a regexp or something would be nice, but we don't
have those mechanics in the regression tests now (unless I missed
something).

> The \h output normally depends on terminal width.  Have you handled that
> somehow?

I don't think that'll actually be an issue, but would love to hear from
people if it is.

> (And if we want something like this, I think we should not have a single
> huge file for the complete test, but a set of smaller files.  I'd even
> put the bunch in src/bin/psql/regress rather than the main regress dir.)

The actual set of tests is rather small.  The output is large, but
that's just because we have alot of things in the catalog.

	Thanks,

		Stephen

Re: Regression testing for psql

Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-27T00:30:36Z

Stephen Frost <sfrost@snowman.net> writes:
> * alvherre (alvherre@commandprompt.com) wrote:
>> (And if we want something like this, I think we should not have a single
>> huge file for the complete test, but a set of smaller files.  I'd even
>> put the bunch in src/bin/psql/regress rather than the main regress dir.)

> The actual set of tests is rather small.  The output is large, but
> that's just because we have alot of things in the catalog.

It sounds to me like this is going to be like the rules regression test
writ large; specifically the part that dumps out view definitions for
all the built-in views.  And that, quite frankly, has been a huge
maintenance burden and AFAIR has never once had any redeeming social
value in terms of catching a bug.  If you're testing things that way,
don't.  There might be some value in psql backslash command tests that
are designed to depend on just one or a few tables (or other appropriate
objects).  Dumping large fractions of the catalogs will just be a net
loss.

			regards, tom lane

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-27T00:36:34Z

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> There might be some value in psql backslash command tests that
> are designed to depend on just one or a few tables (or other appropriate
> objects).  Dumping large fractions of the catalogs will just be a net
> loss.

Fair enough, I can certainly do that pretty easily.  Stripping out the
unqualified \d*S commands should result in a much, much, much smaller
output, with corresponding less changes due to catalog changes.

	Thanks,

		Stephen

Re: Regression testing for psql

Stephen Frost <sfrost@snowman.net> — 2010-05-27T01:25:52Z

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> There might be some value in psql backslash command tests that
> are designed to depend on just one or a few tables (or other appropriate
> objects).

Updated, much much smaller, patch attached.  Also available, again, at
http://snowman.net/~sfrost/psql-regress-help.patch

Basically, I removed anything that would produce data directly from
the catalogs by trying to find a 'none' object which matched.  This
still goes through alot of the same setup and query, it's just that
there aren't any results.

	Thanks,

		Stephen

Re: Regression testing for psql

Selena Deckelmann <selenamarie@gmail.com> — 2010-05-30T00:37:59Z

On Wed, May 26, 2010 at 6:25 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> There might be some value in psql backslash command tests that
>> are designed to depend on just one or a few tables (or other appropriate
>> objects).
>
> Updated, much much smaller, patch attached.  Also available, again, at
> http://snowman.net/~sfrost/psql-regress-help.patch
>
> Basically, I removed anything that would produce data directly from
> the catalogs by trying to find a 'none' object which matched.  This
> still goes through alot of the same setup and query, it's just that
> there aren't any results.

Is this something to be added to 2010-07 commitfest?

-selena


-- 
http://chesnok.com/daily - me