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
