Re: ALTER TYPE 0: Introduction; test cases

On Sun, Jan 16, 2011 at 12:07:44PM -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Sat, Jan 15, 2011 at 10:25 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >> Do you value test coverage so little?
>
> > If you're asking whether I think real-world usability is more
> > important than test coverage, then yes.
>
> Quite honestly, I'd be inclined to rip out most of the DEBUG messages I
> see in that regression test altogether. They are useless, and so is the
> regression test itself. An appropriate regression test would involve
> something more like checking that the relfilenode changed and then
> checking that the contained data is still sane.

This patch is the first of a series. Divorced from the other patches, many of
the test cases exercise the same code path, making them redundant. Even so, the
tests revealed a defect we released with 9.0; that seems sufficient to promote
them out of the "useless" bucket.

One can easily confirm by inspection that the relfilenode will change if and
only if the "rewriting" DEBUG message appears. Your proposed direct comparison
of the relfilenode in the regression tests adds negligible sensitivity. If that
were all, I'd call it a question of style. However, a relfilenode comparison
does not distinguish no-op changes from changes entailing a verification scan.
A similar ambiguity would arise without the "foreign key" DEBUG message.

As for "checking that the contained data is still sane", what do you have in
mind? After the test cases, I SELECT the table-under-test and choice catalog
entries. If later patches in the series leave these expected outputs unchanged,
that confirms the continued sanity of the data. Perhaps I should do this after
every test, or also test forced index scans.

nm