Thanks - patch applied. On Tue, Mar 6, 2018 at 2:57 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote:
> 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 >>>>> >>>> -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
