Re: [HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Stephen Frost]

Tom,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> New tests are not zero-cost; they create a distributed burden on the
> buildfarm and, by increasing the buildfarm cycle time, slow down feedback
> to authors of subsequent patches.  So I'm very much not on board with
> any argument that "more tests are always better and don't even require
> discussion".

I agree with that and certainly considered it while working on these
added tests.

> I'd have liked to see this patch posted with some commentary along the
> lines of "this improves LOC coverage in pg_dump by X%, and for me it
> increases the time taken for 'make installcheck' in bin/pg_dump by Y%".
> Assuming Y isn't totally out of line with X, I doubt anyone would have
> objected or even bothered to review the patch in detail ... but it would
> have been polite to proceed that way.

About 8% increased LOC coverage for pg_dump.c (which isn't small when
you consider how large that file is).  The additional time seemed to be
on the 5-6s range, moving the test from 35s to 40s or so.

> In short, I agree with Stephen's position that test additions can get
> away with less review than other sorts of changes, but I also agree with
> Robert's position that that doesn't mean there's no process to follow
> at all.

Fair enough.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Tom Lane]

Andres Freund  writes:
> On 2017-03-20 10:35:15 -0400, Stephen Frost wrote:
>> I continue to be of the opinion that this entire discussion is quite
>> flipped from how we really should be running things- adding regression
>> tests to improve code coverage, particularly when they're simply adding
>> to the existing structure for those tests, should be strongly encouraged 
>> both before and after feature-freeze.

> I don't think posting a preliminary patch, while continuing to polish,
> with a note that you're working on that and plan to commit soon, would
> slow you down that much.  There's pretty obviously a difference between
> an added 10 line test, taking 30ms, and what you did here - and that
> doesn't mean what you added is wrong or shouldn't be added.

New tests are not zero-cost; they create a distributed burden on the
buildfarm and, by increasing the buildfarm cycle time, slow down feedback
to authors of subsequent patches.  So I'm very much not on board with
any argument that "more tests are always better and don't even require
discussion".

I'd have liked to see this patch posted with some commentary along the
lines of "this improves LOC coverage in pg_dump by X%, and for me it
increases the time taken for 'make installcheck' in bin/pg_dump by Y%".
Assuming Y isn't totally out of line with X, I doubt anyone would have
objected or even bothered to review the patch in detail ... but it would
have been polite to proceed that way.

In short, I agree with Stephen's position that test additions can get
away with less review than other sorts of changes, but I also agree with
Robert's position that that doesn't mean there's no process to follow
at all.

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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Andres Freund]

On 2017-03-20 10:35:15 -0400, Stephen Frost wrote:
> Robert,
> 
> * Robert Haas (robertmh...@gmail.com) wrote:
> > I'm glad that you are working on fixing
> > pg_dump bugs and improving test coverage, but my gladness about that
> > does not extend to thinking that the processes which other people
> > follow for their work should be waived for yours.  Sorry.
> 
> To be clear, I am not asking for any kind of special exception for
> myself.
> 
> I continue to be of the opinion that this entire discussion is quite
> flipped from how we really should be running things- adding regression
> tests to improve code coverage, particularly when they're simply adding
> to the existing structure for those tests, should be strongly encouraged 
> both before and after feature-freeze.

I don't think posting a preliminary patch, while continuing to polish,
with a note that you're working on that and plan to commit soon, would
slow you down that much.  There's pretty obviously a difference between
an added 10 line test, taking 30ms, and what you did here - and that
doesn't mean what you added is wrong or shouldn't be added.

And I don't think that expectation has anything to do with being
anti-test.

Greetings,

Andres Freund


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

[HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> I'm glad that you are working on fixing
> pg_dump bugs and improving test coverage, but my gladness about that
> does not extend to thinking that the processes which other people
> follow for their work should be waived for yours.  Sorry.

To be clear, I am not asking for any kind of special exception for
myself.

I continue to be of the opinion that this entire discussion is quite
flipped from how we really should be running things- adding regression
tests to improve code coverage, particularly when they're simply adding
to the existing structure for those tests, should be strongly encouraged 
both before and after feature-freeze.

Thanks.

Stephen


signature.asc
Description: Digital signature

[HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Mar 20, 2017 at 8:33 AM, Stephen Frost  wrote:
> > * Robert Haas (robertmh...@gmail.com) wrote:
> >> So was this 3340 line patch posted or discussed anyplace before it got
> >> committed?
> >
> > I've mentioned a few times that I'm working on improving pg_dump
> > regression tests and code coverage, which is what these were.  I'm a bit
> > surprised that it's, apparently, a surprise to anyone or that strictly
> > adding regression tests in the existing framework deserves very much
> > discussion.
> 
> I'm not saying it does.  I'm saying that it's polite, and expected, to
> post patches and ask for opinions before committing things.

While I certainly agree with that when it comes to new features, changes
in work-flow, bug fixes and other things, I'm really not sure that
requiring posting to the list and waiting for responses every time
someone wants to add some regression tests is a useful way to spend
time.  While the patch looked big, a lot of that is just that the
current structure requires listing out every 'like' and 'unlike' set for
each test, which adds up.

In this particular case, I've been discussing these pg_dump regression
tests for months, as I've been going through fixing bugs found by them
and back-patching them.  I had time over this weekend to watch the
buildfarm and make sure that it didn't explode (in which case, I would
have reverted the patch immediately, of course).  I would have preferred
to commit these new tests in a more fine-grained fashion, but I kept
finding issues, which meant that commiting them earlier would have just
turned the buildfarm red, which wouldn't have been beneficial to anyone.

I'm quite pleased to see that, for the most part, the tests have been
successful on the buildfarm.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Stephen Frost]

Peter,

* Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> On 3/20/17 08:33, Stephen Frost wrote:
> >> So was this 3340 line patch posted or discussed anyplace before it got
> >> committed?
> > I've mentioned a few times that I'm working on improving pg_dump
> > regression tests and code coverage, which is what these were.  I'm a bit
> > surprised that it's, apparently, a surprise to anyone or that strictly
> > adding regression tests in the existing framework deserves very much
> > discussion.
> 
> I think we had this discussion about adding a large number of (pg_dump)
> tests without discussion or review already about a year ago, so I for
> one am surprised that are still surprised.

The concern you raised at the time, from my recollection, was that I had
added a new set of TAP tests post feature-freeze for pg_dump and there
was concern that it might cause an issue for packagers.

I don't recall a concern being raised about the tests themselves, and I
intentionally worked to make sure this landed before feature-freeze to
avoid that issue, though I believe we should actually be looking to try
to add tests post feature-freeze too, as we work to test things prior to
the release.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Peter Eisentraut]

On 3/20/17 08:33, Stephen Frost wrote:
>> So was this 3340 line patch posted or discussed anyplace before it got
>> committed?
> I've mentioned a few times that I'm working on improving pg_dump
> regression tests and code coverage, which is what these were.  I'm a bit
> surprised that it's, apparently, a surprise to anyone or that strictly
> adding regression tests in the existing framework deserves very much
> discussion.

I think we had this discussion about adding a large number of (pg_dump)
tests without discussion or review already about a year ago, so I for
one am surprised that are still surprised.

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

[HACKERS] Re: [COMMITTERS] pgsql: Improve pg_dump regression tests and code coverage

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> So was this 3340 line patch posted or discussed anyplace before it got
> committed?

I've mentioned a few times that I'm working on improving pg_dump
regression tests and code coverage, which is what these were.  I'm a bit
surprised that it's, apparently, a surprise to anyone or that strictly
adding regression tests in the existing framework deserves very much
discussion.

What I think would be great would be some additional work on our code
coverage, which is abysmal.  This, at least, gets us up over 80% for
src/bin/pg_dump, but there's still quite a bit of work to be done there,
as noted in the commit message, and lots of opportunity for improvement
throughout the rest of the code base, as https://coverage.postgresql.org
shows.

Thanks!

Stephen


signature.asc
Description: Digital signature