Re: [HACKERS] Patch: Add --no-comments to skip COMMENTs with pg_dump

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Fri, Jan 26, 2018 at 11:56 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >> I think you've chosen a terrible design and ought to throw the whole
> >> thing away and start over.
> >
> > I'll all for throwing away the existing test once we've got something
> > that covers at least what it does (ideally more, of course).
>
> I'm for throwing this away now. It's a nuisance for other people to
> maintain, and as Tom's reply makes clear (and it matches my
> suspicions), they are maintaining it without really knowing whether
> the updates are making are *correct*, just knowing that they *make the
> tests pass*. It's nice to make things turn green on the code coverage
> report, but if we're not really verifying that the results are
> correct, we're just kidding ourselves. We'd get the same amount of
> green on the code coverage report by running the pg_dump commands and
> sending the output to /dev/null, and it would be a lot less work to
> keep up to date.

There's not much to argue about if committers are simply hacking away at
it without actually considering if the test are correct or not. I
suspect it's not quite as bad as being outright wrong- if individuals
are at least testing their changes thoroughly first and then making sure
that the tests pass then it's at least more likely that what's recorded
in the test suite is actually correct, but that's not great. Further,
having contributors include updates to the test suite which are then
willfully thrown away is outright bad.

If that's worse than not having this test coverage for pg_dump, I'm not
sure. What strikes me as likely is that removing this test from pg_dump
will just result in bugs being introduced because, for as much as
updating these tests are painful, actually testing all of the different
variations of pg_dump by hand for even a simple change is quite a bit of
work too. I don't have any doubt that the reason bugs were found with
this is because there were permutations of pg_dump that weren't being
tested, either by individuals making the change or through the other
regression tests we have. Indeed, the tests included were specifically
to get code coverage of cases which weren't already being tested.

> I'm glad this helped you find some bugs. It is only worth keeping if
> it prevents other hackers from introducing bugs in the future. I
> doubt that it will have that effect.

I'm not convinced that it won't, but I agree that we want something that
everyone will feel comfortable in maintaining and improving moving
forward. I'll work on it and either submit a rework to make it easier
to maintain or a patch to remove it before the next CF.

Thanks!

Stephen