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
