Обсуждение: Re: [COMMITTERS] pgsql: Add TAP tests for pg_dump

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

http://www.postgresql.org/message-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

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

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

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