pgsql: Add TAP tests for pg_dump

* Stephen Frost (sfrost@snowman.net) wrote:

src/test/modules/test_pg_dump/Makefile | 25 +
src/test/modules/test_pg_dump/README | 2 +
.../modules/test_pg_dump/expected/test_pg_dump.out | 6 +
src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 +
src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++
.../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 +
src/test/modules/test_pg_dump/test_pg_dump.control | 3 +

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Thanks!

Stephen

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

These are just new tests..? I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

We back-patch new tests from time to time too, when they're associated
with bug fixes, so I'm pretty confused why TAP tests would be an issue
to add on HEAD post feature-freeze.

If the consensus is that we shouldn't add new tests during feature
freeze, I'll revert the patch that added them and add them later, but,
for my 2c at least, I think we should be happy to add these even after
feature freeze.

Thanks!

Stephen

On 2016-05-06 15:11:53 -0400, Stephen Frost wrote:

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

These are just new tests..? I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

We back-patch new tests from time to time too, when they're associated
with bug fixes, so I'm pretty confused why TAP tests would be an issue
to add on HEAD post feature-freeze.

If the consensus is that we shouldn't add new tests during feature
freeze, I'll revert the patch that added them and add them later, but,
for my 2c at least, I think we should be happy to add these even after
feature freeze.

+1

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On Fri, May 6, 2016 at 3:14 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-05-06 15:11:53 -0400, Stephen Frost wrote:

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 5/6/16 2:06 PM, Stephen Frost wrote:

Add TAP tests for pg_dump

I'd be the first to welcome this, but what happened to feature freeze?

These are just new tests..? I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

We back-patch new tests from time to time too, when they're associated
with bug fixes, so I'm pretty confused why TAP tests would be an issue
to add on HEAD post feature-freeze.

If the consensus is that we shouldn't add new tests during feature
freeze, I'll revert the patch that added them and add them later, but,
for my 2c at least, I think we should be happy to add these even after
feature freeze.

+1

I think it's appropriate at least in this case. It seems to be
related to Stephen wanting to make sure he found all of the bugs he
introduced in pre-freeze commits, which I think can be included under
an expansive definition of a mop-up commit. I think there's some
limit to what we ought to add to the 9.6 testing infrastructure at
this late date, but I'm not inclined to fight about this one.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Stephen Frost (sfrost@snowman.net) wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

src/test/modules/test_pg_dump/Makefile | 25 +
src/test/modules/test_pg_dump/README | 2 +
.../modules/test_pg_dump/expected/test_pg_dump.out | 6 +
src/test/modules/test_pg_dump/sql/test_pg_dump.sql | 1 +
src/test/modules/test_pg_dump/t/001_base.pl | 535 ++++
.../modules/test_pg_dump/test_pg_dump--1.0.sql | 15 +
src/test/modules/test_pg_dump/test_pg_dump.control | 3 +

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

Thanks!

Stephen

* Stephen Frost (sfrost@snowman.net) wrote:

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

It looks like there's an exclude list that intagg is on for the MSVC
build animals, presumably because it doesn't have a .c file to be
compiled.

I'm guessing the right answer here is to just add test_pg_dump to that
list.

If there aren't any other suggestions, I'll go ahead and do that.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

test_extensions is also included in the "contrib_excludes" list in
src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
thinking that's what is needed.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

Thanks!

Stephen

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

test_extensions is also included in the "contrib_excludes" list in
src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
thinking that's what is needed.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Robert Haas (robertmhaas@gmail.com) wrote:

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Stephen Frost (sfrost@snowman.net) wrote:

Looks like the test_pg_dump extension made the Windows builds upset.
I'm guessing that's because I set 'MODULES_big' even though there isn't
a .c component.

Doing a local build with that commented out, assuming that works and
doesn't generate the .so any more on my Linux box, I'll push the change
up to hopefully fix those buildfarm members.

Alright, apparently that made other Windows buildfarm members unhappy...

I guess the next approach will be to add back MODULES_big and add in a
.c file for the Windows systems to be happy about. I'm certainly open
to other suggestions.

You should not need to do that; cf src/test/modules/test_extensions,
which has got SQL-only extensions.

test_extensions is also included in the "contrib_excludes" list in
src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
thinking that's what is needed.

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

Alright, I'll revert the TAP tests and we can discuss what to do next
post-beta1. At least a few runs got in and looked clean, other than the
Windows issue with the extension building.

