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

Hello Tom,

>> As for included bug fixes, I can do separate patches, but I think that it
>> is enough to first commit the pgbench files and then the tap-test files,
>> in that order. I'll see what committers say.
>
> Starting to look at this.  I concur that the bug fixes ought to be
> committed separately, but since they're in separate files it's not hard
> to disassemble the patch.

Ok.

> A couple of thoughts --
>
> * I do not think we need expr_scanner_chomp_substring.  Of the three
> existing callers of expr_scanner_get_substring, one is doing a manual
> chomp afterwards, and the other two need chomps per your patch.
> Seems to me we should just include the chomp logic in
> expr_scanner_get_substring.  Maybe it'd be worth adding a "bool chomp"
> argument to it, but I think that would be more for clarity than
> usefulness.

Ok. I thought that I would get a slap on the hand if I changed the initial 
function, but I get one not for changing it:-)

> * I do not like the API complexity of command_checks_all, specifically
> not the business of taking either a scalar or an array for the cmd,
> out, and err arguments.  I think it'd be clearer and less bug-prone
> to just take arrays, full stop.  Maybe it's just that I'm a crummy Perl
> programmer, but I do not see uses of this "ref ... eq 'ARRAY'" business
> elsewhere in our test scaffolding, and this doesn't seem like a great
> place to introduce it.  At worst you'd need to add brackets around the
> arguments in a few callers.

Hmmm. I find it quite elegant and harmless, but it can be removed.

> * In the same vein, I don't know what this does:
>
>     sub pgbench($$$$$)
>
> and I see no existing instances of it elsewhere in our tree.  I think it'd
> be better not to require advanced degrees in Perl programming in order to
> read the test cases.

It just says that 5 scalars are expected, so it would complain if "type" 
or number do not match. Now, why give type hints if they are not 
mandatory, as devs can always detect their errors by extensive testing 
instead:-)

But I agree that it is not a usual Perl stance and it can be removed.

> * Another way in which command_checks_all introduces API complexity is
> that it accounts for a variable number of tests depending on how many
> patterns are provided.  This seems like a mess.  I see that you have
> in some places (not very consistently) annotated call sites as to how
> many tests they account for, but who's going to do the math to make
> sure everything adds up?

Perl:-) I run the test, figure out the number it found in the resulting 
error message, and update the number in the source. Not too hard:-)

> Maybe it'd be better to not use like(), or do something else so that 
> each command_checks_all call counts as one Test::More test rather than 
> N.

> Or just forget prespecifying a test count and use done_testing 
> instead.

Yep, "done_testing" looks fine, I'll investigate that, but other test 
seemed to insist on declaring the expected number. Now "done_testing" may 
be a nicer option if some tests need to be skipped on some platform.

-- 
Fabien.