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