Thanks!

Stephen

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

Yes. I have no objection to adding more test cases post-beta1, but
I'd like to have the buildfarm green through the weekend. This isn't
the best time to be debugging-by-buildfarm.

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it. But if it doesn't, it's time to cut our
losses.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

But at this point I think Peter's complaint has some force to it, and that
what you ought to do is revert the testing patch. You can have another go
after beta1.

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta. We can iron out what
else to do later.

Yes. I have no objection to adding more test cases post-beta1, but
I'd like to have the buildfarm green through the weekend. This isn't
the best time to be debugging-by-buildfarm.

Understood.

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it. But if it doesn't, it's time to cut our
losses.

Ok, I like that quite a bit better and will give it a shot, but if it
doesn't work, I'll revert it immediately.

Thanks!

Stephen

Stephen Frost wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

It looks like there's an exclude list that intagg is on for the MSVC
build animals, presumably because it doesn't have a .c file to be
compiled.

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows. I agree with reverting now and figuring out a way
to enable this test on Windows after the beta. I suggest you grab hold
of your own Windows installation so that you can whack it locally until
it works, rather than waiting for buildfarm cycles.

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:

Stephen Frost wrote:

* Stephen Frost (sfrost@snowman.net) wrote:

The intagg contrib modules doesn't have a .c file either, nor a
MODULES_big line, and I don't see any errors with it. I'm not terribly
familiar with how the Windows builds are run though.

It looks like there's an exclude list that intagg is on for the MSVC
build animals, presumably because it doesn't have a .c file to be
compiled.

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows. I agree with reverting now and figuring out a way
to enable this test on Windows after the beta. I suggest you grab hold
of your own Windows installation so that you can whack it locally until
it works, rather than waiting for buildfarm cycles.

This is only for the test_pg_dump extension, which is specifically just
testing pg_dump with extensions. The vast majority of the pg_dump tests
are in src/bin/pg_dump and should be getting run on the Windows systems
(once this issue is addressed).

Thanks!

Stephen

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stephen Frost wrote:

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows.

No, that's incorrect: @contrib_excludes stops the MSVC stuff from trying
to *build* in that directory, which is fine because there's nothing to
build. It doesn't prevent running the test case. See eg.
http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=thrips&amp;dt=2016-05-06%2015%3A37%3A27&amp;stg=testmodules-install-check-C
where the test_extensions test does get run even though it's in
@contrib_excludes.

In any case, reverting the patch would mean no Windows testing, so
your argument doesn't seem to have much force even if it were true?

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Tom Lane wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stephen Frost wrote:

I'm guessing the right answer here is to just add test_pg_dump to that
list.

I don't like this solution, because it means pg_dump will get little
testing on Windows.

No, that's incorrect: @contrib_excludes stops the MSVC stuff from trying
to *build* in that directory, which is fine because there's nothing to
build. It doesn't prevent running the test case. See eg.
http://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=thrips&amp;dt=2016-05-06%2015%3A37%3A27&amp;stg=testmodules-install-check-C
where the test_extensions test does get run even though it's in
@contrib_excludes.

Ah, that's surprising, but much better of course. Works for me.

In any case, reverting the patch would mean no Windows testing, so
your argument doesn't seem to have much force even if it were true?

I was suggesting to revert it for the beta and put it back with a
working Windows implementation after the beta, before GA. I was afraid
that a hack to hide the test from Windows would become permanent.

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

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it. But if it doesn't, it's time to cut our
losses.

Alright, it certainly *appears* to be working. All of the Windows
buildfarm animals which have reported back since I added test_pg_dump to
that list have been green. There are still a few boxes outstanding, but
that seems like a good sign too- they failed pretty quickly before.

I'm saying all this because I have to step out for a couple of hours.
I'll keep an eye on it and will be around later tonight to revert the
tests if necessary, but I'm not going to complain if someone else
chooses to do so if those last few Windows boxes come back red again
before I'm around again.

Thanks!

Stephen

On 5/6/16 4:07 PM, Stephen Frost wrote:

Are you suggesting commiting to still-9.6-HEAD post-beta1? I took
Peter's comment as suggesting that adding the tests would have to wait
til after we branched 9.6, as we do for features.

