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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
