Re: psql - improve test coverage from 41% to 88%

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Fabien COELHO <fabien(dot)coelho(at)mines-paristech(dot)fr>
Cc: vignesh C <vignesh21(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: psql - improve test coverage from 41% to 88%
Date: 2019-09-13 12:46:57
Message-ID: 20190913124657.GA6508@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2019-Sep-13, Fabien COELHO wrote:

> Hello Alvaro,
>
> > I think the TestLib.pm changes should be done separately, not together
> > with the rest of the hacking in this patch.
> >
> > Mostly, because I think they're going to cause trouble. Adding a
> > parameter in the middle of the list may cause trouble for third-party
> > users of TestLib.
>
> That is also what I thought, however, see below.

I see. But you seem to have skipped my suggestion without considering
it.

I think the current API of these functions where they just receive a
plain array of arguments, and all callers have to be patched in unison,
is not very convenient. Also, I *think* your new icommand_checks method
is the same as command_checks_all, except that you also have the "init"
part. So you're duplicating code because the original doesn't have
functionality you need? But why do that, if you could have *one*
function that does both things? If some callers don't have the "init"
part, just omit it from the parameters.

(Whether it's implemented using Expect or not should not matter. Either
Expect works everywhere, and we can use it, or it doesn't and we can't.)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2019-09-13 12:51:30 Re: [PATCH] Speedup truncates of relation forks
Previous Message Robert Haas 2019-09-13 12:14:03 Re: Leakproofness of texteq()/textne()