В письме от 17 апреля 2017 14:51:45 пользователь Fabien COELHO написал:

> When developping new features for pgbench, I usually write some tests
> which are lost when the feature is committed. Given that I have submitted
> some more features and that part of pgbench code may be considered for
> sharing with pgsql, I think that improving the abysmal state of tests
> would be a good move.
Hi! Since I used to be good at perl, I like tests, and I've dealt with 
postgres TAP tests before, I think I can review you patch. If it is OK for 
everyone.

For now I've just gave this patch a quick look through... But nevertheless I 
have something to comment:

> The attached patch:
> 
> (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. 
They are pgbench specific. I do not think anybody will need them outside of 
pgbench tests. 

Generally, these functions as they are now, should be placed is separate .pm 
file. like it was done in src/test/ssl/

May be you should move some code into some kind of helpers and keep them in 
PostgresNode.pm. 

Or write universal functions that can be used for testing any postgres console 
tool. Then they can be placed in PostgresNode.pm

> (3) add a lot of new very small tests so that coverage jumps from very low
> to over 90% for source files. I think that derived files (exprparse.c,
> exprscan.c) should be removed from coverage analysis.
> 
> Previous coverage status:
> 
>   exprparse.y 0.0 %     0 / 77        0.0 %    0 / 8
>   exprscan.l  0.0 %     0 / 102       0.0 %    0 / 8
>   pgbench.c   28.3 %  485 / 1716      43.1 %  28 / 65
> 
> New status:
> 
>   exprparse.y 96.1 %    73 / 76       100.0 %          8 / 8
>   exprscan.l  92.8 %    90 / 97       100.0 %          8 / 8
>   pgbench.c   90.4 %  1542 / 1705      96.9 %         63 / 65
> 
> The test runtime is about doubled on my laptop, which is not too bad given
> the coverage achieved.
What tool did you use to calculate the coverage?

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.

I think that job for creating and removing these files should be moved to 
pgbench and pgbench_likes. But there is more then one way to do it. ;-)



That's what I've noticed so far... I will give this patch more attentive look 
soon.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to