Hello Khushboo, Looks like we are almost doen, just missing one PEP-8 issue: → pycodestyle --config=.pycodestyle pgadmin/tools pgadmin/tools/sqleditor/tests/test_poll_query_tool.py:46: [E126] continuation line over-indented for hanging indent 1 E126 continuation line over-indented for hanging indent 1
The tests run successfully in our CI pipeline. Thanks Joao On Sun, Mar 4, 2018 at 11:51 PM Khushboo Vashi < khushboo.va...@enterprisedb.com> wrote: > On Fri, Mar 2, 2018 at 6:55 PM, Dave Page <dp...@pgadmin.org> wrote: > >> Could you rebase this please? It no longer applies. >> >> Please find the attached updated patch. > >> 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=commitdiff;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 >> >
