Re: [HACKERS] pgbench tap tests & minor fixes - Mailing list pgsql-hackers

Hello Robert,

> 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.

Yep.

> 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.

Indeed.

> 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.