On Mon, Nov 25, 2019 at 11:22 PM vignesh C <vignes...@gmail.com> wrote: > > On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule <pavel.steh...@gmail.com> wrote: > > > > > > > > ne 24. 11. 2019 v 11:25 odesílatel vignesh C <vignes...@gmail.com> napsal: > >> > >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapil...@gmail.com> > >> wrote: > >> > > >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignes...@gmail.com> wrote: > >> > > > >> > > Thanks for fixing the comments. The changes looks fine to me. I have > >> > > fixed the first comment, attached patch has the changes for the same. > >> > > > >> > > >> > Few comments: > >> > -------------------------- > >> > 1. There is a lot of duplicate code in this test. Basically, almost > >> > the same code is used once to test Drop Database command and dropdb. > >> > I think we can create a few sub-functions to avoid duplicate code, but > >> > do we really need to test the same thing once via command and then by > >> > dropdb utility? I don't think it is required, but even if we want to > >> > do so, then I am not sure if this is the right test script. I suggest > >> > removing the command related test. > >> > > >> > >> Pavel: What is your opinion on this? > > > > > > I have not any problem with this reduction. > > > > We will see in future years what is benefit of this test. > > > > Fixed, removed dropdb utility test. >
Hmm, you have done the opposite of what I have asked. This test file is in rc/bin/scripts/t/ where we generally keep the tests for utilities. So, retaining the dropdb utility test in that file seems more sensible. +ok( TestLib::pump_until( + $killme, $psql_timeout, \$killme_stdout, qr/[[:digit:]]+[\r\n]$/m), + 'acquired pid'); How about changing 'acquired pid' to 'acquired pid for SIGTERM'? > >> > > >> > >> I have verified by running perltidy. > >> I think we don't need to run perltidy on the existing code. So, I am not sure if it is a good idea to run it for file 013_crash_restart.pl as it changes some existing code formatting. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com