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?
> 2.
> ok( TestLib::pump_until(
> + $killme,
> + $psql_timeout,
> + \$killme_stderr,
> + qr/FATAL: terminating connection due to administrator command/m
> + ),
> + "psql query died successfully after SIGTERM");
>
> Extra space before TestLib.
Ran perltidy, perltidy adds an extra space. I'm not sure which version
is right whether to include space or without space. I had noticed
similarly in 001_stream_rep.pl, in few places space is present and in
few places it is not present. If required I can update based on
suggestion.
>
> 3.
> +=item pump_until(proc, psql_timeout, stream, untl)
>
> I think moving pump_until to TestLib is okay, but if you are making it
> a generic function to be used from multiple places, it is better to
> name the variable as 'timeout' instead of 'psql_timeout'
>
Fixed.
> 4. Have you ran perltidy, if not, can you please run it? See
> src/test/perl/README for the recommendation.
>
I have verified by running perltidy.
Please find the updated patch attached. 1st patch is same as previous,
2nd patch includes the fix for comments 2,3 & 4.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
