Re: [COMMITTERS] pgsql: Add TAP tests for pg_dump

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