Re: pgbench tap tests & minor fixes

В письме от 20 апреля 2017 19:14:34 пользователь Fabien COELHO написал:

> >> (1) extends the existing perl tap test infrastructure with methods to
> >> test
> >> pgbench, i.e. "pgbench" which runs a pgbench test and "pgbench_likes"
> >> which allows to check for expectations.
> >
> > I do not think it is good idea adding this functions to the
> > PostgresNode.pm.
> I thought it was:-)
>
> > They are pgbench specific.
>
> Sure.
>
> > I do not think anybody will need them outside of pgbench tests.
>
> Hmmm. The pre-existing TAP test in pgbench is about concurrent commits, it
> is not to test pgbench itself. Pgbench allows to run some programmable
> clients in parallel very easily, which cannot be done simply otherwise. I
> think that having it there would encourage such uses.
Since none of us is Tom Lane, I think we have no moral right to encourage
somebody doing somebody in postgres. People do what they think better to do.
If somebody need this functions for his tests, he/she can move them to public
space. If somebody sure he would use them soon, and tell about it in this
list, it would be good reason to make them public too. Until then I would offer
to consider these function private business of pgbench testing and keep them
somewhere inside src/bin/pgbench directory

> Another point is that the client needs informations held as attributes in
> the node in order to connect to the server. Having it outside would mean
> picking the attributes inside, which is pretty unclean as it breaks the
> abstraction. For me, all pg standard executables could have their methods
> in PostgresNode.pm.
you are speaking about
local $ENV{PGPORT} = $self->port;
?
Why do you need it here after all? Lots of TAP tests for bin utilities runs
them using command_like function from TestLib.pm and need no setting
$ENV{PGPORT}. Is pgbench so special? If it is so, may be it is good reason to
fix utilities from TestLib.pm, so they can take port from PostgresNode.

> Finally, it does not cost anything to have it there.
The price is maintainability. If in these tests we can use command_like
instead of pgbench_likes it would increase maintainability of the code.

> > May be you should move some code into some kind of helpers and keep them
> > in
> > PostgresNode.pm.
>
> Hmmm. I can put a "run any client" function with the same effect and pass
> an additional argument to tell that pgbench should be run, but this looks
> quite contrived for no special reason.
I read the existing code more carefully now, and as far as I can see most
console utilities uses command_like for testing. I think the best way would be
to use it for testing. Or write a warping around it, if we need additional
specific behavior for pgbench.

If we need more close integration with PostgresNode then command_like
provides, then may be we should improve command_like or add another function
that provides things that are needed.

> > Or write universal functions that can be used for testing any postgres
> > console tool. Then they can be placed in PostgresNode.pm
>
> There are already "psql"-specific functions... Why not pgbench specific
> ones?
psql -- is a core utility for postgres node. pgbench is optional.

> >> (3) add a lot of new very small tests so that coverage jumps from very
> >> low
> >> to over 90% for source files. [...]
> >
> > What tool did you use to calculate the coverage?
>
> I followed the documentated recipee:
>
> https://www.postgresql.org/docs/devel/static/regress-coverage.html
Thanks...

> > Lots of small test looks good at first glance, except the point that
> > adding
> > lots of small files with pgbench scripts is not great idea. This makes
> > code
> > tree too overloaded with no practical reason. I am speaking of
> > src/bin/pgbench/t/scripts/001_0* files.
> >
> > I think all the data from this file should be somehow imported into
> > 001_pgbench.pl and these files should be created only when tests is
> > running, and then removed when testing is done.
>
> Hmmm. I thought of that but was very unconvinced by the approach: pgbench
> basically always requires a file, so what the stuff was doing was writting
> the script into a file, checking for possible errors when doing so, then
> unlinking the file and checking for errors again after the run.
I think I was wrong about deleting file after tests are finished. Better keep
them.

> Then you
> have to do some escaping the the pgbench script themselves, and the perl
> script becomes pretty horrible and unreadable with plenty of perl, SQL,
> backslash commands in strings...
Perl provides a lot of tools for escaping. If once chooses the right one,
there would be no need in additional backslashes.

> Finally, if the script is inside the perl
> script it makes it hard to run the test outside of it when a problem is
> found, so it is a pain.
I've took back my words about deleting. After a first run one will have these
files "in flesh" so they would be available for further experiments.

I would speak again about maintainability. Having 36 files, most of them <100b
size -- is a very bad idea for maintainability. If each commit of new
functionality would add 36 small files, we will drown in these files quite soon.
These files should be generated on fly. I am 100% sure of it.

The way they are generated may vary... I would prefer to have the script
source code written close to the test that uses it, where it is possible, but
this is just my wishes.

PS. I've read the perl code through much more carefully. All other things are
looking quite good to me.

--
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.