Fabien COELHO <coelho@cri.ensmp.fr> writes:
> 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.
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.
* 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.
* 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.
* 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? 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.
regards, tom lane
