Could you rebase this please? It no longer applies. Thanks.
On Thu, Mar 1, 2018 at 5:56 AM, Khushboo Vashi < khushboo.va...@enterprisedb.com> wrote: > Hi Joao, > > Thanks for reviewing. > > On Wed, Feb 28, 2018 at 8:55 PM, Joao De Almeida Pereira < > jdealmeidapere...@pivotal.io> wrote: > >> Hello Khushboo, >> After reviewing the patch I have the gut feeling that we do not have >> enough test coverage on this issue, specially due to the intricate while >> loop and conditions around the polling. >> I think that this deserve Unit tests around it, When I say Unit Test I am >> not talking about executing queries against the database, but do some >> stubbing of the database so that we can control the flow that we want. >> > You are right. It needs more unit testing. I have checked below scenarios: > 1. Returns 2 notices with data output > 2. Returns 1000 notices with data output > 3. No notices with data output > > By running above, I have checked, each time returned notices are accurate, > no old notices are getting appended, it does not affect with the amount of > messages (few, none or more). Also, with the updated patch, I have made > sure that all these queries run with the single transaction id (same > connection). > > So, please let me know if you think I can add more things to this. > >> >> > It is a temptation to try to always do a Feature Test to test what we want >> because it is "easier" to write and ultimately it is what users see, but >> while 1 Feature Test runs we can run 200 Unit Tests that give us much more >> confidence that the code is doing what we expect it to do. >> >> Right, so added regression tests instead of feature tests. > > This being said, I run the tests on the CI Pipeline and all tests pass. >> Running pycodestyle fails due to some line sizes on the >> psycopg2/__init__py. I believe that it is not what you changed, but since >> you were changing the file it can be fixed it is just: >> >> pgadmin/utils/driver/psycopg2/__init__.py:1276: [E501] line too long (81 >> > 79 characters) >> pgadmin/utils/driver/psycopg2/__init__.py:1277: [E501] line too long (91 >> > 79 characters) >> pgadmin/utils/driver/psycopg2/__init__.py:1282: [E501] line too long (81 >> > 79 characters) >> pgadmin/utils/driver/psycopg2/__init__.py:1283: [E501] line too long (91 >> > 79 characters) >> 4 E501 line too long (81 > 79 characters) >> >> Fixed. Thanks for pointing out. > >> >> Thanks >> Joao >> >> >> On Wed, Feb 28, 2018 at 6:49 AM Khushboo Vashi < >> khushboo.va...@enterprisedb.com> wrote: >> >>> On Mon, Feb 26, 2018 at 10:02 PM, Dave Page <dp...@pgadmin.org> wrote: >>> >>>> Argh, I ran some tests, but didn't spot any lost messages in the tests >>>> I ran. I'll revert the patch. >>>> >>>> Khushboo; >>>> >>>> Please look at the following: >>>> >>>> - Fix the patch so it doesn't drop messages. >>>> >>> Fixed. >>> By default, the notice attribute of the connection object of psycopg 2 >>> only stores 50 notices. Once it reaches to 50 it starts from 1 again. >>> To fix this I have changed the notice attribute from list to deque to >>> append more messages. Currently I have kept the maximum limit at a time of >>> the notice attribute is 100000 (in a single poll). >>> >>>> - Add regression tests to make sure it doesn't break in the future. >>>> This may require creating one or more functions the spew out a whole lot of >>>> notices, and then running a couple of queries and checking the output. >>>> >>> Added. With this regression test, the current code is failing which has >>> been taken care in this patch. >>> >>>> - Check the messages panel on the history tab. I just noticed it seems >>>> to only be showing an even smaller subset of the messages. >>>> >>> Tested and no issues found. >>> >>>> >>>> >>> Thanks. >>>> >>>> On Mon, Feb 26, 2018 at 4:23 PM, Murtuza Zabuawala < >>>> murtuza.zabuaw...@enterprisedb.com> wrote: >>>> >>>>> Sent bit early, >>>>> >>>>> You can run 'VACUUM FULL VERBOSE' in query tool and verify the >>>>> populated messages (pgAdmin3 vs. pgAdmin4). >>>>> >>>>> >>>>> On Mon, Feb 26, 2018 at 9:48 PM, Murtuza Zabuawala < >>>>> murtuza.zabuaw...@enterprisedb.com> wrote: >>>>> >>>>>> Hi Khushboo/Dave, >>>>>> >>>>>> With given commit, I'm again seeing the issue raised in >>>>>> https://redmine.postgresql.org/issues/1523 :( >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Regards, >>>>>> Murtuza Zabuawala >>>>>> EnterpriseDB: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>>> >>>>>> On Mon, Feb 26, 2018 at 7:49 PM, Dave Page <dp...@pgadmin.org> wrote: >>>>>> >>>>>>> Ensure we pick up the messages from the current query and not a >>>>>>> previous one. Fixes #3094 >>>>>>> >>>>>>> Branch >>>>>>> ------ >>>>>>> master >>>>>>> >>>>>>> Details >>>>>>> ------- >>>>>>> https://git.postgresql.org/gitweb?p=pgadmin4.git;a=commitdif >>>>>>> f;h=08b3ccc01a4d57e8ea3657f8882a53dcd1b99386 >>>>>>> Author: Khushboo Vashi <khushboo.va...@enterprisedb.com> >>>>>>> >>>>>>> Modified Files >>>>>>> -------------- >>>>>>> web/pgadmin/utils/driver/abstract.py | 1 + >>>>>>> web/pgadmin/utils/driver/psycopg2/__init__.py | 64 >>>>>>> +++++++++------------------ >>>>>>> 2 files changed, 21 insertions(+), 44 deletions(-) >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
