Hello Tom,
I took a look at this. I have no quibble with the proposed feature,
and the implementation is certainly simple enough. But I'm unconvinced
about the proposed test scaffolding. Spinning up a new PG instance is a
*hell* of a lot of overhead to pay for testing something that could be
tested as per attached.
Admittedly, the attached doesn't positively prove which pipe each output
string went down, but that does not strike me as a concern large enough
to justify adding a TAP test for.
Sure.
The point is that there would be at least *one* TAP tests so that many
other features of psql, although not all, can be tested. I have been
reviewing quite a few patches without tests because of this lack of
infrastructure, and no one patch is ever going to justify a TAP test on
its own. It has to start somewhere. Currently psql coverage is abysmal,
around 40% of lines & functions are called by the whole non regression
tests, despite the hundreds of psql-relying tests. Pg is around 80%
coverage overall.
Basically, I really thing that one psql dedicated TAP test should be
added, not for \warn per se, but for other features.
I'd be happier about adding TAP infrastructure if it looked like it'd
be usable to test some of the psql areas that are unreachable by the
existing test methodology, particularly tab-complete.c and prompt.c.
But I don't see anything here that looks like it'll work for that.
The tab complete and prompt are special interactive cases and probably
require special infrastructure to make a test believe it is running
against a tty while it is not. The point of this proposal is not to
address these special needs, but to lay a basic infra.
I don't like what you did to command_checks_all,
Yeah, probably my fault, not David.
either --- it could hardly say "bolted on after the fact" more clearly
if you'd written that in <blink><red> text. If we need an input-stream
argument, let's just add it in a rational place and adjust the callers.
There aren't that many of 'em, nor has the subroutine been around all
that long.
I wanted to avoid breaking the function signature of it is used by some
external packages. Not caring is an option.
--
Fabien.
