Hello Nikolay,
- I agree to add a generic command TestLib & a wrapper in PostgresNode,
instead of having pgbench specific things in the later, then call
them from pgbench test script.
- I still think that moving the pgbench scripts inside the test script
is a bad idea (tm).
My sum up is the following:
I see my job as a reviewer is to tell "I've read the code, and I am sure
it is good".
I'm ok with that. Does not mean I cannot argue if I happen to disagree on
a point.
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.
ISTM that v2 I sent does not contain any pgbench specific code. However It
contains new generic code to check for status and list of re on stdout &
stderr.
- 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.
There is no call to IPC out of TestLib.pm in v2 I sent.
- 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...
I will write an ugly stuff if it is require to pass this patch, but I'm
unconvinced that this is a good idea.
What it is issue with having 36 small files in a directory?
Pg source tree currently contains about 79 under 100 bytes files related
to the insufficient test it is running, so this did not seem to be an
issue in the past.
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...
Of all the issues you list, 2/3 are already fixed in the v2 I sent
attached to the mail you are responding to, and I actually think that the
last point is a bad idea, which I can implement and be sad about.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
