Hello Heikki,
I stared at the new test case for a while, and I must say it looks very
cryptic. It's not exactly this patch's fault - all the pgbench tests are
cryptic -
Perl is cryptic. Regexprs are cryptic.
but I think we need to do something about that before adding any more
tests. I'm not sure what exactly, but I'd like them to be more like
pg_regress tests, where you have an expected output and you compare it
with the actual output. I realize that's not easy, because there are a
lot of varying numbers in the output, but we've got to do something.
As a good first step, I wish the pgbench() function used named
arguments. [...]
You would have something like this:
my $elapsed = pgbench(
test_name => 'pgbench progress',
opts => '-T 2 -P 1 -l --aggregate-interval=1'
I do not like them much in perl because it changes the code significantly,
but why not. That would be another patch anyway.
A lighter but efficient option would be to add a few comments on the
larger calls, see attached.
My other complaint about the new test is that it does nothing to check
if the output looks sensible. That's even harder to test, so it's
probably not worth the trouble to try. But as it is, how much value does
the test really have?
I do not understand. The list of expected regexpr on stdout and stderr
*are* the checks out the outputs, and there quite a few plenty of them?
It would fail, if --progress caused pgbench to crash, or if no progress
reports were printed at all, but I can't get very excited about that.
I cannot help you on this one: tests are never exciting:-)
The point is to improve code coverage, inducing that if a changes breaks
something the test should help notice before field reports.
There is nothing "exciting" about that, indeed.
The current test status is that one could write a dozen abort() in the
code and it would pass the tests.
Current tests coverage is much too low all over the project, see
https://coverage.postgresql.org/ which looks abysmal to me. I would to put
pgbench in the green. I do think that the whole project should be in the
green when all tests are run. Although coverage is not everything there is
about testing, it is a good start.
--
Fabien.