I'd really like to have these tests included as that will make them
available to others more easily to add on to, and I'm certainly planning
to continue adding tests until I get pg_dump.c's coverage a lot better.
That seems like the perfect kind of effort that should be happening
right now- adding more tests and working to make sure that what's been
committed is correct (and fixing it when it isn't, as discovered by the
test suite with transforms and casts...).

Well, we all want to get our code in and let others make it better. But
here we are trying to get a release out in a few days with only a
weekend in between, and we are still trying to figure out basic
portability issues using build-farm-whacking technology. This is not
the time.

Again, I applaud this, but this is clearly a new major chunk of mostly
not-peer-reviewed code with potential for portability problems and the
possibility to break downstream packaging routines. There is no harm in
leaving this for 9.7.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

On 5/6/16 3:11 PM, Stephen Frost wrote:

These are just new tests..?

This is a matter of degree, but I think there is a difference between
new test cases and a whole new test suite.

I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

I didn't see that discussion and I still can't find it.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

Again, I applaud this, but this is clearly a new major chunk of mostly
not-peer-reviewed code with potential for portability problems and the
possibility to break downstream packaging routines. There is no harm in
leaving this for 9.7.

FWIW, I was and remain perfectly on board with putting this into 9.6;
adding new tests seems to me to be very much within the charter of beta
test period. My only gripe was that shoving it in so soon before beta1
wrap is risky. But it looks from the current buildfarm results that
Stephen has gotten away with that. If we see any more buildfarm failures
over the weekend, I'd counsel a temporary revert, but if we don't I think
it's OK to ship beta1 with these tests. In either case I'd aim to have
them in place in beta2.

regards, tom lane

--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 5/6/16 3:11 PM, Stephen Frost wrote:

These are just new tests..?

This is a matter of degree, but I think there is a difference
between new test cases and a whole new test suite.

To be clear, I've been calling it a 'test suite' because there ended up
being over 1000 tests added, but it's all using the exinsting
infrastructure of the TAP testing system, just as we have for other
commands (pg_ctl, initdb, etc).

I assumed that would be welcome during post
feature-freeze, and certainly no one raised any concerns about adding
these tests during the discussion prior to my commiting them.

I didn't see that discussion and I still can't find it.

The new tests were discussed on the thread related to the changes to
pg_dump. The first patch, which included just 300 or so tests, was
here:

/messages/by-id/20160425043909.GW10850@tamriel.snowman.net

There was a bit of back-and-forth on that thread and new patches were
posted there, as well as indication that I was planning to push them a
couple days before I did.

Honestly, over the next couple of months between feature-freeze and
release, I'd like to add even more tests, and not just to pg_dump but
also to other commands that don't have very good testing today (psql, in
particular, but pg_dumpall needs more also, and there's more to do with
pg_dump too).

I certainly understand the concern raised about making the buildfarm go
red right before a release, though I believe that's now been dealt with.
When it comes to packaging, if adding tests using the existing test
infrastructure (the TAP system) causes a problem there, then I think we
need to address that issue rather than not add new tests. Packagers
also always have the option to not enable the tap tests, if there really
is an issue there which can't be addressed.

Thanks!

Stephen

On 5/7/16 9:36 AM, Stephen Frost wrote:

Honestly, over the next couple of months between feature-freeze and
release, I'd like to add even more tests, and not just to pg_dump but
also to other commands that don't have very good testing today (psql, in
particular, but pg_dumpall needs more also, and there's more to do with
pg_dump too).

If we're going to do that, there will be no stopping it. I also have a
bunch of code in this area lined up, but I was hoping to submit it to
the commit fest process at the right time and order to get feedback and
testing. If we're going to allow new tests being developed right up
until release, I can just stop doing release preparation work right now
and get back to coding and reviewing.

Ultimately, tests are code and should be treated as such.

When it comes to packaging, if adding tests using the existing test
infrastructure (the TAP system) causes a problem there, then I think we
need to address that issue rather than not add new tests. Packagers
also always have the option to not enable the tap tests, if there really
is an issue there which can't be addressed.

Well, that's like saying, if the feature I just committed the night
before the release turns out to be problematic, you can just ifdef it
out. I don't think we want that, and I think it simplifies the way the
world works. Because new code interacts with old code, and then there
will be even more new code which interacts with other code, and so it
becomes harder and harder to isolate issues. Yeah, if adding more tests
causes instabilities because of issues not related to the tests
themselves, we should fix that. But that doesn't mean we should
hand-wave past the fact that that potential exists. That's software
development. If everything were perfect, we wouldn't need a beta period
at all.

