Re: pgbench tap tests & minor fixes

On Mon, Apr 24, 2017 at 4:45 PM, Nikolay Shaplov <dhyan(at)nataraj(dot)su> wrote:
> I can tell this about this code, if:
>
> - There is no pgbench specific staff in src/test/perl. Or there should be
> _really_big_ reason for it.
>
> - All the testing is done via calls of TestLib.pm functions. May be these
> functions should be improved somehow. May be there should be some warper
> around them. But not direct IPC::Run::run call.
>
> - All the pgbench scripts are stored in one file. 36 files are not acceptable.
> I would include them in the test script itself. May be it can be done in other
> ways. But not 36 less then 100 byte files in source code tree...
>
>
> May be I am wrong. I am not a guru. But then somebody else should say "I've
> read the code, and I am sure it is good" instead of me. And it would be his
> responsibility then. But if you ask me, issues listed above are very
> important, and until they are solved I can not tell "the code is good", and I
> see no way to persuade me. May be just ask somebody else...

A few things that I notice:

1. This patch makes assorted cosmetic and non-cosmetic changes to
pgbench.c. That is not expected for a testing patch. If those
changes need to be made because they are bug fixes or whatever, then
they should be committed separately. 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".

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. It'd be easy
to end up with those things getting out of sync (small files that
aren't in @errors, for example) and isn't real evident on a quick
glance how those files actually get used. I think it would be better
to move the expected output into @errors instead of having a separate
file for it in each case.

3. The comment for check_pgbench_logs() is just "... with logs",
which, at least to me, is not at all clear.

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 assume
that if the pgbench directory has tests in a file called "pgbench",
that's either because there is only one file of tests, or because it
contains the most basic tests. But then we have another file called
"basic". So basically these could be called "foo" and "bar" and it
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.

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

I generally support this effort to improve test coverage of pgbench.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company