Hello Khushboo, Thanks for the changes here. Now everything looks good, and the tests all pass.
Thanks Joao On Tue, Mar 6, 2018 at 5:09 AM Khushboo Vashi < khushboo.va...@enterprisedb.com> wrote: > On Mon, Mar 5, 2018 at 8:42 PM, Joao De Almeida Pereira < > jdealmeidapere...@pivotal.io> wrote: > >> 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 >> >> >> Thanks Joao. > Please find the attached updated patch. > >> The tests run successfully in our CI pipeline. >> >> Thanks >> Joao >> >> Thanks, > Khushboo > >> 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 >>>> >>>