The configure flag to disable TAP tests isn't there so that we get
license to play with them at random, it's because we wanted to make the
software dependency optional. The psql tests run under pg_regress, so
they can't be made optional anyway.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:

On 5/7/16 9:36 AM, Stephen Frost wrote:

Honestly, over the next couple of months between feature-freeze and
release, I'd like to add even more tests, and not just to pg_dump but
also to other commands that don't have very good testing today (psql, in
particular, but pg_dumpall needs more also, and there's more to do with
pg_dump too).

If we're going to do that, there will be no stopping it. I also
have a bunch of code in this area lined up, but I was hoping to
submit it to the commit fest process at the right time and order to
get feedback and testing. If we're going to allow new tests being
developed right up until release, I can just stop doing release
preparation work right now and get back to coding and reviewing.

I do think that now is a good time for people to be reviewing what's
been committed, which includes writing tests (either automated ones,
which can be included in our test suite, or one-off's for testing
specific things but which don't make sense to include).

That doesn't mean we should be codeing or reviewing new features for
commit.

As for release prep, I certainly applaud everyone involved in that
effort as we do have the beta release and back-branch releases coming
up.

Regarding when we should stop adding new tests, I can't agree with the
notion that they should be treated as new features. New tests may break
the buildfarm, but they do not impact the stability of committed code,
nor do they introduce new bugs into the code which has been committed
(if anything, they expose committed bugs, as the set of tests we're
discussing did, which should then be fixed).

Ultimately, tests are code and should be treated as such.

Perhaps in some manners this is accurate, but I'd view our test suite as
an independent code base from PG. Bugs in the test suite might cause
false test failures or other issues on the buildfarm or during
packaging, but bugs or issues in the test suite won't cause data loss,
data corruption, or generally impact running operations of our users.

I can see some sense in holding off on adding more tests once we hit RC,
as we want anything done with RC to be essentially identical to the
release, unless there is a serious issue, but holding off on adding new
tests which could expose issues in committed code for the months during
beta doesn't seem sensible.

As such, I'd certainly encourage you to propose new tests, even now, but
not changes to the PG code, except for bug fixes, or changes agreed to
by the RMT. How you prioritise that with the release preparation work
is up to you, of course, though if I were in your shoes, I'd take care
of the release prep first, as we're quite close to doing a set of
releases. I'm happy to try and help with that effort myself, though
I've not been involved very much in release prep and am not entirely
sure what I can do to help.

Thanks!

Stephen

On Sun, May 8, 2016 at 6:44 AM, Stephen Frost <sfrost@snowman.net> wrote:

I do think that now is a good time for people to be reviewing what's
been committed, which includes writing tests (either automated ones,
which can be included in our test suite, or one-off's for testing
specific things but which don't make sense to include).

And what if the tests are buggy? New test suites should go through a
CF first I think for proper review. And this is clearly one.

Regarding when we should stop adding new tests, I can't agree with the
notion that they should be treated as new features. New tests may break
the buildfarm, but they do not impact the stability of committed code,
nor do they introduce new bugs into the code which has been committed
(if anything, they expose committed bugs, as the set of tests we're
discussing did, which should then be fixed).

Yes, new tests do not impact the core code, but they could hide
existing bugs, which is what I think Peter is arguing about here, and
why it is not a good idea to pushed in a complete new test suite just
before a beta release. The buildfarm is made so as a run stops once of
the tests turns red, not after all of them have run.

As such, I'd certainly encourage you to propose new tests, even now, but
not changes to the PG code, except for bug fixes, or changes agreed to
by the RMT. How you prioritise that with the release preparation work
is up to you, of course, though if I were in your shoes, I'd take care
of the release prep first, as we're quite close to doing a set of
releases. I'm happy to try and help with that effort myself, though
I've not been involved very much in release prep and am not entirely
sure what I can do to help.

Adding new tests that are part of an existing bug fix is fine because
the failure of such a test would mean that the bug fix is not working
as intended. Based on my reading of the code this adds test coverage
that is larger than the basic test for a bug fix.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Sun, May 8, 2016 at 6:44 AM, Stephen Frost <sfrost@snowman.net> wrote:

I do think that now is a good time for people to be reviewing what's
been committed, which includes writing tests (either automated ones,
which can be included in our test suite, or one-off's for testing
specific things but which don't make sense to include).

And what if the tests are buggy? New test suites should go through a
CF first I think for proper review. And this is clearly one.

They still won't result in data loss, corruption, or other direct impact
on our users, even if they are buggy. They also won't destablize the
code base or introduce new bugs into the code. Those are the things we
need to be concerned about when it comes to introducing new features
into the system and why we take a break from doing so before a release.

Regarding when we should stop adding new tests, I can't agree with the
notion that they should be treated as new features. New tests may break
the buildfarm, but they do not impact the stability of committed code,
nor do they introduce new bugs into the code which has been committed
(if anything, they expose committed bugs, as the set of tests we're
discussing did, which should then be fixed).

Yes, new tests do not impact the core code, but they could hide
existing bugs, which is what I think Peter is arguing about here, and
why it is not a good idea to pushed in a complete new test suite just
before a beta release. The buildfarm is made so as a run stops once of
the tests turns red, not after all of them have run.

I disagree that new tests are going to hide existing bugs. The case
which I believe you're making for that happening is where the buildfarm
is red for an extended period of time, but I've agreed that we don't
want the buildfarm to be red and have said as much, and that's different
from the question about adding new tests and simply means the same thing
it's always meant- anything which makes the buildfarm red should be
addressed in short order.

As such, I'd certainly encourage you to propose new tests, even now, but
not changes to the PG code, except for bug fixes, or changes agreed to
by the RMT. How you prioritise that with the release preparation work
is up to you, of course, though if I were in your shoes, I'd take care
of the release prep first, as we're quite close to doing a set of
releases. I'm happy to try and help with that effort myself, though
I've not been involved very much in release prep and am not entirely
sure what I can do to help.

Adding new tests that are part of an existing bug fix is fine because
the failure of such a test would mean that the bug fix is not working
as intended. Based on my reading of the code this adds test coverage
that is larger than the basic test for a bug fix.

Yes, which is an all-together good thing, I believe. We need more and
better test coverage of a *lot* of the system and this is just the start
of adding that coverage for pg_dump.

There is pretty clearly a lack of consensus on the question about adding
new tests. It seems like it'd be a good topic for the dev meeting, but
I don't think everyone involved in the discussion so far is going to be
there, unfortunately. I'm open to still proposing it as a topic, though
I dislike having to leave people out of it.

I'm not quite sure what the best way to put this is, but I can't help
but feel the need to comment on it in at least some way:

What *should* we be doing right now? Reviewing code and testing the
system is what I had thought but perhaps I'm wrong. To the extent that
we're testing the system, isn't it better to write repeatable tests,
particularly ones which add code coverage, than one-off tests that only
check that the current snapshot of the code is operating correctly? I
feel like that's what we should be encouraging, but this position is
suggesting that we should be doing that independently and holding all of
those new tests until the next commitfest, which is the same as we're
asked to do for new development at this point. Perhaps it's not ideal,
but I feel it's necessary to point out that between writing code
coverage tests and doing new development, other things being equal, you
can guess which would be preferred. I'd rather we encourage more of the
former rather than acting as if they should be treated the same during
this phase of the development cycle.

Thanks!

Stephen

On 5/9/16, Stephen Frost <sfrost@snowman.net> wrote:

And what if the tests are buggy? New test suites should go through a
CF first I think for proper review. And this is clearly one.

They still won't result in data loss, corruption, or other direct impact
on our users, even if they are buggy. They also won't destablize the
code base or introduce new bugs into the code.

Yes, but they could make things worse long term. For example if
introduce intermittent failures or if they're hard to understand or if
they test internals too deeply and don't let those internals evolve
because the tests break. All of them reasons why it's good that
they're reviewed.

There is pretty clearly a lack of consensus on the question about adding
new tests.

What *should* we be doing right now? Reviewing code and testing the
system is what I had thought but perhaps I'm wrong. To the extent that
we're testing the system, isn't it better to write repeatable tests,
particularly ones which add code coverage, than one-off tests that only
check that the current snapshot of the code is operating correctly?

I also think that testing *and* keeping those tests for the future is
more valuable than just testing. But committers can just push their
tests while non committers need to wait for the next CF to get their
new tests committed. And committers pushing tests means they bypass
the review process which, as far as I know, doesn't happen for regular
patches: committers usually only directly commit small fixes not lots
of new (test) code.

So, crazy idea: what about test only CommitFests in between now and
the release? The rules would be: only new tests and existing test
improvements are allowed. For the rest, the CF would be the same as
usual: have an end date, committers agree to go through each patch and
commit or return it with feedback etc. Stephen would have added his
tests to the upcoming June test CF, Michael would have reviewed them
and maybe add his own and so on.

This does create work for committers (go through the submitted
patches) but acts as an incentive for test writers. Want your test
code committed? Well, there will be a CF very soon where it will get
committer attention so better write that test. It also gives a clear
date after which test churn also stops in order to not destabilize the
buildfarm right before a release.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Catalin,

* Catalin Iacob (iacobcatalin@gmail.com) wrote:

On 5/9/16, Stephen Frost <sfrost@snowman.net> wrote:

And what if the tests are buggy? New test suites should go through a
CF first I think for proper review. And this is clearly one.

They still won't result in data loss, corruption, or other direct impact
on our users, even if they are buggy. They also won't destablize the
code base or introduce new bugs into the code.

Yes, but they could make things worse long term. For example if
introduce intermittent failures or if they're hard to understand or if
they test internals too deeply and don't let those internals evolve
because the tests break. All of them reasons why it's good that
they're reviewed.

I agree that such tests could be bad but I'm unconvinced that's a
justification for having to have all new testing go through the CF
process.

Further, this test framework was under discussion on-list and commented
on by at least one other committer prior to being committed. It was not
entirely without review.

There is pretty clearly a lack of consensus on the question about adding
new tests.

What *should* we be doing right now? Reviewing code and testing the
system is what I had thought but perhaps I'm wrong. To the extent that
we're testing the system, isn't it better to write repeatable tests,
particularly ones which add code coverage, than one-off tests that only
check that the current snapshot of the code is operating correctly?

I also think that testing *and* keeping those tests for the future is
more valuable than just testing. But committers can just push their
tests while non committers need to wait for the next CF to get their
new tests committed.

I'd actually love to have non-committers reviewing and testing code and
submitting their tests for review and commit by a committer. This
entire discussion is about when it's appropriate to do that and I'm
trying to argue that now is the right time for that review and testing
to happen.

If the consensus is that new tests are good and that we can add them
during this period of development, then I don't see why that would be
any different for committers vs. non-committers. We certainly don't
require that non-committers go through the CF process for bug fixes
(although we encourage them to add it to the CF if it isn't picked up
immediately, to ensure it isn't forgotten).

Honestly, this argument appears to be primairly made on the basis of
"committers shouldn't get to do things differently from non-committers"
and, further, that the CF process should be an all-encompassing
bureaucratic process for every commit that goes into PG simply because
we have a CF system. I don't agree with either of those.

And committers pushing tests means they bypass
the review process which, as far as I know, doesn't happen for regular
patches: committers usually only directly commit small fixes not lots
of new (test) code.

As mentioned, the test suite wasn't simply pushed without any review or
discussion. That didn't happen through the CF process, but that doesn't
mean it didn't happen.

So, crazy idea: what about test only CommitFests in between now and
the release? The rules would be: only new tests and existing test
improvements are allowed. For the rest, the CF would be the same as
usual: have an end date, committers agree to go through each patch and
commit or return it with feedback etc. Stephen would have added his
tests to the upcoming June test CF, Michael would have reviewed them
and maybe add his own and so on.

If having a process for adding tests would be beneficial then I'm happy
to support it. I agree that having specific goals for both committers
and non-committers during this time in the development cycle is a good
thing and having a "test-only" or perhaps "review/test-only"
commitfest sounds like it would encourage those as goals for this
period.

This does create work for committers (go through the submitted
patches) but acts as an incentive for test writers. Want your test
code committed? Well, there will be a CF very soon where it will get
committer attention so better write that test. It also gives a clear
date after which test churn also stops in order to not destabilize the
buildfarm right before a release.

I don't have any issue with that and would be happy to support such an
approach, as a committer.

Thanks!

Stephen

On Mon, May 9, 2016 at 11:58 AM, Stephen Frost <sfrost@snowman.net> wrote:

Further, this test framework was under discussion on-list and commented
on by at least one other committer prior to being committed. It was not
entirely without review.

No, just almost entirely without review.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers