On 2 March 2016 at 10:07, Craig Ringer <cr...@2ndquadrant.com> wrote:
> On 2 March 2016 at 05:46, Alvaro Herrera <alvhe...@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.... -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services