Thread: Removed unused import modules from tap tests
Hi, While trying to add some new tests, I found that PostgreSQL::Test::Utils is not required. I felt PostgreSQL::Test::Utils can be removed from a lot of tap tests which do not require it. I removed it, ran the tests and found the tests to be executing fine. I have made a patch including the changes for it. If this import can be removed, kindly accept the attached patch for the same. Regards, Vignesh
Attachment
On Tue, Nov 09, 2021 at 09:48:30PM +0530, vignesh C wrote: > While trying to add some new tests, I found that > PostgreSQL::Test::Utils is not required. I felt > PostgreSQL::Test::Utils can be removed from a lot of tap tests which > do not require it. I removed it, ran the tests and found the tests to > be executing fine. I have made a patch including the changes for it. > If this import can be removed, kindly accept the attached patch for > the same. I would not have bothered changing things if the names of the modules were the same across stable branches to minimize merge conflicts. However, everything has changed on HEAD, so there is a good argument for simplifying the tests as you are proposing here. Any thoughts from others? -- Michael
Attachment
On 2021-Nov-10, Michael Paquier wrote: > I would not have bothered changing things if the names of the modules > were the same across stable branches to minimize merge conflicts. > > However, everything has changed on HEAD, so there is a good argument > for simplifying the tests as you are proposing here. Any thoughts > from others? I agree with your reasoning, but I wonder what's the *benefit* of removing those includes. IOW, what's the reason not to simply drop the patch? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Nov-10, Michael Paquier wrote: > >> I would not have bothered changing things if the names of the modules >> were the same across stable branches to minimize merge conflicts. >> >> However, everything has changed on HEAD, so there is a good argument >> for simplifying the tests as you are proposing here. Any thoughts >> from others? > > I agree with your reasoning.. Agreed, if we are ever to do it now would be the time. > ..but I wonder what's the *benefit* of removing those includes. IOW, what's > the reason not to simply drop the patch? I think the value is mostly neatnikism, the actual effect on runtime is unlikely to be measureable. I won't argue against doing it, but I suspect we'll just slowly add a lot of these back as tests evolve making excercise less useful. -- Daniel Gustafsson https://vmware.com/
On Wed, Nov 10, 2021 at 6:07 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Nov-10, Michael Paquier wrote: > > > I would not have bothered changing things if the names of the modules > > were the same across stable branches to minimize merge conflicts. > > > > However, everything has changed on HEAD, so there is a good argument > > for simplifying the tests as you are proposing here. Any thoughts > > from others? > > I agree with your reasoning, but I wonder what's the *benefit* of > removing those includes. IOW, what's the reason not to simply drop the > patch? The idea was to clean up the unused import. I noticed that generally while adding new test files we copy from existing files, this results in existing unused imports also being added when new tests are added. It is just a cleanup activity to remove it from the existing code and probably can be taken care during the review so that it does not get added in the new tests. Regards, Vignesh
Daniel Gustafsson <daniel@yesql.se> writes:
>> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> ..but I wonder what's the *benefit* of removing those includes. IOW, what's
>> the reason not to simply drop the patch?
> I think the value is mostly neatnikism, the actual effect on runtime is
> unlikely to be measureable. I won't argue against doing it, but I suspect
> we'll just slowly add a lot of these back as tests evolve making excercise
> less useful.
Yeah, that last was pretty much my reaction. I don't know enough about
Perl to be sure how much an unused import costs, but I suspect you're
right that it won't be measurable in context, considering that most of
these test scripts run at least one initdb.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Daniel Gustafsson <daniel@yesql.se> writes: >>> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >>> ..but I wonder what's the *benefit* of removing those includes. IOW, what's >>> the reason not to simply drop the patch? > >> I think the value is mostly neatnikism, the actual effect on runtime is >> unlikely to be measureable. I won't argue against doing it, but I suspect >> we'll just slowly add a lot of these back as tests evolve making excercise >> less useful. > > Yeah, that last was pretty much my reaction. I don't know enough about > Perl to be sure how much an unused import costs, but I suspect you're > right that it won't be measurable in context, considering that most of > these test scripts run at least one initdb. On my laptop (using the "performance" CPU frequency governor to avoid scaling noise) loading PostgreSQL::Test::Utils takes about 80ms: $ time perl -I./src/test/perl -e 'use PostgreSQL::Test::Utils;' real 0m0.079s user 0m0.078s sys 0m0.001s However, if we're also loading PostgreSQL::Test::Cluster, the difference is negligible, because most of the time is spent loading modules used by both: $ time perl -I./src/test/perl -e 'use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils;' real 0m0.092s user 0m0.088s sys 0m0.004s $ time perl -I./src/test/perl -e 'use PostgreSQL::Test::Cluster;' real 0m0.090s user 0m0.081s sys 0m0.008s - ilmari
On Wed, Nov 10, 2021 at 9:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yeah, that last was pretty much my reaction. I don't know enough about > Perl to be sure how much an unused import costs, but I suspect you're > right that it won't be measurable in context, considering that most of > these test scripts run at least one initdb. I think focusing on the runtime might be missing the point to some extent. Removing used #include directives from C source files is generally good practice just for cleanliness, and this is the same kind of thing. That said, the argument that future changes are likely to just put the same 'use' directive back seems to me to have some merit. But we've probably spent more energy debating this than the topic deserves. It wouldn't hurt to remove these, and it also won't hurt if we don't. -- Robert Haas EDB: http://www.enterprisedb.com
On 11/10/21 09:53, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 10 Nov 2021, at 13:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>> ..but I wonder what's the *benefit* of removing those includes. IOW, what's
>>> the reason not to simply drop the patch?
>> I think the value is mostly neatnikism, the actual effect on runtime is
>> unlikely to be measureable. I won't argue against doing it, but I suspect
>> we'll just slowly add a lot of these back as tests evolve making excercise
>> less useful.
> Yeah, that last was pretty much my reaction. I don't know enough about
> Perl to be sure how much an unused import costs, but I suspect you're
> right that it won't be measurable in context, considering that most of
> these test scripts run at least one initdb.
>
>
:Cluster uses :Utils, and perl is smart enough not to try to reprocess
the module. Thus the extra cost here is almost certainly very close to zero.
This is a perfectly reasonable piece of boilerplate to use at the top of
a TAP test:
use strict;
use warnings;
use PostgreSQL:Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com