Thread: Rewrite of pg_dump TAP tests

Greetings,

Attached is a patch (which applies cleaning against a2a2205, but not so
much anymore, obviously, but I will fix after the releases) which
greatly improves the big pg_dump TAP tests.  There's probably more which
can be done, but I expect people will be much happier with this.  The
cliff-notes are:

Reworked how the tests are defined to instead of a set of 'full runs'
and then a way to exclude certain runs from certain tests (using the
same 'unlike' mechanism, but now it works to remove 'like' entries which
are pulled into the 'like' part by a broad hash).  This ends up removing
some 4k+ lines (more than half the file) but, more importantly, makes
the way the runs-to-be-tested are defined make more sense.

As discussed in the updated comments, for example, take the test which
does CREATE TABLE test_table.  That CREATE TABLE should show up in all
'full' runs of pg_dump, except those cases where 'test_table' is
excluded, of course, and that's exactly how the test gets defined now
(modulo a few other related cases, like where we dump only that table,
or we dump the schema it's in, or we exclude the schema it's in):

like => {
    %full_runs,
    %dump_test_schema_runs,
    only_dump_test_table    => 1,
    section_pre_data        => 1, },
unlike => {
    exclude_dump_test_schema => 1,
    exclude_test_table => 1, }, },

Next, we no longer expect every run to be listed for every test.  If a
run is listed in 'like' (directly or through a hash) then it's a 'like',
unless it's listed in 'unlike' in which case it's an 'unlike'.  If it
isn't listed in either, then it's considered an 'unlike' automatically.

Lastly, I've changed the code to no longer use like/unlike but rather to
use 'ok()' with 'diag()' which allows much more control over what gets
spit out to the screen.  Gone are the days of the entire dump being sent
to the console, now you'll just get a couple of lines for each failing
test which say the test that failed and the run that it failed on.  The
only concern I have with this is if it'll make analysis of buildfarm
failures difficult, but, thankfully, I've not seen too many cases of
pg_dump behavior differently in the buildfarm so perhaps this trade-off
for making it easier for hackers to work with will be worth the
reduction in clarity from the buildfarm in this specific case.

Comments and suggestions welcome, of course.  As mentioned, this applies
as of around a2a2205, but I will fix it either this weekend or early
next week to account for the recent changes.

Thanks!

Stephen

Hi,

On 2018-02-26 13:15:04 -0500, Stephen Frost wrote:
> Attached is a patch (which applies cleaning against a2a2205, but not so
> much anymore, obviously, but I will fix after the releases) which
> greatly improves the big pg_dump TAP tests.  There's probably more which
> can be done, but I expect people will be much happier with this.  The
> cliff-notes are:

Could you update?


Do you think this should be backpatched so we can backpatch tests when
backpatching fixes?

Greetings,

Andres Freund

Andres,

* Andres Freund (andres@anarazel.de) wrote:
> On 2018-02-26 13:15:04 -0500, Stephen Frost wrote:
> > Attached is a patch (which applies cleaning against a2a2205, but not so
> > much anymore, obviously, but I will fix after the releases) which
> > greatly improves the big pg_dump TAP tests.  There's probably more which
> > can be done, but I expect people will be much happier with this.  The
> > cliff-notes are:
>
> Could you update?

Working on it and will provide an update, hopefully by the end of the
weekend.

> Do you think this should be backpatched so we can backpatch tests when
> backpatching fixes?

That's an interesting question.  I hadn't planned to and it wouldn't be
trivial, but if others would like to see these changes back-patched then
I'm willing to put in the work to do it.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> Attached is a patch (which applies cleaning against a2a2205, but not so
> much anymore, obviously, but I will fix after the releases) which
> greatly improves the big pg_dump TAP tests.  There's probably more which
> can be done, but I expect people will be much happier with this.  The
> cliff-notes are:

Attached is an updated patch which applies cleanly against current
master.  I've not yet looked into back-patching these changes, but will
if this can get some review and feedback, and folks like this better and
are ok with the same being done in the back-branches.

Thanks!

Stephen

