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.
