On Mon, Nov 25, 2019 at 11:22 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sun, Nov 24, 2019 at 5:06 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> >
> >
> >
> > ne 24. 11. 2019 v 11:25 odesílatel vignesh C <vignesh21@gmail.com> napsal:
> >>
> >> On Sat, Nov 23, 2019 at 4:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> >
> >> > On Fri, Nov 22, 2019 at 3:16 PM vignesh C <vignesh21@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
