Re: [PATCH v4] Add \warn to psql

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.