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.
>>
>> > 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.
>>
Attached patch handles the fix for the above comments.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
