Обсуждение: Re: Regression testing for psql
* 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
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
* 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
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.
* 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
* 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 psqlbackslash 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 thatthis needs to also be updated. Still, it's a start, we can reduce the set of tests if that makes sense or they becomea 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
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
* 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
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
* 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
* 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
Вложения
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