On Thu, Feb 4, 2016 at 5:23 PM, Stas Kelvich <s.kelv...@postgrespro.ru> wrote:

(Please do not top-post, this breaks the thread flow.)

> I’ve looked over proposed patch and migrated my shell tests scripts that i’ve 
> used for testing twophase commits on master/slave to this test framework. 
> Everything looks mature, and I didn’t encountered any problems with writing 
> new tests using this infrastructure.
> From my point of view I don’t see any problems to commit this patches in 
> their current state.

Thanks for the review!

> 0) There are several routines that does actual checking, like 
> is/command_ok/command_fails. I think it will be very handy to have wrappers 
> psql_ok/psql_fails that calls psql through the command_ok/fails.

Do you have a test case in mind for it?

> 1) Better to raise more meaningful error when IPC::Run is absent.

This has been discussed before, and as far as I recall the current
behavior has been concluded as being fine. That's where
--enable-tap-tests becomes meaningful.

> 2) --enable-tap-tests deserves mention in test/recovery/README and more 
> obvious error message when one trying to run make check in test/recovery 
> without --enable-tap-tests.

When running without --enable-tap-tests from src/test/recovery you
would get the following error per how prove_check is defined:
"TAP tests not enabled"

> 3) Is it hard to give ability to run TAP tests in extensions?

Not really. You would need to enforce a check rule or similar. For the
recovery test suite I have mapped the check rule with prove_check.

> 4) It will be handy if make check will write path to log files in case of 
> failed test.

Hm, perhaps. The log files are hardcoded in log/, so it is not like we
don't know it. That's an argument for the main TAP suite though, not
really this series of patch.

> 5) psql() accepts database name as a first argument, but everywhere in tests 
> it is ‘postgres’. Isn’t it simpler to store dbname in connstr, and have 
> separate function to change database?
> 6) Clean logs on prove restart? Clean up tmp installations?

Those are issues proper to the main TAP infrastructure, though I agree
that we could improve things here, particularly for temporary
installations that get automatically... Hm... Cleaned up should a test
failure happen?

> 7) Make check sets PGPORT PG_REGRESS for prove. Is it necessary?

No, that's not needed (I think I noticed that at some point) and
that's a bug. We could live without setting it.

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to