On Mon, Aug 10, 2015 at 07:02:17AM +0100, Simon Riggs wrote:
> Almost every patch I review has either zero or insufficient tests.
>
> If we care about robustness, then we must discuss tests. Here are my two
> recent experiences:
>
> I agree we could do with x10 as many tests, but that doesn't mean all tests
> make sense, so there needs to be some limiting principles to avoid adding
> pointless test cases. I recently objected to adding a test case to
> Isolation tester for the latest ALTER TABLE patch because in that case
> there is no concurrent behavior to test. There is already a regression test
> that tests lock levels for those statements, so in my view it is more
> sensible to modify the existing test than to add a whole new isolation test
> that does nothing more than demonstrate the lock levels work as described,
> which we already knew.
Committers press authors to delete tests more often than we press them to
resubmit with more tests. No wonder so many patches have insufficient tests;
we treat those patches more favorably, on average. I have no objective
principles for determining whether a test is pointlessly redundant, but I
think the principles should become roughly 10x more permissive than the
(unspecified) ones we've been using.
> On another review I suggested we add a function to core to allow it to be
> used in regression tests. A long debate ensued, deciding that we must be
> consistent and put diagnostic functions in contrib. My understanding is
> that we are not allowed to write regression tests that use contrib modules,
> yet the consistent place to put diagnostic functions is contrib -
> therefore, we are never allowed to write tests utilizing diagnostic
> functions. We are allowed to put modules for testing in another directory,
> but these are not distributed to users so cannot be used for production
> diagnosis. Systemic fail, advice needed.
If you want a user-visible function for production diagnosis, set aside the
fact that you wish to use it in a test, and find the best place for it. Then,
place the tests with the function. (That is, if the function belongs in
contrib for other reasons, put tests calling it in the contrib module itself.)
Place non-user-visible test support functions in regress.c, or use one of the
options Robert described.
Thanks,
nm
