Re: TAP / recovery-test fs-level backups, psql enhancements etc
| От | Craig Ringer |
|---|---|
| Тема | Re: TAP / recovery-test fs-level backups, psql enhancements etc |
| Дата | |
| Msg-id | CAMsr+YEPU5zVDorHoD3C_5j1ENfK+pfmVGv+PPsK-Jx50VRfBA@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: TAP / recovery-test fs-level backups, psql enhancements etc (Craig Ringer <craig@2ndquadrant.com>) |
| Список | pgsql-hackers |
On 2 March 2016 at 10:07, Craig Ringer <craig@2ndquadrant.com> wrote:
-- On 2 March 2016 at 05:46, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I think we should change the existing psql method to be what you propose
as psql_expert. I don't see any advantage in keeping the old one. Many
of the existing uses of psql should become what you call psql_check; but
we should probably call that psql_ok() instead, and also have a
psql_fails() for src/test/recovery/t/001_stream_rep.pl (and others to
come).I agree and that's what I really wanted to do. I just didn't want to produce a massive diff that renames the method across all of src/bin etc too, since I thought that'd be harder to commit and might have backporting consequences.If you think that's the way to go I'm very happy with that and will proceed.
I'll make the change you suggested to make 'psql_expert' into 'psql' and change call sites to use it or psql_check as appropriate. I'll probably make it an immediately following patch in the series so it's easier to separate the bulk-rename from the functional changes, but it can be trivially squashed for commit.
On reflection I want to keep the name as psql_check, rather than psql_ok. I'll insert another patch that changes src/bin to use psql_check where appropriate.
The reason I used _check rather than psql_ok is partly that psql_check isn't a test. It doesn't run any Test::More checks, it die()s on failure because failure isn't expected but is incidental to the test that's using psql. I did it that way because I don't think the psql invocation should be a test in its self - then you'd have to pass a test name to every psql_ok invocation and you'd get a flood of meaningless micro-tests showing up that obscure the real thing being tested. It'd also be a PITA maintaining the number of tests in the tests => 'n' argument to Test::More.
So I'm inclined to keep it as psql_check, to avoid confusion with the names 'ok' and 'fails' that Test::More uses. It's not actually a test. I don't think we need or should have a psql_ok wrapper, since with this change you can now just write:
is($node->psql('db', 'SELECT syntaxerror;'), 3, 'psql exits with code 3 on syntax error');
which is clearer and more specific than:
$node->psql_ok('db', 'SELECT syntaxerror;', test => 'psql exits on syntax error');
The reason I didn't do that is that the indenting in PostgresNode.pm is already well out of whack and, TBH, I didn't want to rebase on top of a perltidy'd version. I can bite the bullet and move the perltidy to the start of the patch series then make sure each subsequent patch is tidy'd but I'd want to be very sure you'd be OK to commit the perltidy of PostgresNode.pm otherwise I'd have to rebase messily all over again...
This wasn't as bad as I thought. I pulled the tidy changes to the $self->psql stuff into that patch and rebased the rest to the start of the series so it only touches what's currently committed. I agree that's better.
Updated tree pushed. I'll send a new patch series once I've done the psql_ok part.
It's funny that as part of implementing timeline following in logical decoding and implementing failover slots I'm writing perl test framework improvements....
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Craig RingerДата:
Сообщение: Re: TAP / recovery-test fs-level backups, psql enhancements etc