On Tue, Mar 30, 2021 at 12:15:07PM -0300, Alvaro Herrera wrote:
> 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".
Your suggestions are better, indeed.
> 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.
Fixed.
> (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.)
Jacob has just raised this as an issue for an integration with NLS,
because it may be possible that things fail with "SSL error" but a
different error pattern, causing false positives:
https://www.postgresql.org/message-id/e0f0484a1815b26bb99ef9ddc7a110dfd6425931.camel@vmware.com
I agree that those matches should be much more picky. We may need to
be careful across all versions of OpenSSL supported though :/
> As I understand, our perlcriticrc no longer requires 'return' at the end
> of routines (commit 0516f94d18c5), so you can omit that.
Fixed. Thanks.
With all the comments addressed, with updates to use a single scalar
for all the connection strings and with a proper indentation, I finish
with the attached. Does that look fine?
--
Michael