Обсуждение: Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
От
Daniel Gustafsson
Дата:
The SSL tests for pg_ctl restarts with an incorrect key passphrase run pg_ctl manually and use the internal method _update_pid to set the server PID file accordingly. This is needed since $node->restart will BAIL in case the restart fails, which clearly isn't useful to anyone wanting to test restarts. This is the only use of _update_pid outside of Cluster.pm. To avoid this, the attached adds fail_ok functionality to restart() which makes it easier to use it in tests, and aligns it with how stop() and start() works. The resulting SSL tests are also more readable IMO. -- Daniel Gustafsson
Вложения
Re: Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
От
Melih Mutlu
Дата:
Hi Daniel, Thanks for the patch. Daniel Gustafsson <daniel@yesql.se>, 31 May 2023 Çar, 15:48 tarihinde şunu yazdı: > > To avoid this, the attached adds fail_ok functionality to restart() which makes > it easier to use it in tests, and aligns it with how stop() and start() works. > The resulting SSL tests are also more readable IMO. I agree that it's more readable this way. I only have a few comments: > > + BAIL_OUT("pg_ctl restart failed") unless $params{fail_ok}; > + return 0; > + } > > > $self->_update_pid(1); > return; I was comparing this new restart function to start and stop functions. I see that restart() does not return a value if it's successful while others return 1. Its return value is not checked anywhere, so it may not be useful at the moment. But I feel like it would be nice to make it look like start()/stop(). What do you think? > command_fails( > [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ], > 'restart fails with incorrect SSL protocol bounds'); There are two other places where ssl tests restart the node like above. We can call $node->restart in those lines too. Best, -- Melih Mutlu Microsoft
Re: Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
От
Daniel Gustafsson
Дата:
> On 31 May 2023, at 15:46, Melih Mutlu <m.melihmutlu@gmail.com> wrote: > I was comparing this new restart function to start and stop functions. > I see that restart() does not return a value if it's successful while > others return 1. > Its return value is not checked anywhere, so it may not be useful at > the moment. But I feel like it would be nice to make it look like > start()/stop(). What do you think? It should absolutely return 1, that was an oversight. Fixed as well as documentation updated. >> command_fails( >> [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ], >> 'restart fails with incorrect SSL protocol bounds'); > > There are two other places where ssl tests restart the node like > above. We can call $node->restart in those lines too. Fixed in the attached v2. -- Daniel Gustafsson
Вложения
Re: Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
От
Heikki Linnakangas
Дата:
On 31/05/2023 15:47, Daniel Gustafsson wrote: > The SSL tests for pg_ctl restarts with an incorrect key passphrase run pg_ctl > manually and use the internal method _update_pid to set the server PID file > accordingly. This is needed since $node->restart will BAIL in case the restart > fails, which clearly isn't useful to anyone wanting to test restarts. This is > the only use of _update_pid outside of Cluster.pm. > > To avoid this, the attached adds fail_ok functionality to restart() which makes > it easier to use it in tests, and aligns it with how stop() and start() works. > The resulting SSL tests are also more readable IMO. Makes sense. > diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl > index 76442de063..e33f648aae 100644 > --- a/src/test/ssl/t/001_ssltests.pl > +++ b/src/test/ssl/t/001_ssltests.pl > @@ -85,10 +85,8 @@ switch_server_cert( > passphrase_cmd => 'echo wrongpassword', > restart => 'no'); > > -command_fails( > - [ 'pg_ctl', '-D', $node->data_dir, '-l', $node->logfile, 'restart' ], > - 'restart fails with password-protected key file with wrong password'); > -$node->_update_pid(0); > +$result = $node->restart(fail_ok => 1); > +is($result, 0, 'restart fails with password-protected key file with wrong password'); > > switch_server_cert( > $node, In principle, this makes the tests more lenient. If "pg_ctl restart" fails because of a timeout, for example, the PID file could be present. Previously, the _update_pid(0) would error out on that, but now it's accepted. I think that's fine, but just wanted to point it out. -- Heikki Linnakangas Neon (https://neon.tech)
Re: Refactor ssl tests to avoid using internal PostgreSQL::Test::Cluster methods
От
Daniel Gustafsson
Дата:
> On 3 Jul 2023, at 13:37, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > If "pg_ctl restart" fails because of a timeout, for example, the PID file could be present. Previously, the _update_pid(0)would error out on that, but now it's accepted. I think that's fine, but just wanted to point it out. Revisiting an old patch. Agreed, I think that's fine, so I went ahead and pushed this. Thanks for review! It's unfortunately not that common for buildfarm animals to run the SSL tests, so I guess I'll keep an extra eye on the CFBot for this one. -- Daniel Gustafsson