Stephen Frost wrote:
> Greetings,
> 
> * Stephen Frost (sfrost@snowman.net) wrote:
> > Attached is a patch (which applies cleaning against a2a2205, but not so
> > much anymore, obviously, but I will fix after the releases) which
> > greatly improves the big pg_dump TAP tests.  There's probably more which
> > can be done, but I expect people will be much happier with this.  The
> > cliff-notes are:
> 
> Attached is an updated patch which applies cleanly against current
> master.  I've not yet looked into back-patching these changes, but will
> if this can get some review and feedback, and folks like this better and
> are ok with the same being done in the back-branches.

Just looking at the commit message (sql script not dumped to screen),
and the fact that this removes the bulk of the like/unlike arrays, I'd
say this is a definite improvement over the existing tests.

Perhaps we'd like to tweak more later, but this is a good step forward
IMO.

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

On Tue, Mar 06, 2018 at 11:53:41AM -0500, Stephen Frost wrote:
> Greetings,
>
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Attached is a patch (which applies cleaning against a2a2205, but not so
>> much anymore, obviously, but I will fix after the releases) which
>> greatly improves the big pg_dump TAP tests.  There's probably more which
>> can be done, but I expect people will be much happier with this.  The
>> cliff-notes are:
>
> Attached is an updated patch which applies cleanly against current
> master.  I've not yet looked into back-patching these changes, but will
> if this can get some review and feedback, and folks like this better and
> are ok with the same being done in the back-branches.

It would be nice to improve the readability of test_pg_dump's
001_base.pl at the same time by using similarly a hash syntax for common
options.
--
Michael

Michael,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Mar 06, 2018 at 11:53:41AM -0500, Stephen Frost wrote:
> > * Stephen Frost (sfrost@snowman.net) wrote:
> >> Attached is a patch (which applies cleaning against a2a2205, but not so
> >> much anymore, obviously, but I will fix after the releases) which
> >> greatly improves the big pg_dump TAP tests.  There's probably more which
> >> can be done, but I expect people will be much happier with this.  The
> >> cliff-notes are:
> >
> > Attached is an updated patch which applies cleanly against current
> > master.  I've not yet looked into back-patching these changes, but will
> > if this can get some review and feedback, and folks like this better and
> > are ok with the same being done in the back-branches.
>
> It would be nice to improve the readability of test_pg_dump's
> 001_base.pl at the same time by using similarly a hash syntax for common
> options.

Yes, good point, will fix.

Thanks!

Stephen

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Michael Paquier (michael@paquier.xyz) wrote:
> > On Tue, Mar 06, 2018 at 11:53:41AM -0500, Stephen Frost wrote:
> > > * Stephen Frost (sfrost@snowman.net) wrote:
> > >> Attached is a patch (which applies cleaning against a2a2205, but not so
> > >> much anymore, obviously, but I will fix after the releases) which
> > >> greatly improves the big pg_dump TAP tests.  There's probably more which
> > >> can be done, but I expect people will be much happier with this.  The
> > >> cliff-notes are:
> > >
> > > Attached is an updated patch which applies cleanly against current
> > > master.  I've not yet looked into back-patching these changes, but will
> > > if this can get some review and feedback, and folks like this better and
> > > are ok with the same being done in the back-branches.
> >
> > It would be nice to improve the readability of test_pg_dump's
> > 001_base.pl at the same time by using similarly a hash syntax for common
> > options.
>
> Yes, good point, will fix.

I've updated those tests as well and rebased the patch (though no real
changes were needed for the rebase).  Passes all tests.  I'll take
another look through the changes again but plan to push them in a few
hours, later on this afternoon.

Thanks!

Stephen

On Wed, Apr 04, 2018 at 10:25:03AM -0400, Stephen Frost wrote:
> I've updated those tests as well and rebased the patch (though no real
> changes were needed for the rebase).  Passes all tests.  I'll take
> another look through the changes again but plan to push them in a few
> hours, later on this afternoon.

Okay, I can see that those have been pushed as 446f7f5.
--
Michael