1. This patch makes assorted cosmetic and non-cosmetic changes to pgbench.c. That is not expected for a testing patch.
Indeed, cosmetic changes should be avoided.
If those changes need to be made because they are bug fixes or whatever,
Yep, this is the case, minor bugs, plus comment I added to make clear things I had to guess when doing the debug. There are thread & clients stats, and they were not well managed.
then they should be committed separately.
Fine with me. Note that the added tests check that the bugs/issues are fixed. That would suggest (1) apply the pretty small bug fixes to pgbench and (2) add the test, in order to avoid adding non working tests, or having to change test afterwards.
A bunch of them look cosmetic and could be left out or all committed together, according to the committer's discretion, but the functional ones shouldn't just be randomly included into a commit that says "add TAP tests for pgbench".
The only real cosmectic one is the "." added to the comment. Others are useful comments which helped debug.
2. I agree that the way the expression evaluation tests are structured, with lots of small files, is not great. The problem with it in my view is not so much that it creates a lot of small files, but that you end up with half of the test definition in 001_pgbench.pl and the other half spread across all of those small files.
It'd be easy to end up with those things getting out of sync (small files that aren't in @errors, for example)
Hmmm. They are not expected to change, mostly new tests and files should be added when new features are added.
and isn't real evident on a quick glance how those files actually get used.
Sure. This is also more or less true of existing tests and has not been a problem before.
I think it would be better to move the expected output into @errors instead of having a separate file for it in each case.
The files do not contain the "expected output", they are the pgbench scripts to run through pgbench with the -f option.
The problem I see with the alternative is to have the contents of plenty pgbench scripts (sql comments + sql commands + backslash commands) stored in the middle of a perl script making it pretty unreadable, having to write and remove files on each test, having problems with running a given test again if it fails because the needed files are not there and have to be thought for in the middle of the perl script or not have been cleaned up by the test script which is not very clean...
So between "not great" and "quite bad", I chose "not great". I can do "quite bad", but I would prefer to avoid it:-)
3. The comment for check_pgbench_logs() is just "... with logs", which, at least to me, is not at all clear.
4. With this patch, we end up with 001_pgbench.pl and 002_basic.pl. Now, those file names seem completely uninformative to me.
I can rename them. "001_pgbench.pl" is just the pre-existing one that I just edited. The "basic" is a new one with basic option error tests, replicating what is done in other TAP tests.
would provide about the same amount of information; I think we can do better. Maybe call it 002_without_server or something; that actually explains the distinction.
Ok. Why not.
5. I don't think adding something like command_likes() is a problem particularly; it's similar to command_like() which we have already got, and seems like a reasonable extension of the same idea. But I don't like the fact that the code looks quite different,
Indeed, I put comments:-) Otherwise it does not do the same thing.
and it seems like it might be better to just extend command_like() and command_fails_like to each allow $expected_stdout to optionally be an array of patterns that are tested in turn rather than just a single pattern. That seems better than adding another very similar function.
It is not so similar: I want to test (1) precise exit status, (2) one or range of re on stdout, and (3) one or range of re on stderr.
The available commands just allow to check stdout once if status is ok, OR stderr if status is not ok. No functions check for the precise status, no functions check for a range of re, no functions checks for both stdout & stderr, or only in special cases (help, version, invalid option...).
Basically what I do does not fit any function prototype cleanly, so adding a new functions looked like the right approach. Probably a better name could be thought of.
-- Fabien. -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers