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.
> 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.
