Re: Refactor SSL test framework to support multiple TLS libraries

On 2021-Mar-30, Michael Paquier wrote:

> On Tue, Mar 30, 2021 at 03:50:28PM +0900, Michael Paquier wrote:
> > The test_*() ones are just wrappers for psql able to use a customized
> > connection string.  It seems to me that it would make sense to move
> > those two into PostgresNode::psql itself and extend it to be able to
> > handle custom connection strings?
> 
> Looking at this part, I think that this is a win in terms of future
> changes for SSLServer.pm as it would become a facility only in charge
> of managing the backend's SSL configuration.  This has also the
> advantage to make the error handling with psql more consistent with
> the other tests.
> 
> So, attached is a patch to do this simplification.  The bulk of the
> changes is within the tests themselves to adapt to the merge of
> $common_connstr and $connstr for the new routines of PostgresNode.pm,
> and I have done things this way to ease the patch lookup.  Thoughts?

I agree this seems a win.

The only complain I have is that "the given node" is nonsensical in
PostgresNode.  I suggest to delete the word "given".  Also "This is
expected to fail with a message that matches the regular expression
$expected_stderr".

The POD doc for connect_fails uses order: ($connstr, $testname, $expected_stderr)
but the routine has:
  +   my ($self, $connstr, $expected_stderr, $testname) = @_;

these should match.

(There's quite an inconsistency in the existing test code about
expected_stderr being a string or a regex; and some regexes are quite
generic: just qr/SSL error/.  Not this patch responsibility to fix that.)

As I understand, our perlcriticrc no longer requires 'return' at the end
of routines (commit 0516f94d18c5), so you can omit that.

-- 
Álvaro Herrera       Valdivia, Chile